Prefer providing CodeQL via dependency injection - #3011
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the CodeQL Action codebase to prefer dependency injection of CodeQL instances over using global cached instances. The change makes CodeQL usage more explicit and facilitates better testing by providing stub CodeQL objects directly to functions that need them.
- Replaces setCodeQL with createStubCodeQL to create stub instances without modifying global state
- Updates functions to accept CodeQL parameters instead of internally calling getCodeQL
- Modifies test files to pass stub CodeQL instances explicitly
Reviewed Changes
Copilot reviewed 28 out of 42 changed files in this pull request and generated no comments.
Show a summary per file| File | Description |
|---|---|
| src/trap-caching.test.ts | Updates test to use createStubCodeQL instead of setCodeQL |
| src/testing-utils.ts | Changes test utility to use createStubCodeQL |
| src/init-action-post.ts | Adds CodeQL parameter to initActionPostHelper.run call |
| src/init-action-post-helper.ts | Updates function signatures to accept CodeQL via dependency injection |
| src/init-action-post-helper.test.ts | Passes stub CodeQL instances to test functions |
| src/debug-artifacts.ts | Updates functions to accept CodeQL parameter instead of calling getCodeQL internally |
| src/database-upload.test.ts | Changes test to use createStubCodeQL |
| src/config-utils.test.ts | Updates multiple test cases to use createStubCodeQL and removes unused getCachedCodeQL calls |
| src/codeql.ts | Refactors setCodeQL to call createStubCodeQL and removes getCachedCodeQL function |
| src/analyze.ts | Updates runQueries to accept CodeQL parameter |
| src/analyze.test.ts | Updates test to use createStubCodeQL and pass CodeQL instance to runQueries |
| src/analyze-action.ts | Passes CodeQL instance to runQueries call |
| src/analyze-action-input.test.ts | Adds assertions to verify function calls |
| src/analyze-action-env.test.ts | Adds assertions to verify function calls |
| lib/* files | Generated JavaScript equivalents of TypeScript changes |
Sorry, something went wrong.
mbg
left a comment
There was a problem hiding this comment.
Looks like a nice change, definitely makes it clearer when the CodeQL object is used.
It would be nice if we could get rid of setCodeQL entirely.
Sorry, something went wrong.
| @@ -42,7 +42,7 @@ test("status report fields", async (t) => { | |||
| sinon.stub(uploadLib, "validateSarifFileSchema"); | |||
|
|
|||
| for (const language of Object.values(KnownLanguage)) { | |||
There was a problem hiding this comment.
Just an observation unrelated to this PR: KnownLanguage doesn't include "actions" anymore, so that doesn't get tested here.
Sorry, something went wrong.
Agreed, though this is trickier since we have a couple of tests that run the top-level Actions, namely src/analyze-action-env.test.ts and src/analyze-action-input.test.ts. It might be worth seeing if we can rewrite these without losing coverage, but I'll leave that as future work. |
Sorry, something went wrong.
This makes it more obvious when CodeQL is being used, and therefore when we might need to provide stubs in tests.
Merge / deployment checklist