Repository navigation
[Tests] Cover the app security output format helpers - #8905
Draft
github-actions[bot] wants to merge 1 commit into
Draft
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
countLabel, checksWithFindingsLabel and checkStatusLabels had no co-located tests. They were only exercised through the review and record presenters, which never reach the zero-count or missing-status branches. Co-Authored-By: Claude Opus 4.8 <[email protected]>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
packages/app/src/cli/services/app-security-format.tsholds the wording shared by theapp securitycommands' terminal output —countLabel,checksWithFindingsLabelandcheckStatusLabels— and had no co-located test.Its only coverage was incidental, through the presenters that consume it:
security-review-output.test.tsasserts whole headlines like3 checks with findings. 1 check unresolved.andsecurity-record.test.tsassertsRecorded 1 check and 2 findings in. Those presenters never construct a summary that reaches the helpers' boundaries, so several branches were unexercised:countLabel(0, noun)— zero pluralizes like a plural, which no presenter assertion covers.checkStatusLabelsreturning[]when every check has findings, which is what tells the caller to render no status line at all.passed,not applicable,unresolved, and the omission of a status whose count is zero.Because the assertions lived in the presenters, a change to the helpers would surface as a failure in an unrelated output test rather than at the helper itself.
I reviewed the Main tests runs created between 2026-10-08 and 2026-10-15 UTC (46 runs, no retried attempts) first. One run failed: run 38383005193,
[Main] Node 24.1.0 in windows-latest, where the PowerShell record test inapp-security-commands.test.tstimed out at 13000ms. A fix for exactly that test is already open in #8832, so there was no actionable, non-duplicate flake left and I took a coverage gap instead.WHAT is this pull request doing?
Adds
packages/app/src/cli/services/app-security-format.test.ts, covering the three exported helpers directly: pluralization including the zero case, thatchecksWithFindingsLabelreads onlywithFindingsand ignores the other counts, and thatcheckStatusLabelskeeps its display order, drops zero-count statuses, and is empty when every check has findings. The summary fixture is a plain local factory overCombinedChecksSummary, so each test states only the counts it cares about and shares no mutable state. No production code changes.Validation on Linux Node 22.20.1: the new file passes (6 tests), passes three times under
--sequence.shuffle, and passes alongside both consumers,security-review-output.test.tsandsecurity-record.test.ts(89 tests total).nx run app:type-check, ESLint on the new file, andknipall pass.I sanity-checked that the tests earn their keep by temporarily mutating the source two ways: changing
total === 1tototal <= 1, and swapping thenot applicableandunresolvedentries. Four of the six tests fail under those mutations, and the source was restored before committing — the diff is the new test file only.Remaining uncertainty: these are pure string helpers with no I/O, clock or platform dependency, so there is nothing platform-specific to confirm beyond the Linux run. The change adds no coverage for the presenters themselves, which keep their existing assertions.
How to manually test your changes?
Checklist
patchfor bug fixes ·minorfor new features ·majorfor breaking changes) and added a changeset withpnpm changeset add