Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor CodeQL setup tests and fix GHAE test #1474

Merged
merged 6 commits into from
Jan 11, 2023

Conversation

henrymercer
Copy link
Contributor

This PR refactors the CodeQL setup tests and fixes a GHAE setup test.

Refactoring of CodeQL setup tests

  • I've split up mockApiAndSetupCodeQL into mockDownloadApi, installIntoToolcache, and setupCodeQL so it's a easier to understand the intent of the tests.
  • I've renamed the tests to include assertions about expected behavior.
  • I've also renamed the bundle tag name from version to tagName to prepare for the controlled rollout changes, which will involve keeping track of both a CLI version and a bundle tag name.

Fix to GHAE setup test

  • Previously we weren't actually testing GHAE setup.
    • The GHAE API URL was missing, so calling the GHAE endpoint failed.
    • However the test didn't fail because a mock for the fallback Dotcom endpoint was hanging around from a previous test.
  • I have added the missing GHAE API URL and added a call to the test infrastructure to clear all nock network request mocks after each test, just as we clear all sinon mocks.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@henrymercer
Copy link
Contributor Author

Checks are failing on main — merging in main once #1466 is merged should fix.

@henrymercer henrymercer marked this pull request as ready for review January 10, 2023 21:10
@henrymercer henrymercer requested a review from a team as a code owner January 10, 2023 21:10
angelapwen
angelapwen previously approved these changes Jan 10, 2023
Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of small notes and there's a merge conflict with one of the source map files, but overall it looks great and clean! Thanks for doing all this refactoring ✨

async function mockApiAndSetupCodeQL({
apiDetails,
bypassToolcache,
async function mockDownloadApi({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it wasn't super clear to me what the Promise this function returned was, could we add a comment indicating it's returning the URL or rename the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, I've added some JSDoc.

@@ -93,8 +85,8 @@ async function mockApiAndSetupCodeQL({

const baseUrl = apiDetails?.url ?? "https://example.com";
const relativeUrl = apiDetails
? `/github/codeql-action/releases/download/${version}/codeql-bundle-${platform}.tar.gz`
: `/download/codeql-bundle-${version}/codeql-bundle.tar.gz`;
? `/github/codeql-action/releases/download/${tagName}/codeql-bundle-${platform}.tar.gz`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be clear — this is where you're renaming the bundle tag name in the tests from version to tagNamethat includes thecodeql-bundle-` prefix, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct!

Copy link
Contributor

@angelapwen angelapwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

@henrymercer henrymercer merged commit 70fdddf into main Jan 11, 2023
@henrymercer henrymercer deleted the henrymercer/fix-ghae-setup-test branch January 11, 2023 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants