Conversation
justinkumpe
added
documentation
Reviewer's GuideAdds a full testing and CI setup for the Git QuickOps VS Code extension, including fast build tests integrated with Git QuickOps’ requireTests feature, VS Code integration tests, cross-platform GitHub Actions workflows, validation scripts, and corresponding documentation updates in README and CONTRIBUTING. Sequence diagram for Git QuickOps requireTests build test executionsequenceDiagram
actor Developer
participant GitQuickOpsHook
participant Shell
participant Npm
participant Node
participant BuildRunner
Developer->>GitQuickOpsHook: git commit
GitQuickOpsHook->>Shell: run configured requireTests command
Shell->>Npm: npm test
Note over Npm: npm test -> npm run test:build
Npm->>Npm: npm run compile
Npm->>Node: node -e require(./out/test/suite/buildRunner).run()
Node->>BuildRunner: load module buildRunner
BuildRunner->>BuildRunner: run()
BuildRunner-->>Node: build tests result
Node-->>Npm: exit code 0 or 1
Npm-->>GitQuickOpsHook: success or failure
alt tests passed
GitQuickOpsHook-->>Developer: allow commit
else tests failed
GitQuickOpsHook-->>Developer: block commit
end
Flow diagram for local validation scripts (validate.sh and validate.bat)flowchart TD
A[Start validation script] --> B[Install dependencies\n npm ci]
B --> C[Run ESLint\n npm run lint]
C --> D[Compile TypeScript\n npm run compile]
D --> E[Run build tests\n npm test]
E --> F[Validate package.json fields]
F --> G[Package extension\n npx @vscode/vsce package --no-dependencies]
G --> H[All validation checks passed]
H --> I[Extension ready to push or publish]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| name: Build and Test | ||
| runs-on: ${{ matrix.os }} | ||
|
|
||
| strategy: | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| node-version: [18.x, 20.x] | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js ${{ matrix.node-version }} | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: ${{ matrix.node-version }} | ||
| cache: 'npm' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Lint code | ||
| run: npm run lint | ||
|
|
||
| - name: Compile TypeScript | ||
| run: npm run compile | ||
|
|
||
| - name: Run build test | ||
| run: npm run test:build | ||
|
|
||
| - name: Package extension | ||
| run: npx @vscode/vsce package | ||
|
|
||
| - name: Upload VSIX artifact | ||
| uses: actions/upload-artifact@v4 | ||
| if: matrix.os == 'ubuntu-latest' && matrix.node-version == '20.x' | ||
| with: | ||
| name: git-quickops-vsix | ||
| path: '*.vsix' | ||
| retention-days: 7 | ||
|
|
||
| - name: Run integration tests (Ubuntu) | ||
| if: runner.os == 'Linux' | ||
| run: xvfb-run -a npm run test:integration | ||
|
|
||
| - name: Run integration tests (macOS) | ||
| if: runner.os == 'macOS' | ||
| run: npm run test:integration | ||
|
|
||
| - name: Run integration tests (Windows) | ||
| if: runner.os == 'Windows' | ||
| run: npm run test:integration | ||
|
|
||
| lint-check: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 21 hours ago
In general, to fix this issue you should explicitly set permissions for the workflow or individual jobs so that the GITHUB_TOKEN has only the minimal privileges required. For a typical CI workflow that just checks out code, installs dependencies, runs tests, and uploads artifacts, contents: read at the workflow level is usually sufficient. Additional scopes are only needed if the workflow writes to PRs, issues, releases, etc.
For this specific workflow, none of the jobs (build, lint-check, validate-package) perform write operations against the repository or GitHub’s APIs; they only read source code and push artifacts to the Actions storage (which does not use contents: write). Therefore, the single best fix is to add a root-level permissions block setting contents: read so it applies to all jobs. This keeps existing functionality unchanged while constraining the token.
Concretely:
- Edit
.github/workflows/ci.yml. - Insert a
permissions:block after thename:and beforeon::name: CI Build and Test permissions: contents: read on: ...
- No job-level permissions are necessary unless you later add steps that require broader access.
No extra imports or methods are needed; this is purely a YAML configuration change.
| @@ -1,5 +1,8 @@ | ||
| name: CI Build and Test | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ main, dev ] |
| name: Lint and Format Check | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20.x' | ||
| cache: 'npm' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Run ESLint | ||
| run: npm run lint | ||
|
|
||
| - name: Check TypeScript compilation | ||
| run: npm run compile | ||
|
|
||
| validate-package: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 21 hours ago
In general, the fix is to explicitly restrict GITHUB_TOKEN permissions using a permissions: block, either at the workflow root (affecting all jobs) or per job, granting only the scopes needed (typically contents: read for this kind of CI workflow). Since none of the jobs performs write operations to the repository or uses APIs that require elevated scopes, we can add a minimal permissions: block at the workflow root so all jobs inherit it.
The single best fix here is to add a root-level permissions: block right after the name: (or on:) section in .github/workflows/ci.yml, specifying contents: read. This applies to all jobs (build, lint-check, validate-package) without altering their behavior because they only read repository contents. No changes are necessary inside individual jobs, and no additional libraries or steps are required.
Concretely, in .github/workflows/ci.yml, between line 2 (blank) and line 3 (on:), insert:
permissions:
contents: readThis ensures that the GITHUB_TOKEN has read-only access to repository contents for the entire workflow, satisfying the CodeQL recommendation while preserving existing functionality.
| @@ -1,5 +1,8 @@ | ||
| name: CI Build and Test | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ main, dev ] |
| name: Validate Package | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '20.x' | ||
| cache: 'npm' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm ci | ||
|
|
||
| - name: Validate package.json | ||
| run: | | ||
| node -e "const pkg = require('./package.json'); console.log('Package validation passed');" | ||
|
|
||
| - name: Check for missing fields | ||
| run: | | ||
| node -e " | ||
| const pkg = require('./package.json'); | ||
| const required = ['name', 'displayName', 'version', 'publisher', 'engines', 'categories', 'main']; | ||
| const missing = required.filter(f => !pkg[f]); | ||
| if (missing.length) { | ||
| console.error('Missing required fields:', missing); | ||
| process.exit(1); | ||
| } | ||
| console.log('All required fields present'); | ||
| " | ||
|
|
||
| - name: Compile and package | ||
| run: | | ||
| npm run compile | ||
| npx @vscode/vsce package --no-dependencies | ||
|
|
||
| - name: Verify VSIX creation | ||
| run: | | ||
| if [ ! -f *.vsix ]; then | ||
| echo "VSIX file was not created" | ||
| exit 1 | ||
| fi | ||
| echo "VSIX file created successfully" |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 21 hours ago
In general, the fix is to explicitly declare a permissions block that restricts the GITHUB_TOKEN to the least privileges needed. Since this workflow only checks out code, installs dependencies, runs linting/testing, and packages artifacts, it only needs read access to repository contents. It does not need to write to contents, issues, pull requests, or other scopes.
The best fix, without changing functionality, is to add a single top-level permissions block (at the root of .github/workflows/ci.yml, alongside name and on). This will apply to all jobs (build, lint-check, and validate-package) uniformly. Set it to contents: read, which is the minimal scope required for actions/checkout@v4 to read the repository. No job appears to need any write permissions or additional scopes (such as packages, pull-requests, or statuses), and none of the used third-party actions require elevated permissions for their normal operation here.
Concretely:
- Edit
.github/workflows/ci.yml. - Insert a
permissions:section after thename: CI Build and Testline and before theon:block. - Set the permissions to:
permissions: contents: read
No additional methods, imports, or definitions are needed, and no steps need modification.
| @@ -1,5 +1,8 @@ | ||
| name: CI Build and Test | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| push: | ||
| branches: [ main, dev ] |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The CI workflow currently runs lint/compile in both the
buildandlint-checkjobs; consider consolidating these steps into a single job or reusing artifacts to avoid redundant work and speed up the pipeline. - The
validatenpm script usesbash scripts/validate.sh || scripts\validate.bat; on Windows without Bash installed this will still work but the initial error may be noisy—consider a small platform check (e.g., via Node) to invoke only the appropriate script per OS.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The CI workflow currently runs lint/compile in both the `build` and `lint-check` jobs; consider consolidating these steps into a single job or reusing artifacts to avoid redundant work and speed up the pipeline.
- The `validate` npm script uses `bash scripts/validate.sh || scripts\validate.bat`; on Windows without Bash installed this will still work but the initial error may be noisy—consider a small platform check (e.g., via Node) to invoke only the appropriate script per OS.
## Individual Comments
### Comment 1
<location> `package.json:425` </location>
<code_context>
"pretest": "npm run compile && npm run lint",
"lint": "eslint src --ext ts",
- "test": "node ./out/test/runTest.js"
+ "test": "npm run test:build",
+ "test:integration": "node ./out/test/runTest.js",
+ "test:build": "npm run compile && node -e \"require('./out/test/suite/buildRunner').run().then(() => process.exit(0)).catch((e) => { console.error(e); process.exit(1); })\"",
+ "test:compile": "tsc -p ./ && echo 'Build successful'",
+ "validate": "bash scripts/validate.sh || scripts\\validate.bat"
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid running compilation twice in the default `npm test` lifecycle.
As written, `npm test` will first run `pretest` (compile + lint) and then `test:build` (compile again). To avoid the duplicate compilation, either drop `npm run compile &&` from `test:build` and rely on `pretest`, or remove `pretest` and keep compilation only in `test:build`.
```suggestion
"test:build": "node -e \"require('./out/test/suite/buildRunner').run().then(() => process.exit(0)).catch((e) => { console.error(e); process.exit(1); })\"",
```
</issue_to_address>
### Comment 2
<location> `.github/workflows/ci.yml:39-40` </location>
<code_context>
+ - name: Compile TypeScript
+ run: npm run compile
+
+ - name: Run build test
+ run: npm run test:build
+
+ - name: Package extension
</code_context>
<issue_to_address>
**suggestion (performance):** Reduce duplicate compilation work in CI by reusing the existing compile step.
This job already runs `npm run compile`, and `test:build` also calls `npm run compile`, so each build compiles twice. Consider either removing the explicit `Compile TypeScript` step and relying on `test:build`, or updating `test:build` to assume compilation has already happened in CI and dropping its internal `npm run compile`.
</issue_to_address>
### Comment 3
<location> `scripts/validate.bat:35-36` </location>
<code_context>
+echo Compilation successful
+echo.
+
+echo Running build tests...
+call npm test
+if errorlevel 1 (
+ echo Build tests failed
</code_context>
<issue_to_address>
**suggestion (performance):** Batch validate script is also running lint/compile twice due to `npm test`’s lifecycle.
On Windows this runs `lint` and `compile` twice, since `npm test` already triggers them via `pretest`. To avoid duplicate work, either call `npm run test:build` directly here or drop the earlier `lint`/`compile` calls and rely on the `pretest` hook.
```suggestion
echo Running build tests...
call npm run test:build
```
</issue_to_address>
### Comment 4
<location> `src/test/suite/buildRunner.ts:28-31` </location>
<code_context>
+ return reject(err);
+ }
+
+ if (files.length === 0) {
+ console.log('No build tests found');
+ return resolve();
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider failing the build when no build tests are found instead of resolving successfully.
Silently succeeding when `files.length === 0` can hide the fact that build tests have been removed or misnamed, while CI still reports success. Consider rejecting in this case (or making it configurable) so that “no build tests found” is treated as a failure condition for pre-commit/validation runs.
```suggestion
if (files.length === 0) {
return reject(new Error('No build tests found'));
}
```
</issue_to_address>
### Comment 5
<location> `README.md:8` </location>
<code_context>
-A comprehensive git operations toolkit with an intuitive menu-driven interface, bringing the power of the git-quickops shell script directly into VS Code.
+A comprehensive git operations toolkit with both a visual sidebar interface and menu-driven command palette, bringing the power of the git-quickops shell script directly into VS Code with a native GUI experience.
## Features
</code_context>
<issue_to_address>
**suggestion (typo):** Consider adding an article before “menu-driven command palette” for smoother grammar.
You could rewrite this as “with both a visual sidebar interface and a menu-driven command palette …” to keep the list parallel and easier to read.
```suggestion
A comprehensive git operations toolkit with both a visual sidebar interface and a menu-driven command palette, bringing the power of the git-quickops shell script directly into VS Code with a native GUI experience.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "test": "node ./out/test/runTest.js" | ||
| "test": "npm run test:build", | ||
| "test:integration": "node ./out/test/runTest.js", | ||
| "test:build": "npm run compile && node -e \"require('./out/test/suite/buildRunner').run().then(() => process.exit(0)).catch((e) => { console.error(e); process.exit(1); })\"", |
There was a problem hiding this comment.
suggestion (performance): Avoid running compilation twice in the default npm test lifecycle.
As written, npm test will first run pretest (compile + lint) and then test:build (compile again). To avoid the duplicate compilation, either drop npm run compile && from test:build and rely on pretest, or remove pretest and keep compilation only in test:build.
| "test:build": "npm run compile && node -e \"require('./out/test/suite/buildRunner').run().then(() => process.exit(0)).catch((e) => { console.error(e); process.exit(1); })\"", | |
| "test:build": "node -e \"require('./out/test/suite/buildRunner').run().then(() => process.exit(0)).catch((e) => { console.error(e); process.exit(1); })\"", |
| - name: Run build test | ||
| run: npm run test:build |
There was a problem hiding this comment.
suggestion (performance): Reduce duplicate compilation work in CI by reusing the existing compile step.
This job already runs npm run compile, and test:build also calls npm run compile, so each build compiles twice. Consider either removing the explicit Compile TypeScript step and relying on test:build, or updating test:build to assume compilation has already happened in CI and dropping its internal npm run compile.
| echo Running build tests... | ||
| call npm test |
There was a problem hiding this comment.
suggestion (performance): Batch validate script is also running lint/compile twice due to npm test’s lifecycle.
On Windows this runs lint and compile twice, since npm test already triggers them via pretest. To avoid duplicate work, either call npm run test:build directly here or drop the earlier lint/compile calls and rely on the pretest hook.
| echo Running build tests... | |
| call npm test | |
| echo Running build tests... | |
| call npm run test:build |
| if (files.length === 0) { | ||
| console.log('No build tests found'); | ||
| return resolve(); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Consider failing the build when no build tests are found instead of resolving successfully.
Silently succeeding when files.length === 0 can hide the fact that build tests have been removed or misnamed, while CI still reports success. Consider rejecting in this case (or making it configurable) so that “no build tests found” is treated as a failure condition for pre-commit/validation runs.
| if (files.length === 0) { | |
| console.log('No build tests found'); | |
| return resolve(); | |
| } | |
| if (files.length === 0) { | |
| return reject(new Error('No build tests found')); | |
| } |
|  | ||
|
|
||
| A comprehensive git operations toolkit with an intuitive menu-driven interface, bringing the power of the git-quickops shell script directly into VS Code. | ||
| A comprehensive git operations toolkit with both a visual sidebar interface and menu-driven command palette, bringing the power of the git-quickops shell script directly into VS Code with a native GUI experience. |
There was a problem hiding this comment.
suggestion (typo): Consider adding an article before “menu-driven command palette” for smoother grammar.
You could rewrite this as “with both a visual sidebar interface and a menu-driven command palette …” to keep the list parallel and easier to read.
| A comprehensive git operations toolkit with both a visual sidebar interface and menu-driven command palette, bringing the power of the git-quickops shell script directly into VS Code with a native GUI experience. | |
| A comprehensive git operations toolkit with both a visual sidebar interface and a menu-driven command palette, bringing the power of the git-quickops shell script directly into VS Code with a native GUI experience. |
1 participant
Summary by Sourcery
Add a dedicated test and validation infrastructure for the Git QuickOps VS Code extension and document how to use it for local development and CI.
Enhancements:
Build:
CI:
Documentation:
Tests: