[codex] cover verifier report test gaps#5
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical test gaps in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request improves test coverage for the verifier report helpers, specifically for summary generation and file output logic. The changes are a good step forward. I've identified an opportunity to make the new summary preference test more comprehensive and maintainable by using a data-driven approach with it.each and adding missing test cases. This will ensure all summary logic branches are robustly covered.
| it("prefers the highest-severity summary branch", () => { | ||
| expect( | ||
| buildVerifyReportOutput({ | ||
| clarified: makeReport("semantically clarified but not fully proven"), | ||
| }).summary, | ||
| ).toBe("semantically clarified but not fully proven"); | ||
|
|
||
| expect( | ||
| buildVerifyReportOutput({ | ||
| proven: makeReport("proven working"), | ||
| clarified: makeReport("semantically clarified but not fully proven"), | ||
| blocked: makeReport("blocked by setup/state"), | ||
| }).summary, | ||
| ).toBe("blocked by setup/state"); | ||
|
|
||
| expect( | ||
| buildVerifyReportOutput({ | ||
| proven: makeReport("proven working"), | ||
| deeper: makeReport("deeper issue remains"), | ||
| blocked: makeReport("blocked by setup/state"), | ||
| }).summary, | ||
| ).toBe("deeper issues remain"); | ||
| }); |
There was a problem hiding this comment.
While this test covers several important cases for summary preference, it could be more comprehensive and maintainable. Specifically, it's missing a test case where proven working is the expected summary, and it doesn't explicitly test the precedence of semantically clarified... over proven working as described in the PR description.
I suggest refactoring this into a data-driven test using vitest's it.each. This makes the different scenarios clearer, improves readability, and ensures all levels of severity precedence are explicitly tested.
it.each([
[
"only proven reports are present",
{ proven: makeReport("proven working") },
"proven working",
],
[
"clarified reports are present with proven reports",
{
proven: makeReport("proven working"),
clarified: makeReport("semantically clarified but not fully proven"),
},
"semantically clarified but not fully proven",
],
[
"blocked reports are present with clarified reports",
{
clarified: makeReport("semantically clarified but not fully proven"),
blocked: makeReport("blocked by setup/state"),
},
"blocked by setup/state",
],
[
"deeper issue reports are present with blocked reports",
{
deeper: makeReport("deeper issue remains"),
blocked: makeReport("blocked by setup/state"),
},
"deeper issues remain",
],
])("prefers the highest-severity summary branch when %s", (_caseName, reports, expectedSummary) => {
expect(buildVerifyReportOutput(reports).summary).toBe(expectedSummary);
});
Issue
The latest verifier follow-up changed the repository's declared Node engine range from
>=20 <26to>=20 <27so the automation host's Node 26 runtime is treated as supported instead of an environment mismatch. That compatibility change had no regression test around it, which meant a future edit could silently narrow the range again.User impact
If the engine range regressed, installs and automation runs on Node 26 would fall back to warning or failure behavior even though the repo has already been verified against that runtime. The risk is narrow, but it affects the exact environment the verification tooling is now expected to support.
Root cause
The existing config/package tests asserted the coverage runner wiring, but they did not validate the semantics of the
package.jsonengine range itself. Recent changes updated the manifest without adding a test for the newly admitted major-version path.Fix
This PR adds a focused regression test in
scripts/vitest-config.test.tsthat parses the declared engine constraints and proves two things:Validation
I ran the targeted test file covering the package/config assertions:
pnpm vitest run scripts/vitest-config.test.ts --maxWorkers 1That run passed with all 3 tests green.