Skip to content

[Tests] Cover the app security output format helpers - #8905

Draft
github-actions[bot] wants to merge 1 commit into
mainfrom
tests-maintenance-38659679433
Draft

github-actions[bot] wants to merge 1 commit into
mainfrom
tests-maintenance-38659679433

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

WHY are these changes introduced?

packages/app/src/cli/services/app-security-format.ts holds the wording shared by the app security commands' terminal output — countLabel, checksWithFindingsLabel and checkStatusLabels — and had no co-located test.

Its only coverage was incidental, through the presenters that consume it: security-review-output.test.ts asserts whole headlines like 3 checks with findings. 1 check unresolved. and security-record.test.ts asserts Recorded 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.
  • checkStatusLabels returning [] when every check has findings, which is what tells the caller to render no status line at all.
  • The display order of 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 in app-security-commands.test.ts timed 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, that checksWithFindingsLabel reads only withFindings and ignores the other counts, and that checkStatusLabels keeps its display order, drops zero-count statuses, and is empty when every check has findings. The summary fixture is a plain local factory over CombinedChecksSummary, 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.ts and security-record.test.ts (89 tests total). nx run app:type-check, ESLint on the new file, and knip all pass.

I sanity-checked that the tests earn their keep by temporarily mutating the source two ways: changing total === 1 to total <= 1, and swapping the not applicable and unresolved entries. 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?

shopify app security review
shopify app security record --help

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've considered analytics changes to measure impact
  • The change is user-facing — I've identified the correct bump type (patch for bug fixes · minor for new features · major for breaking changes) and added a changeset with pnpm changeset add

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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants