test: organize coverage by responsibility and add invariants - #174
Conversation
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
🟡 Changes recommended
Critical CI-formatting findings and moderate test-coverage gaps remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
test/rouge-s.test.ts:138
- This performance test never exercises finite-window matching:
maxSkip: 1returns through the identical-summary fast path, whilemaxSkip: 0returns before counting pairs. Use distinct long candidate/reference summaries (with a known partial match) so a regression in the finite-window implementation cannot pass without doing the intended work.
test('should score finite windows without scanning every distinct token pair', () => {
expectBundledScriptToPass(
`
const summary = Array.from({ length: 30000 }, (_, index) => \`token\${index}\`).join(' ');
if (module.exports.s(summary, summary, { maxSkip: 0 }) !== 0) {
throw new Error('zero-window score changed');
}
if (module.exports.s(summary, summary, { maxSkip: 1 }) !== 1) {
throw new Error('finite-window score changed');
test/rouge-s.test.ts:13
- The third argument is optional on
s, so this explicitanycast is unnecessary and conflicts with the repository's no-explicit-anyconvention inAGENTS.md:67. Omitting the argument exercises the same undefined-options path.
expect(() => s('', ref, undefined as any)).toThrow(RangeError);
test/rouge-s.test.ts:17
- The third argument is optional on
s, so this explicitanycast is unnecessary and conflicts with the repository's no-explicit-anyconvention inAGENTS.md:67. Omitting the argument exercises the same undefined-options path.
expect(() => s(cands[0], '', undefined as any)).toThrow(RangeError);
test/segmentation.test.ts:114
- Both tests in this
describeblock use the same title, so Jest reports cannot distinguish the list-marker-only case from this later case that also checksvs.. Rename this title to describe the additional abbreviation assertion.
test('recognizes list markers without depending on item capitalization', () => {
- Files reviewed: 21/23 changed files
- Comments generated: 3
- Review effort level: Lite
|
Critical reconciliation of the four suppressed findings on prior head127ba: the finite-window performance coverage gap is confirmed and fixed independently by #181, now retained here byte-for-byte. The two unnecessary undefined-as-any findings are addressed by omitting all three redundant third arguments in the ROUGE-S cases, preserving the optional-argument default. The duplicate segmentation title is clarified for its sentence-final-abbreviation assertion. These changes are included in f5c7d51 and the complete 3,040-test validation. The two generated formatting claims were disproven by template AST inspection and four-lane strict CI; their embedded programs are unchanged. The generated whitespace-invariant finding is fixed and its thread resolved. |
|
@codex review Please review code and security on final integrated head f5c7d51, tree be603143d9397c37543f01bfb0d8965118563bdf, against current main c739e91. The PR reorganizes the fully integrated regression suite into 18 executable suites, shared helpers and Golden Rule fixtures, preserving all existing test behavior and production bytes. This final head also addresses the actual prior review: strengthen the original whitespace/content invariant, remove three unnecessary explicit undefined arguments and clarify one duplicate test title. The inherited finite-window performance coverage gap was addressed independently in merged #181, whose exact new regression is retained in the split ROUGE-S suite. The two prior formatting claims referred to code inside template strings and were disproven by AST inspection plus actual strict CI; those threads were replied to and resolved. All 3,040 tests and seven local checks pass with 98.22% branch coverage. Independent exact-byte and AST checks preserve the full prior test inventory, new performance fixture, enclosing scope and bundled helper. All 58 validated file hashes survive normal commits. The final merge of actual main changes ancestry only; no duplicate runtime run is claimed. Please inspect discovery, moved scope, fixtures, invariant strength and configuration. Current-head declared-toolchain CI runs separately; the preserved local package installation denial was not retried or bypassed. |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved moderate findings remain in bundle reuse and the performance-test timeout, along with two unnecessary casts.
Review details
Suppressed comments (4)
test/helpers.ts:63
- This helper rebuilds the same
src/rouge.tsbundle on every invocation. It is called many times across the suites, including repeatedly in the performance tests, so the test run repeats identical esbuild work unnecessarily; build the bundle once at module scope and reuse the resulting text.
const bundled = buildSync({
entryPoints: [join(__dirname, '../src/rouge.ts')],
bundle: true,
platform: 'node',
target: 'node18',
test/rouge-l.test.ts:50
- This repeats the unnecessary
undefined as anycast, which violates the repository'snoExplicitAnyrule;laccepts an omitted optional options argument. Remove the third argument here as well.
expect(() => l(cands[0], '', undefined as any)).toThrow(RangeError);
test/rouge-l.test.ts:46
- This explicit
anytriggers the repository'snoExplicitAnywarning under the strict Biome configuration, and the cast is unnecessary becauseoptsis already optional onl. Call the function with two arguments instead.
expect(() => l('', ref, undefined as any)).toThrow(RangeError);
test/segmentation-performance.test.ts:90
- The child process is allowed 15 seconds, but this Jest test has no per-test timeout and the config leaves Jest's default at 5 seconds. Under normal CI load Jest can terminate the test before the intended child-process guard; give the test an explicit timeout longer than 15 seconds, as the neighboring constrained-heap tests do.
});
- Files reviewed: 21/23 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Critical reconciliation of the four suppressed Copilot findings on f5c7d51: both redundant ROUGE-L optional arguments are removed in 5ae7aa6. Their claimed Biome-rule violation was incorrect (the test override disables noExplicitAny), but the cleanup is useful and both focused cases pass. The suggested timeout fix is unnecessary for this synchronous spawnSync callback: Jest cannot run its timer until the callback yields; the existing15-second child timeout remains the enforcing bound. Independent Jest-source review and a six-second blocking-callback probe with a five-second Jest timeout confirm that behavior. Bundle compilation is unchanged from the prior helper; caching is optional optimization, and the proposed eager module-scope build would also run in helper-importing suites that never bundle. No introduced correctness defect or relation to the child timeout was demonstrated, so helper lifetime stays unchanged. Separately, the real CI resource contention concern is addressed by a one-worker default, preserving all original timeout/heap limits; the full normal command passed3040 tests before the two-call cleanup. |
|
@codex review Please review code and security on final head 5ae7aa6, tree bb8302842fbefb91f3b1aafd14b65708964ec735, against current main c739e91. This PR splits the integrated regression inventory into 18 suites, shared helpers and Golden Rule fixtures, preserving production bytes and all prior tests. It includes the strengthened whitespace invariant, optional-argument cleanup and duplicate-title correction from actual review; #181's separately merged finite-window test is retained exactly. The previous head f5c7d51 failed one Node18 CI test: the unchanged seven-million-character/64MiB quotation case exceeded its20-second child limit. Prior split-suite Node18/22 passes were already19.2/19.8seconds. This additive change sets Jest maxWorkers to1 so resource-limit suites do not compete. All test inputs, outputs,20-second child/25-second Jest limits, and production files remain unchanged. Independent review supports contention as an explanation; the final declared-toolchain CI run must verify it. The normal npm test command passed all3,040 tests in18suites without any worker CLI override. The only subsequent change removes two redundant optional arguments in ROUGE-L tests; both affected cases, test types and strict lint pass. Effective Jest configuration and repository strict lint also pass. Existing types/build/consumer validation is reused for byte-identical inputs. All58 files are bound through normal commits to this explicit validation chain; no full-suite rerun after the two-call cleanup is claimed. The latest four suppressed Copilot findings are critically reconciled: both optional-argument casts are removed; the synchronous callback cannot be preempted by the Jest timer, so its existing child timeout remains effective; the helper caching proposal is an inherited optional optimization, not a demonstrated introduced defect. Its proposed eager cache would also build in suites that import other helpers but never use bundling. Independent source review and a bounded Jest probe support these dispositions. Please review discovery, fixture/scope retention, the worker setting and security. No blocked dependency installation was retried or bypassed. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Split the integrated regression suite into 18 executable test suites, shared helpers and Golden Rule fixtures. Preserve the existing regression inventory, declaration scope, fixture values, helper behavior and all production bytes. Jest discovers only executable suites.
The final integration includes the independently merged #181 finite-window regression. Review follow-up strengthens the original whitespace/content invariant, removes redundant optional arguments and clarifies one duplicate test title. The original seven expanded invariant cases remain included.
Validation: all 3,040 tests, source/test types, strict installed lint, Markdown, build and packed consumers pass. Independent AST and exact-byte checks verify complete test retention and unchanged production modules. The final main merge preserves the validated tree. Exact current-head CI validates the declared toolchain; the previously recorded local package-install denial was not retried or bypassed. Codex code/security and Copilot reviews are requested on the final published head.
Run suites with one Jest worker so resource-limit regressions do not compete across suites. This addresses a Node18 CI timeout of the unchanged seven-million-character quotation case; its input, expected output,20-second child timeout,25-second Jest limit and64MiB heap limit are preserved. The normal npm test command passes all3,040 tests with the configured worker count.
Final review cleanup removes two redundant ROUGE-L optional arguments. Both focused cases, test types and strict lint pass after that edit; the complete3,040-test normal-command result precedes only this two-call cleanup, with the remaining files unchanged. Fresh final-head CI validates the complete combination.