PDF export change badges have no test coverage for zero-value vs. no-prior-data cases
- Dominant language
- JavaScript
- Stars
- 1.4k
- Forks
- 383
- Avg merge
- 4d 12h
- Merged PRs (30d)
- 80
Description
## Bug Description
Split from #12867 ([ITEM 2](https://github.com/google/site-kit-wp/issues/12867#issuecomment-5286223042)). While fixing the email report decimal-comma badge bug, QA asked whether the PDF export's `0%` change badge can be exercised in a real report, since it can't be reproduced with live or fixture data (it needs the current and previous period to have the exact same non-zero value).
By design, `calculateChange()` (`assets/js/util/index.js`) and `getPDFTileChange()` (`assets/js/components/pdf-export/getPDFTileChange.ts`, which wraps it for the PDF change badge) already distinguish a genuine zero change from a missing comparison:
- `calculateChange( 0, 0 )` → `0` (both periods are zero, a real `0%` change).
- `calculateChange( 0, 100 )` → `null` (previous is zero, divide-by-zero guard) → `getPDFTileChange` returns `{}`, hiding the badge.
- `calculateChange( 100, 100 )` → `0` (real `0%` change).
None of these three cases are covered by an automated test today, so this behavior — including the case that's hardest to reproduce manually — has no regression protection.
**Open question raised in review** ([Slack thread](https://fueled.slack.com/archives/C0788NZMLF2/p1786639277978809)): is treating `previous === 0` as "no prior data" actually correct? Kelvin pointed out that a previous value of `0` is usually genuine zero-traffic data, not necessarily evidence the property/metric didn't exist yet in the prior period — "no prior data" to him means the property wasn't created yet, which `calculateChange()` has no way to distinguish from real zero traffic. This should be confirmed before the tests are written, since the expected results below encode the current (possibly incomplete) interpretation.
## Additional Context
- Discovered while working on #12867 (PR #13265 / #13353).
- Not a regression and not required by #12867's acceptance criteria — filed separately since it's PDF export code, not the email report fix.
---------------
_Do not alter or remove anything below. The following sections will be managed by moderators only._
## Acceptance criteria
* It's confirmed (or reconsidered) whether `previous === 0` should always be treated as "no prior data" for the PDF change badge, per the open question raised in the linked Slack thread.
* `calculateChange( 0, 0 )` is covered by a test asserting it returns a real `0` (not `null`).
* `calculateChange( 0, 100 )` is covered by a test asserting it returns `null`.
* `calculateChange( 100, 100 )` is covered by a test asserting it returns a real `0`.
* The same three cases are covered at the `getPDFTileChange()` level, asserting a real `0%`/`noChange` badge for the first and third, and no badge (`{}`) for the second.
## Implementation Brief (Proposed while creating issue, needs to be confirmed/verified)
### Files to modify
* [ ] Update file `assets/js/util/test/calculateChange.js`: add cases for `( 0, 0 )`, `( 0, 100 )`, and `( 100, 100 )` to the existing `valuesToTest`/`it.each` table.
* [ ] Add file `assets/js/components/pdf-export/getPDFTileChange.test.ts`: new test file covering the same three cases at the `getPDFTileChange()` level (no existing test file covers this module).
### Test Coverage
* `assets/js/util/test/calculateChange.js` — add `( 0, 0 ) → 0`, `( 0, 100 ) → null`, `( 100, 100 ) → 0`.
* `assets/js/components/pdf-export/getPDFTileChange.test.ts` — add `( 0, 0 ) → real 0% / 'noChange'`, `( 0, 100 ) → no badge`, `( 100, 100 ) → real 0% / 'noChange'`.
## QA Brief
## Changelog entry
Contributor guide
Research direction
First confirm the previous === 0 interpretation raised in the issue and Slack thread. Then read assets/js/util/test/calculateChange.js and assets/js/components/pdf-export/getPDFTileChange.ts, adding the specified cases in the existing table and a new focused test file. Run both focused test suites; done means all three calculation and PDF badge outcomes are covered with the agreed expectations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript, typescript
- Domain
- frontend, testing
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 55/100