test: cover truncated usage output - #1261
PandaHUN777 wants to merge 1 commit into
Conversation
|
Reviewed PR #1261 (test: cover truncated usage output, by PandaHUN777, closes #1259). I checked out the branch in a throwaway worktree and ran it. What's good
|
prx-my
left a comment
prx-my
left a comment
There was a problem hiding this comment.
Nice one I checked this by actually deleting each branch and confirming the matching test goes red, and all three do. Build and lint:tests are green on my side too.
One thing I'd like to see before this is airtight:
tests/html-reporter.test.ts:318 — this is only a negative assertion. It proves the sentence is absent when imported is null, but if the sentence generation in formatters.ts were removed entirely (or the === false guard broke so it never fires), this test would still pass. The other two tests avoid that by positively asserting "unknown". Could you add a companion case with usage: { imported: false, files: [] } and relationship: "transitive" asserting the sentence is present? Then the absence actually means something.
Two nits, no need to block on these:
tests/output.test.ts:1639,tests/output.test.ts:2254,tests/html-reporter.test.ts:306— the names say "when the usage scan is truncated", but the tests setimported: nulldirectly rather than simulating truncation. That's the right state to pin, it's just the name overstating the setup. Maybe name it after the rendered state, e.g. "renders unknown, not unused, when usage is null", or drop a one-line comment thatnullis the "we did not look" sentinel.src/usage/scanner.ts:15—usage-scan coverageis the only hyphenated "usage scan" in the codebase; everywhere else it's two words (Usage scan stopped...). Align it tousage scan coverage.
tests/usage.test.ts:12 and :20 look good as-is. Thanks for picking this up.
| expect(html).toContain("Risk summary"); | ||
| expect(html).toContain("Transitive issue."); | ||
| expect(html).not.toContain( | ||
| "No direct imports found in source code, so practical reachability may be low.", |
There was a problem hiding this comment.
his is only a negative assertion. If the sentence generation in formatters.ts were removed entirely (or its === false guard broke so it never fires), this test would still pass. Add a companion case with usage: { imported: false, files: [] } and relationship: "transitive" asserting the sentence is present, so the negative assertion actually means something.
There was a problem hiding this comment.
Confirmed, and this is the better mutation. Deleting the whole imported === false block from formatters.ts leaves all 2221 tests green, so the sentence can be removed silently and only a positive assertion would notice.
I had tested flipping the guard to !== true, which makes the sentence fire on a truncated finding, and that one is caught. Feature deletion is the case neither of us had covered until you raised it.
| expect(output).not.toContain("⚠ no fix"); | ||
| }); | ||
|
|
||
| it("renders unknown usage in the findings table when the usage scan is truncated", () => { |
There was a problem hiding this comment.
these don't simulate truncation; they set imported: null directly, which is the correct sentinel. Rename to the rendered state (e.g. "renders unknown, not unused, when usage is null") or add a one-line comment noting null = "we did not look".
| expect(outputDefault).toContain("Run with --verbose for fix plan"); | ||
| }); | ||
|
|
||
| it("prints unknown usage in compact output when the usage scan is truncated", () => { |
There was a problem hiding this comment.
again the same, it don't simulate truncation; they set imported: null directly, which is the correct sentinel. Rename to the rendered state (e.g. "renders unknown, not unused, when usage is null") or add a one-line comment noting null = "we did not look".
| * reported. Otherwise it only warns that coverage is incomplete. | ||
| */ | ||
| export function usageScanTruncationWarning(opts?: { subfolder?: string; onlyUsed?: boolean }): string { | ||
| const remedy = "The only way to improve usage-scan coverage is to scan a narrower path."; |
There was a problem hiding this comment.
elsewhere the message and codebase use "usage scan" (two words, e.g. Usage scan stopped...). Change usage-scan coverage to usage scan coverage.
There was a problem hiding this comment.
Retracted. Do not act on this review. I posted it without reading @prx-my's inline comments, one of which is correct and identifies a real gap. See the request-changes review below for what is actually needed.
This is a good first contribution, thank you @PandaHUN777.
Worth saying what it closes, because it is more than the title suggests. When #1202 landed I noted two follow-ups for ourselves: the truncation warning never named the one remedy a user actually has, which is scanning a narrower path, and the three rendering paths that turn the truncated state into a readable line had no tests at all, so deleting any of them left the suite green. This closes both.
The tests discriminate, which is the part that matters here. Rendering the truncated state as unused in the findings table, changing the compact label, and letting the low-reachability line leak onto a truncated finding each failed exactly one of your tests and nothing else. The reachability guard is the one I am most glad to see pinned: imported === false against imported !== true is easy to get wrong later, and the cost of getting it wrong is telling someone a package is probably unreachable when the truth is we never looked at the files that import it.
Merged with current main it is 2160 tests green and a clean build.
Approved. I merge these one at a time rather than in a batch, so it will land shortly.
sonukapoor
left a comment
sonukapoor
left a comment
There was a problem hiding this comment.
This supersedes my approval above, which I should not have posted. @prx-my had left four inline comments on this two days ago and I approved without reading them. His first one is correct and it is the one that matters.
His point on tests/html-reporter.test.ts is that the assertion is only negative, so it would also pass if the sentence generation were removed entirely rather than merely mis-guarded. That is right: deleting the whole imported === false block from formatters.ts leaves all 2221 tests green.
Worth being precise about what that does and does not mean for your PR. It is still a net improvement. Four mutations die that did not before, including the === false against !== true confusion that is the easy one to introduce later, and the remedy sentence you added to the truncation warning closes a gap we had written down for ourselves. The problem is narrower: one of your assertions guards less than its name suggests.
Three things, the first being the only one that changes behaviour of the test suite:
- Add the positive counterpart in the html test. A finding with
imported: falseshould contain the reachability sentence. With both assertions present, deleting the feature fails and mis-guarding it fails, which is what the negative assertion alone cannot do. - @prx-my's naming point on the two
output.test.tscases. They setimported: nulldirectly, which is the correct sentinel, so the names want to describe the rendered state rather than claiming they simulate truncation. - His wording point in
scanner.ts. The codebase and the message itself say "usage scan", so "usage-scan coverage" should match.
Thanks @prx-my. My own mutation pass covered the guard direction and not feature deletion, so this is a catch I did not have.
This branch has not been deployed
3 participants
Closes #1259. Adds regression coverage for unknown usage in table and compact output and for the HTML risk summary after scan truncation, and explains that scanning a narrower path is the only way to improve usage coverage. Validation: npm run lint:tests; npm run build; node dist/index.js advisories sync; npm test (155 suites, 2,150 tests); git diff --check.