CodeRabbit Generated Unit Tests: Generate Unit Tests for PR Changes - #168
CodeRabbit Generated Unit Tests: Generate Unit Tests for PR Changes#168coderabbitai[bot] wants to merge 1 commit into
Conversation
Reviewer's GuideAdds Vitest configuration to run new Node-based CJS tests for CI workflows and the vulnerability-check script, and introduces comprehensive end-to-end-style tests for scripts/check-new-vulns.cjs plus structural regression tests for key GitHub Actions and CI config files. Sequence diagram for check-new-vulns.cjs test execution via child processsequenceDiagram
participant Vitest as Vitest_runner
participant Tests as check_new_vulns_test_cjs
participant Spawn as spawnSync
participant Script as check_new_vulns_cjs
Vitest->>Tests: run()
Tests->>Spawn: spawnSync(process.execPath, [SCRIPT_PATH, args])
Spawn->>Script: invoke script
Script-->>Spawn: return result
Spawn-->>Tests: status, stdout, stderr
Tests-->>Vitest: expect(status, stdout, stderr)
Flow diagram for updated Vitest multi-project configurationgraph TD
VitestConfig[vitest.config.ts] --> ServerProject[project ./server]
VitestConfig --> RootProject[project root]
RootProject --> NodeEnvironment[node test environment]
NodeEnvironment --> CjsTests[scripts/**/*.test.cjs]
NodeEnvironment --> GithubCiTests[.github/**/*.test.cjs]
CjsTests --> CheckNewVulnsScript[scripts/check-new-vulns.cjs]
GithubCiTests --> GithubWorkflows[workflow and CI YAML files]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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 1 issue, and left some high level feedback:
- The workflow-structure tests in
.github/ci-config.test.cjsrely on large, exact substring matches, which may be brittle to formatting-only changes; consider narrowing those checks to smaller, more targeted substrings or regexes around the key lines you care about (e.g., specificif:orcontinue-on-error:directives) to reduce false failures from harmless YAML reformatting. - The
branchesForhelper assumes a single-linebranches: [..]array; if any workflow later switches to multilinebranches:syntax, these tests will silently returnnulland fail in non-obvious ways—consider enhancing the parser or adding explicit assertions that the match succeeded before using the result.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The workflow-structure tests in `.github/ci-config.test.cjs` rely on large, exact substring matches, which may be brittle to formatting-only changes; consider narrowing those checks to smaller, more targeted substrings or regexes around the key lines you care about (e.g., specific `if:` or `continue-on-error:` directives) to reduce false failures from harmless YAML reformatting.
- The `branchesFor` helper assumes a single-line `branches: [..]` array; if any workflow later switches to multiline `branches:` syntax, these tests will silently return `null` and fail in non-obvious ways—consider enhancing the parser or adding explicit assertions that the match succeeded before using the result.
## Individual Comments
### Comment 1
<location path="vitest.config.ts" line_range="7-14" />
<code_context>
+ const jsonPath = writeFile(
+ "audit.json",
+ cargoAudit([
+ {
+ advisory: {
+ id: "RUSTSEC-2026-0001",
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Move `root` out of the nested `test` config so Vitest actually uses it as the project root.
In this inline project config, `root` is defined under `test`, so Vitest will likely ignore it and use the global root instead. To ensure this project is scoped correctly, define `root` at the project level and keep only test-specific options under `test`:
```ts
{
root: ".",
test: {
name: "root",
environment: "node",
include: ["scripts/**/*.test.cjs", ".github/**/*.test.cjs"],
},
}
```
```suggestion
{
root: ".",
test: {
name: "root",
environment: "node",
include: ["scripts/**/*.test.cjs", ".github/**/*.test.cjs"],
},
},
```
</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: { | ||
| name: "root", | ||
| root: ".", | ||
| environment: "node", | ||
| include: ["scripts/**/*.test.cjs", ".github/**/*.test.cjs"], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
suggestion (bug_risk): Move root out of the nested test config so Vitest actually uses it as the project root.
In this inline project config, root is defined under test, so Vitest will likely ignore it and use the global root instead. To ensure this project is scoped correctly, define root at the project level and keep only test-specific options under test:
{
root: ".",
test: {
name: "root",
environment: "node",
include: ["scripts/**/*.test.cjs", ".github/**/*.test.cjs"],
},
}| { | |
| test: { | |
| name: "root", | |
| root: ".", | |
| environment: "node", | |
| include: ["scripts/**/*.test.cjs", ".github/**/*.test.cjs"], | |
| }, | |
| }, | |
| { | |
| root: ".", | |
| test: { | |
| name: "root", | |
| environment: "node", | |
| include: ["scripts/**/*.test.cjs", ".github/**/*.test.cjs"], | |
| }, | |
| }, |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🔍 OpenCodeReview found 4 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| it("no longer marks the SonarQube Scan step as continue-on-error", () => { | ||
| expect(content).toContain( | ||
| [ | ||
| " - name: SonarQube Scan", | ||
| " id: sonar-scan", | ||
| " uses: SonarSource/sonarqube-scan-action", | ||
| ].join("\n"), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The test title claims to verify that continue-on-error is no longer present on the SonarQube Scan step, but the assertion only checks that three fixed lines exist (- name: SonarQube Scan, id: sonar-scan, uses: SonarSource/...). It does not assert the absence of continue-on-error: true from this step. If someone re-adds continue-on-error: true to this step (a regression the test claims to prevent), the assertion would still pass silently.
Suggestion: Explicitly verify that continue-on-error does not appear between the step name and the next step, or assert that the step's snippet does not contain continue-on-error: true.
Suggestion:
| it("no longer marks the SonarQube Scan step as continue-on-error", () => { | |
| expect(content).toContain( | |
| [ | |
| " - name: SonarQube Scan", | |
| " id: sonar-scan", | |
| " uses: SonarSource/sonarqube-scan-action", | |
| ].join("\n"), | |
| ); | |
| }); | |
| it("no longer marks the SonarQube Scan step as continue-on-error", () => { | |
| // Verify the step exists as expected | |
| const stepStart = content.indexOf("- name: SonarQube Scan"); | |
| expect(stepStart).toBeGreaterThan(-1); | |
| // Check that continue-on-error is NOT present within the SonarQube step | |
| const stepSnippet = content.slice( | |
| stepStart, | |
| content.indexOf("\n - name:", stepStart + 1) // up to the next step | |
| ); | |
| expect(stepSnippet).not.toContain("continue-on-error"); | |
| }); |
| it("only runs the SonarCloud PR comment job when the scan succeeded", () => { | ||
| const jobStart = content.indexOf("sonar-pr-comment:"); | ||
| const jobEnd = content.indexOf("\n dockerfile:"); | ||
| expect(jobStart).toBeGreaterThan(-1); | ||
| expect(jobEnd).toBeGreaterThan(jobStart); | ||
| const job = content.slice(jobStart, jobEnd); | ||
| expect(job).toContain( | ||
| "if: github.event_name == 'pull_request' && needs.sonar.result == 'success'", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The job boundary is detected by searching for \n dockerfile: as the end marker of the sonar-pr-comment job. This is fragile because:
- If a new job is inserted between
sonar-pr-commentanddockerfilein ci.yml, the slice will include unrelated content. - If the
dockerfilejob is renamed (e.g., toDockerfile-lint),indexOfreturns -1 and the test fails cryptically. - If the string
\n dockerfile:appears anywhere earlier in the file (e.g., in a comment or step name), the slice will be wrong.
Suggestion: Use a more robust boundary marker, such as searching for the next top-level job key pattern (\n [a-z]+: following the current job) or parsing job-level YAML boundaries.
Suggestion:
| it("only runs the SonarCloud PR comment job when the scan succeeded", () => { | |
| const jobStart = content.indexOf("sonar-pr-comment:"); | |
| const jobEnd = content.indexOf("\n dockerfile:"); | |
| expect(jobStart).toBeGreaterThan(-1); | |
| expect(jobEnd).toBeGreaterThan(jobStart); | |
| const job = content.slice(jobStart, jobEnd); | |
| expect(job).toContain( | |
| "if: github.event_name == 'pull_request' && needs.sonar.result == 'success'", | |
| ); | |
| }); | |
| it("only runs the SonarCloud PR comment job when the scan succeeded", () => { | |
| const jobStart = content.indexOf("sonar-pr-comment:"); | |
| expect(jobStart).toBeGreaterThan(-1); | |
| // Find the next top-level job (lines starting with 2 spaces + word + :) | |
| const afterJob = content.indexOf("\n ", jobStart + 1); | |
| const jobEnd = content.indexOf("\n ", afterJob + 1); | |
| // Or alternatively, use a more specific next-job-name lookup | |
| // const jobEnd = content.indexOf("\n dockerfile:"); | |
| if (jobEnd > jobStart) { | |
| const job = content.slice(jobStart, jobEnd); | |
| expect(job).toContain( | |
| "if: github.event_name == 'pull_request' && needs.sonar.result == 'success'", | |
| ); | |
| } | |
| }); |
| function branchesFor(content, triggerName) { | ||
| const match = content.match( | ||
| new RegExp(`${triggerName}:\\n\\s*branches: \\[([^\\]]+)\\]`), | ||
| ); | ||
| return match ? match[1] : null; | ||
| } |
There was a problem hiding this comment.
The branchesFor regex helper only supports inline array format (branches: [rebuild, develop]). If any workflow file is reformatted to use block-style branch lists (as ci.yml already does: branches:\n - main), the regex returns null and tests fail with a confusing Cannot read properties of null error.
While all currently tested files use inline format, this hidden assumption makes the tests brittle against YAML formatting changes. It contradicts the project's own acknowledgment in the test header that "the same lightweight approach" is used elsewhere — since ci.yml already uses a different format.
Suggestion: Either add a fallback regex for block-style branches, or document this limitation explicitly and add a defensive null check with a clear error message.
Suggestion:
| function branchesFor(content, triggerName) { | |
| const match = content.match( | |
| new RegExp(`${triggerName}:\\n\\s*branches: \\[([^\\]]+)\\]`), | |
| ); | |
| return match ? match[1] : null; | |
| } | |
| function branchesFor(content, triggerName) { | |
| // Try inline array format first: branches: [a, b, c] | |
| const inlineMatch = content.match( | |
| new RegExp(`${triggerName}:\\n\\s*branches: \\[([^\\]]+)\\]`), | |
| ); | |
| if (inlineMatch) return inlineMatch[1]; | |
| // Fallback: block format (branches:\n - a\n - b) | |
| const blockMatch = content.match( | |
| new RegExp(`${triggerName}:\\n\\s*branches:\\n((?:\\s+-\\s+\\S+\\n?)+)`), | |
| ); | |
| if (blockMatch) return blockMatch[1]; | |
| return null; | |
| } |
| expect(content).toContain( | ||
| [ | ||
| " - name: Post coverage gaps to PR", | ||
| ].join("\n"), | ||
| ); | ||
| const stepStart = content.indexOf("- name: Post coverage gaps to PR"); |
There was a problem hiding this comment.
The test uses toContain with exact leading spaces (" - name: Post coverage gaps to PR" — 6 spaces) to assert the step name exists, but then uses indexOf without leading spaces ("- name: Post coverage gaps to PR") to find the step boundary for further assertions. This inconsistency works today but is fragile: if the same step name appears elsewhere in the file with different indentation (e.g., in a different job), indexOf could locate the wrong occurrence.
Suggestion: Use consistent leading spaces in both toContain and indexOf calls.
Suggestion:
| expect(content).toContain( | |
| [ | |
| " - name: Post coverage gaps to PR", | |
| ].join("\n"), | |
| ); | |
| const stepStart = content.indexOf("- name: Post coverage gaps to PR"); | |
| expect(content).toContain( | |
| " - name: Post coverage gaps to PR", | |
| ); | |
| const stepStart = content.indexOf(" - name: Post coverage gaps to PR"); |
|
SonarCloud Analysis ✅No BLOCKER, CRITICAL, or MAJOR issues found. |
1 participant
Unit test generation was requested by @BillyOutlast.
The following files were modified:
.github/ci-config.test.cjsscripts/check-new-vulns.test.cjsvitest.config.tsSummary by Sourcery
Add Vitest configuration and unit-style tests to validate CI workflows and the vulnerability-checking script.
Tests: