google / google/site-kit-wp

PDF export change badges have no test coverage for zero-value vs. no-prior-data cases

Open
#13,397 0 comments 0 reactions 0 assignees View on GitHub
P2 Type: Enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.