-
Notifications
You must be signed in to change notification settings - Fork 422
Refactor CodeQL setup tests and fix GHAE test #1474
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
Conversation
|
Checks are failing on main — merging in main once #1466 is merged should fix. |
angelapwen
left a comment
There was a problem hiding this 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({ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| 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` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct!
angelapwen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!!
This PR refactors the CodeQL setup tests and fixes a GHAE setup test.
Refactoring of CodeQL setup tests
mockApiAndSetupCodeQLintomockDownloadApi,installIntoToolcache, andsetupCodeQLso it's a easier to understand the intent of the tests.versiontotagNameto 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
nocknetwork request mocks after each test, just as we clear allsinonmocks.Merge / deployment checklist