Refactor a few things out of util.ts - #4251
mbg merged 15 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
🟡 Changes recommended
The new CodeQL stub silently ignores supplied version-check overrides.
1 open issue 1 file not reviewed- lib/entry-points.js: Excluded by your content exclusion settings
Refactors shared helpers out of util.ts to reduce coupling and dependency cycles in the CodeQL Action.
Changes:
- Extracts error helpers and GitHub types into focused modules.
- Moves environment initialization onto Env.
- Moves CLI version comparison onto CodeQL and updates callers and tests.
| File | Description |
|---|---|
| src/util.ts | Removes relocated helpers and preserves compatibility exports. |
| src/upload-sarif-action.ts | Initializes the action environment through Env. |
| src/upload-lib.test.ts | Updates environment initialization. |
| src/tools-download.test.ts | Spies on the relocated error helper. |
| src/setup/repository.ts | Imports InstanceURL from its owning module. |
| src/setup/repository.test.ts | Updates environment initialization. |
| src/setup-codeql.test.ts | Updates environment initialization. |
| src/setup-codeql-action.ts | Initializes through the action environment. |
| src/init-action.ts | Uses the new initialization and version methods. |
| src/github.ts | Houses GitHub variant and version types. |
| src/feature-flags/offline-features.test.ts | Updates environment initialization. |
| src/feature-flags.ts | Uses the CodeQL version method. |
| src/feature-flags.test.ts | Updates environment initialization. |
| src/errors/index.ts | Centralizes error helpers and classes. |
| src/environment.ts | Adds initialization and simplifies environment types. |
| src/database-upload.test.ts | Updates environment initialization. |
| src/config-utils.ts | Uses the CodeQL version method. |
| src/codeql.ts | Adds version comparison and stub wiring. |
| src/codeql.test.ts | Updates environment initialization. |
| src/autobuild-action.ts | Passes the environment into status reporting. |
| src/api-client.test.ts | Updates environment initialization. |
| src/analyze-action.ts | Initializes through the action environment. |
| src/actions-util.test.ts | Tests initialization through Env. |
Review effort: Max
Sorry, something went wrong.
There was a problem hiding this comment.
🔵 Needs a closer look
The stub version check can disagree with its reported version, affecting version-gated tests.
No open issues 🎉
1 previously missed issueIn code that hasn't changed since last review
src/codeql.ts:434
Could this default implementation call the returned stub's getVersion(), as the real implementation does? createStubCodeQL({}).isVersionAtLeast("1.0.0") returns false even though getVersion() reports "1.0.0". It also ignores replacements of getVersion on the returned stub because it reads partialCodeql instead. Version-gated tests can therefore take a different branch from the one implied by the stub's reported version, unlike the previous helper.
1 resolved since last review 1 file not reviewed- lib/entry-points.js: Excluded by your content exclusion settings
Review effort: Max
Sorry, something went wrong.
mario-campos
left a comment
There was a problem hiding this comment.
LGTM! It would also be nice to have withTmpDir and withTmpFile moved to testing-utils.ts, as they are only used for testing IIRC.
Sorry, something went wrong.
Groundwork for other things. This is a pure refactoring PR that moves various things out of util.ts because util.ts is large and unfocused, and easily creates dependency cycles for other files that depend on it.