Repository navigation
test(coverage): fix what the deletion pins claimed - #3501
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Size Report
Startup median (7 runs, lower is better):
|
|
This PR is ready for human review at 6bf8ab9. The code change is correct and I found nothing that needs fixing. Not blocking: the sentence at https://github.com/callstack/agent-device/blob/6bf8ab9/CONTRIBUTING.md#L258 covers deletion-only edits, but an edit that also adds lines inside the surviving function is charged its post-deletion score. "Only lines you add are charged" implies this, so adding "a deletion-only edit" to the sentence is optional, and you can take it or leave it. On the open threads from other reviewers, two do not apply, so please resolve them. The test at scripts/coverage-crap/run.test.ts:346 pins the case where the head file no longer contains the function, and the surviving-function tests near lines 309 and 328 keep the --all score assertion, so the stale-map case stays covered: #3501 (comment). The wording at CONTRIBUTING.md:258 is about deletion-only edits and follows "Only lines you add are charged", so a mixed edit is charged normally: #3501 (comment). The Coverage check fails in the eager-closure-budgets test "no APPROVED_OVER_CEILING row is stale". It reads the approvals against the merge-base, and no file in this diff is on that route, so it looks unrelated to this PR. The merge-base e36f117 is behind main, which fits stale approvals. I did not read the failing job log, and I did not run the coverage-crap tests or trace scripts/coverage-crap/run.ts to confirm how fallow indexes added lines. The two Smoke Tests jobs are still queued. No conflicts are known. Please rebase onto main to clear the stale approvals, then wait for the queued Smoke Tests. |
thymikee
added
the
ready-for-human
|
Why this PR's Coverage job is red, and what it needs. Not this diff — it is one test file and one docs paragraph. It fails
#3502 deletes those three rows (-34 lines, no logic). Files are disjoint and the two merge clean. Merge #3502 first, then rebase this onto |
Two review threads survived #3486 and #3492 because both were merged while the checks were still queued. The second deletion case was named for a function whose own lines did not change, and its own fixture deletes a line inside that function. Renamed to the scenario it covers: additions below the span, alongside a removal inside it. The third case pinned its fixture rather than any scoping rule, since a function absent from the map is unscored in either scope. Measured that first: a stale map which still records the deleted function yields nothing from `--all` either, so no scoping keeps it uncharged — the removal takes the score with it. The test now asserts that in both scopes and says which of the two mechanisms it is evidence for. CONTRIBUTING said removing code retires debt. It does not always: dropping lines that used to be covered lowers the coverage side of the ratio and can raise the score, and that debt lands in the `--all` backlog rather than the PR. The sentence now says deletions are uncharged, which is the accurate claim, and names where the debt it can create goes. Part of #3414.
Both review points on the previous commit were right. The third case's `--all` assertion proved nothing about a stale map, because its fixture recorded no functions at all — there was no deleted-function score for `--all` to omit. It now keeps the `score` record at CRAP 42 while the source loses the function, which is the shape a forgotten `pnpm test:coverage` really leaves behind, and asserts `--all` omits it anyway: scoring walks the source, so neither scope reports a function that is no longer there. Measured through run() before being written. CONTRIBUTING said a deletion's raised score reaches `--all` rather than the PR, which holds only when no added line lands inside that function — a charged function is scored as it stands after the deletions, so the rise blocks with it. Said conditionally now. The rewrap also brought two over-long lines this paragraph had inherited back under the house width.
6bf8ab9 to
42b2403
Compare
#3483 landed the three test-kit entries these rows approved, so from the next branch on the merge-base carries them and the no-growth rule governs: nothing reads a row any more, which is the invariant's own definition of a stale one and the failure message says to remove them. The trap is that `main` cannot show it. `staleApprovalRows` defers when the merge-base is the head, because otherwise every approval would guarantee a red `main` one commit after it landed (#2329). So the merged PR is green, `main` is green, and the first branch rebased onto post-#3483 `main` inherits a failing blocking Coverage job for a row it did not write. Reproduced on this branch's own head: three rows named, pristine `main` passes. Only first-introduced entries consult a ceiling, so deleting a dead row cannot loosen anything: an entry the merge-base carries is policed by the no-growth rule, and an entry nobody has carried yet is still measured against `NEW_ENTRY_CEILINGS`. Unblocks #3501 and every other branch based on current `main`.
|
#3501 rewrote the deleted-function case to keep the `score` record while the source loses the function, which was the assertion that could actually fail. That left `noFunctions` declared and branched on with no caller, so the empty-fnMap path it exists to build is built by nobody and checked by nothing — the same shape as the test-only seam removed earlier, with the fixture standing in for a tool. Nothing else reads the knob, and 59 tests still pass, so the record builder is now the one shape every case uses.
The paragraph said a deletion's raised score waits in the `--all` backlog, and read the way the reviewer did: only added lines are charged, so a deletion is never. That holds for a deletion-only edit and for a mixed one whose added lines stay outside the function -- `run.test.ts` pins exactly that mixed case, adding below `score` while deleting inside it, and the run is green. It does not hold for added lines inside the function, which is charged the post-deletion score. Names both halves so the conditional is visible rather than inferred from "Adding lines is what charges a function", per the reviewer's optional wording on #3501. Kept the mixed-edit clause because dropping it would overstate the exemption the same paragraph is there to bound.
#3518) The paragraph said a deletion's raised score waits in the `--all` backlog, and read the way the reviewer did: only added lines are charged, so a deletion is never. That holds for a deletion-only edit and for a mixed one whose added lines stay outside the function -- `run.test.ts` pins exactly that mixed case, adding below `score` while deleting inside it, and the run is green. It does not hold for added lines inside the function, which is charged the post-deletion score. Names both halves so the conditional is visible rather than inferred from "Adding lines is what charges a function", per the reviewer's optional wording on #3501. Kept the mixed-edit clause because dropping it would overstate the exemption the same paragraph is there to bound.
#3483 landed the three test-kit entries these rows approved, so from the next branch on the merge-base carries them and the no-growth rule governs: nothing reads a row any more, which is the invariant's own definition of a stale one and the failure message says to remove them. The trap is that `main` cannot show it. `staleApprovalRows` defers when the merge-base is the head, because otherwise every approval would guarantee a red `main` one commit after it landed (#2329). So the merged PR is green, `main` is green, and the first branch rebased onto post-#3483 `main` inherits a failing blocking Coverage job for a row it did not write. Reproduced on this branch's own head: three rows named, pristine `main` passes. Only first-introduced entries consult a ceiling, so deleting a dead row cannot loosen anything: an entry the merge-base carries is policed by the no-growth rule, and an entry nobody has carried yet is still measured against `NEW_ENTRY_CEILINGS`. Unblocks #3501 and every other branch based on current `main`. (cherry picked from commit ecf597b)
1 participant
Summary
Stacked on #3502: this branch's Coverage job can only go green once that PR retires the three dead
APPROVED_OVER_CEILINGrows it inherits from post-#3483main. Retarget tomainafter. Part of #3414.Two review threads survived #3486 and #3492, both merged while their checks were queued.
run.test.tsnamed a case for "a function whose own lines did not change" while its fixture deletes a line inside it. Renamed to what it covers: additions below the span, a removal inside. The third case pinned its fixture rather than a rule, so it now keepsscoreat CRAP 42 while the source loses the function — what a forgottenpnpm test:coverageleaves — and asserts--allomits it: scoring walks the source, so neither scope reports a function that is gone.CONTRIBUTING claimed removing code retires debt, and that a deletion's raised score reaches
--allrather than the PR. Neither holds as written: dropping covered lines can raise a score, and a function the PR adds lines inside is scored at that value and blocks. The paragraph now says deletions are uncharged, and makes the--allclaim conditional.Validation
42b240396: 59 tests pass, including the--allassertion, which fails if a deleted function ever stops taking its score with it;check:affected --base crap/3426-retire-approval-rows --rungreen under the gate lock.run()in both scopes first, not reasoned about.