Skip to content

Add web unit tests (Vitest) and fix threshold percentage formatting - #66

Open
BlancaMunizaga wants to merge 3 commits into
devfrom
BlancaMunizaga/explore-frontend-tests
Open

BlancaMunizaga wants to merge 3 commits into
devfrom
BlancaMunizaga/explore-frontend-tests

Conversation

@BlancaMunizaga

Copy link
Copy Markdown
Collaborator

Why

The web had no tests: its CI job only type-checked, linted and built. eslint.config.mjs even keeps three react-hooks rules at warn because fixing them meant refactoring "with no automated test coverage behind it". Exploring this surfaced two bugs in the threshold formatting.

What

Unit tests (pnpm test in apps/web; the root pnpm test runs pytest, then these): 106 tests.

  • Vitest + jsdom + Testing Library. Tests sit next to the code; helpers are in src/test/: the setup, typed factories, and renderHookWithData, which renders a hook under an injected DataContext with the real translations.
  • The setup mocks next/navigation and file-saver, fails any request a test didn't stub, and resets the runtime config after every test.
  • Covered:
    • the utils (formatting, thresholds, GeoJSON, upload validation);
    • the real upload templates in public/files/ (empty, filled, and a renamed header);
    • en/es translation parity;
    • the hooks that read DataContext.
  • Left for later changes: the context providers (DataProvider, AdminSessionProvider, ReportProvider), the PDF, and E2E.

Threshold formatting fix (src/utils/numbers.ts). The tests were written first and failed on the old code.

  • With a 1% threshold, values above it lost their decimal: 1.4% showed as "1%", 10.5% as "10%". They now show as many decimals as the threshold has, and at least one.
  • The "< X%" label is localized: "< 0,5%" in Spanish, not "< 0.5%".
  • This applies to the tables, the map and the Excel/GeoJSON downloads. The PDF and what counts as "deforestation-free" are unchanged.

CI

  • The web job runs the tests (after lint, before build).
  • The job is renamed "Tests, type-check, lint, build". It is a required check, so the dev and main rulesets now require the new name. The procedure is in docs/branch_protection.md, "Renaming a required check".

Check

  • pnpm test: 106 passed, including twice in random order (--sequence.shuffle)
  • Root pnpm test: 296 pytest, then 106 Vitest
  • tsc --noEmit and next build pass
  • Lint: 0 errors (the same 18 warnings as before)
  • openspec validate add-frontend-unit-tests --strict passes

OpenSpec change: openspec/changes/add-frontend-unit-tests/.

The web had no tests: its CI job only type-checked, linted and built. This
adds a first layer (106 tests) and fixes two bugs the exploration surfaced.

Tests (apps/web, `pnpm test`; the root `pnpm test` runs pytest, then these)
- Vitest + jsdom + Testing Library; tests next to the code, helpers in
  src/test/ (setup, typed factories, renderHookWithData with an injected
  DataContext and the real translations)
- The setup mocks next/navigation and file-saver, fails any request a test
  didn't stub, and resets the runtime config after every test
- Covers the utils (formatting, thresholds, GeoJSON, upload validation),
  the real upload templates in public/files/, en/es translation parity and
  the hooks that read DataContext
- Context providers, the PDF and E2E are left for later changes

Threshold formatting (src/utils/numbers.ts)
- With a 1% threshold, values above it lost their decimal (1.4% -> "1%").
  They now show the threshold's decimals, at least one
- The "< X%" label is localized ("< 0,5%" in Spanish)

CI
- The web job runs the tests and is renamed "Tests, type-check, lint,
  build". It is a required check: the dev and main rulesets must switch to
  the new name in step with the merges (docs/branch_protection.md,
  "Renaming a required check")

OpenSpec change: add-frontend-unit-tests
Moves the change to openspec/changes/archive/2026-10-08-add-frontend-unit-tests
and applies its deltas: the new frontend-unit-tests spec, the renamed frontend
check in continuous-integration and api-contracts, and how threshold-dependent
percentages are displayed in product-configuration.

@BlancaMunizaga BlancaMunizaga left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of findings

  • 🟠 High: 1
  • 🟡 Medium: 1

Prioritized findings

  • 🟠 Required check on main was switched before its workflow (docs/branch_protection.md:39).
  • 🟡 Six-digit cap can turn a valid positive threshold or result into 0% (apps/web/src/utils/numbers.ts:18).

Positive feedback

The new tests exercise the real upload templates and both translation sets, and the threshold regression cases use explicit expected output. Both CI jobs passed on the reviewed head commit.

Comment thread docs/branch_protection.md
[How to apply it](#how-to-apply-it)), and the PR is merged at once.
Other open PRs into `dev` then wait for the new name: update them from `dev` so
their CI runs the renamed job.
3. **`main`, at the next release.** `main` keeps the old workflow until the release

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 High · Operations: The live main protection ruleset already requires Tests, type-check, lint, build, but main still runs .github/workflows/frontend.yml with the old Type-check, lint, build job name. A hotfix PR into main therefore cannot satisfy the required web check before this release reaches main, contrary to the rollout sequence documented here. Please restore the old required check in the main ruleset now, then switch it when the release PR carrying the renamed workflow is ready.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already done, so no change in this PR.

The premise was true when the review was posted (18:56 UTC): the main protection ruleset (id 23937824) required Tests, type-check, lint, build. It was switched back to the old name at 19:00 UTC the same day (ruleset history, version 52473138), and that is its current state:

$ gh api repos/undp/monbo/rules/branches/main --jq '.[] | (.parameters.required_status_checks[]?.context)'
Test and static checks
Type-check, lint, build

So a hotfix into main reports exactly the check its ruleset requires, and dev protection requires the new name, which is what this PR reports. The sequence written here (step 3: switch main at the release that carries the rename) is the one in force, so the doc text stands. If you'd rather the doc also record that the main switch is still pending as of this PR, say so and I'll add a line.

Comment thread apps/web/src/utils/numbers.ts Outdated
const DEFAULT_DECIMAL_PLACES = 1;
const DEFAULT_DISPLAY_THRESHOLD = Math.pow(10, -DEFAULT_DECIMAL_PLACES);

const MAX_THRESHOLD_DECIMAL_PLACES = 6;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Medium · Correctness: The API accepts any percentage from 0 to 100, including 0.0000001. Capping the displayed precision at six makes that valid threshold format as < 0% in both locales; a value just above it can likewise format as 0%. These labels also enter the Excel/GeoJSON exports, so they misstate a nonzero result. Please either enforce a six-decimal limit when parsing the API setting or preserve enough precision to display every accepted positive threshold and above-threshold value.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 72d3858.

The cap is now 20 decimals, the fraction-digit limit Intl.NumberFormat supports in every engine (older ones throw above it). A 0.0000001 threshold now reads < 0,0000001% / < 0.0000001%, and a value just above it 0.0000002%, in the tables, the map and the Excel/GeoJSON exports alike. The new test covers both locales; the suite, tsc and lint pass.

This is the second of the two options you proposed, chosen because it keeps the change inside the web app. It is a display limit, not a guarantee: a threshold below 1e-20% would still read < 0%. If we want a hard guarantee, the place is _percentage in apps/api/app/config/env.py, rejecting more than 20 decimals, and I'd do that in a separate API PR with its own pytest. Say if you'd rather have it in this one.

The six-decimal cap introduced with the threshold formatting made a valid
0.0000001% threshold read "< 0%", and a value just above it "0%", in the
tables, the map and the Excel/GeoJSON exports. Raise the cap to 20, the
fraction-digit limit Intl.NumberFormat supports in every engine, and cover
the tiny-threshold case in both locales.
@BlancaMunizaga

Copy link
Copy Markdown
Collaborator Author

Review comment triage

Fixed

  • apps/web/src/utils/numbers.ts:18 (six-decimal cap turned a valid tiny threshold into < 0%): fixed in 72d3858, cap raised to 20 with tests in both locales.

Discarded

  • docs/branch_protection.md:39 (main's required check switched before its workflow): already done, the main protection ruleset was put back on Type-check, lint, build at 19:00 UTC, four minutes after the review, and the documented sequence is the one in force.

Proposed follow-up

  • API-side validation of the thresholds' decimals in _percentage (apps/api/app/config/env.py), if a hard guarantee is wanted rather than a display limit.

Nothing left open pending clarification. Threads left unresolved for the reviewer.

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.

1 participant