Skip to content

ci: quality-gate floors — ESLint warning ceilings + coverage thresholds#2654

Merged
jordan-simonovski merged 5 commits into
mainfrom
jordansimonovski/eslint-warning-ceilings
Jul 17, 2026
Merged

ci: quality-gate floors — ESLint warning ceilings + coverage thresholds#2654
jordan-simonovski merged 5 commits into
mainfrom
jordansimonovski/eslint-warning-ceilings

Conversation

@jordan-simonovski

@jordan-simonovski jordan-simonovski commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Two ratchet-style quality gates, combined into one PR (they touch different files and share the "this metric can only improve" philosophy).

What changed

  • ESLint warning ceilings. Every package's lint script drops --quiet (which made ESLint 9 skip warn-level rules entirely) and adds a --max-warnings <N> ceiling pinned to that package's current count (app 740 / api 357 / common-utils 92 / cli 9 / hdx-eval 3). Warn rules run again and the count can't grow.
  • Coverage floors. common-utils and hdx-eval gain a coverageThreshold.global; app gains thresholds scoped to src/hooks/ and src/utils/ only. Each floor is pinned ~1 point below measured, so coverage can only ratchet up. hdx-eval's ci:unit gains --coverage so its floor is collected.

Key decisions

  • Both gates are pinned to current reality and lowered/raised only by a reviewed diff — decay is a build failure, improvement is deliberate.
  • App coverage is scoped to hooks and utils, not components. A global app floor would gate React components, where unit tests tend to be low-value and fragile (E2E and Storybook cover them better). Jest subtracts the globbed paths from the overall pool, so components carry no coverage requirement while the ratchet still holds on the genuinely unit-testable code. Hooks/utils measure 73–77% against a 53% global — the gap was almost entirely components.
  • packages/api coverage floor is deferred until the API unit/integration split (ci: faster test feedback — split API tests, parallelise common-utils, shard integration #2653) merges (no API ci:unit before then); packages/cli is skipped (single test file).

Impact

  • lint fails when a package exceeds its warning ceiling; the unit job fails when coverage drops below a floor.
  • Config/tooling only — no changeset.
  • Known follow-up: no collectCoverageFrom, so a brand-new fully-untested file doesn't lower a floor — worth a tracked issue (adding it re-baselines all thresholds).
Implementation detail

Ceilings and floors were both verified in both directions: they pass at current levels, and an injected regression (a @ts-ignore for lint; a temporary threshold bump above measured for coverage) fails as expected. The app's scoped floors were confirmed to enforce per glob — raising ./src/hooks/ to 99% fails with coverage threshold for statements (99%) not met: 73.72%, while components stay ungated.

@vercel

vercel Bot commented Jul 16, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Preview Jul 17, 2026 11:24am
hyperdx-storybook Ignored Ignored Preview Jul 17, 2026 11:24am

Request Review

@changeset-bot

changeset-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 48cab51

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

  • Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats
  • Production files changed: 8
  • Production lines changed: 60
  • Branch: jordansimonovski/eslint-warning-ceilings
  • Author: jordan-simonovski

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@github-actions github-actions Bot added the review/tier-3 Standard — full human review required label Jul 16, 2026
@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces two ratchet-style quality gates across all packages: ESLint warning ceilings (via --max-warnings <N>, replacing the previously silent --quiet) and Jest coverage floors (new coverageThreshold entries in jest configs). Both are pinned to current measured reality so they can only improve over time.

  • ESLint ceilings: --quiet removed, --max-warnings N added with per-package counts (app: 740, api: 357, common-utils: 92, cli: 9, hdx-eval: 3). Warn-level rules now actively run and are capped.
  • Coverage floors: common-utils and hdx-eval gain coverageThreshold.global; app gains path-scoped thresholds targeting only src/hooks/ and src/utils/; hdx-eval ci:unit gains --coverage so its floor is actually collected.

Confidence Score: 5/5

Config-only change that adds quality gates pinned to current measured state — safe to merge with no risk of breaking the build on merge.

All eight changed files are package.json scripts and jest config additions. The gates were verified in both directions (pass at current levels, fail on injected regressions) per the PR description. Tradeoffs like omitting collectCoverageFrom are documented and deferred deliberately.

No files require special attention. The app/jest.config.js path-scoped thresholds are the most novel part of the change, but they are well-commented and the scoping rationale is sound.

Important Files Changed

Filename Overview
packages/app/jest.config.js Adds path-scoped coverageThreshold for src/hooks/, src/utils/, and src/utils.ts with well-commented ratchet floors.
packages/hdx-eval/jest.config.js Adds global coverageThreshold (statements: 70, branches: 55, functions: 72, lines: 71).
packages/common-utils/jest.config.js Adds global coverageThreshold (statements: 86, branches: 78, functions: 85, lines: 86) — highest bar in the repo.
packages/hdx-eval/package.json Adds --coverage to ci:unit and sets --max-warnings 3 on lint.
packages/app/package.json Removes --quiet, adds --max-warnings 740 to lint.
packages/api/package.json Removes --quiet, adds --max-warnings 357 to lint.
packages/common-utils/package.json Removes --quiet, adds --max-warnings 92 to lint.
packages/cli/package.json Removes --quiet, adds --max-warnings 9.

Reviews (5): Last reviewed commit: "Merge branch 'main' into jordansimonovsk..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. This is a CI-config-only change; the gates are correctly wired (main.yml lint and unit jobs invoke yarn lint and jest --coverage, and nx.json does not cache these targets, so they run on every PR), and the eslint --max-warnings / jest path-resolution mechanics are sound. The findings below are about how much protection the new gates actually provide.

🟡 P2 — recommended

  • packages/app/jest.config.js:28 — No collectCoverageFrom in the app, common-utils, or hdx-eval jest configs, so Jest measures coverage only over files transitively imported by tests; a brand-new, fully-untested hook/util/module is absent from the denominator and passes every floor, so the ratchet guards regression in already-tested files only, not new untested code.
    • Fix: Add a collectCoverageFrom glob for each threshold scope (source globs, excluding __tests__ and .d.ts) so untested files count as 0% against the denominator, then re-baseline the floors.
    • testing, project-standards, correctness
🔵 P3 nitpicks (4)
  • packages/app/jest.config.js:41 — The ./src/utils/ threshold key resolves via path.resolve to the prefix src/utils and Jest matches files by string-prefix, so it also captures src/utils.ts and would silently pull in any future src/utils* sibling (e.g. src/utilsHelpers.ts) into that directory floor.
    • Fix: Use an explicit glob such as ./src/utils/** if independent scoping of utils.ts versus the utils/ directory is intended, or document the overlap.
    • correctness
  • packages/app/package.json:17 — The five --max-warnings ceilings (740/357/92/9/3) are bare numbers with no in-repo record of their ratchet-down-only intent (unlike the coverage floors, which carry explanatory comments), so a future contributor may raise a ceiling to absorb new warnings rather than fix them.
    • Fix: Record the ceilings and their intent in a contributing/tooling doc, or add an adjacent comment where the config format allows one.
    • maintainability
  • packages/api/package.json:95 — Coverage-floor coverage is inconsistent across packages (api and cli have none) with no in-repo note of why, so the gap reads as an oversight and risks silent coverage decay in those packages.
    • Fix: Add a one-line comment or tracking-issue link in the api/cli jest or package configs recording that a floor is intentionally deferred/skipped.
    • maintainability
  • packages/hdx-eval/jest.config.js:20 — Branch floors are low relative to the other metrics (hdx-eval branches 55, app hooks/utils branches 59), permitting meaningful sad-path/error-path decay before the build fails.
    • Fix: Confirm each branch floor is pinned just below current measured coverage and raise any that sit well under the real number so the ratchet has teeth.
    • testing

Reviewers (4): correctness, testing, maintainability, project-standards.

Testing gaps:

  • Run each package's ci:unit on a clean checkout to confirm every app path group (./src/hooks/, ./src/utils/, ./src/utils.ts) has ≥1 collected file — an empty group throws Jest: Coverage data for <path> was not found and fails CI — and that all floors are pinned just below (not above) measured reality.
  • Run each lint script on a clean tree to confirm the current warning count equals the pinned --max-warnings value, so the ceiling is a passing gate rather than an already-breached one.

@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 241 passed • 1 skipped • 836s

Status Count
✅ Passed 241
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 1

Tests ran across 4 shards in parallel.

View full report →

Pin Jest coverageThreshold in the unit configs for app, common-utils and
hdx-eval to just below each package's measured coverage (measured minus one,
rounded down). Coverage can now only ratchet upwards: any decay below these
floors fails the build, so the floors rise only when someone raises them
deliberately. Also add --coverage to hdx-eval's ci:unit so its threshold is
actually collected and enforced.

Integration configs are left untouched. The API unit config is deferred to a
follow-up until the API unit/int split (#2653) lands.
@jordan-simonovski jordan-simonovski changed the title ci(lint): enforce ESLint warning ceilings instead of suppressing them ci: quality-gate floors — ESLint warning ceilings + coverage thresholds Jul 16, 2026
@pulpdrew

Copy link
Copy Markdown
Contributor

My 2 cents: I don't love coverage requirements on React components. Unit tests on React components can be helpful, but often are not, with other methods (E2E, storybooks) providing more value. Hooks and utils are decent candidates for coverage requirements. Of course with agents we can always just generate tests to hit coverage requirements, but I suspect they'll often be relatively low value and fragile.

If the team thinks this is valuable though, happy to get over my hesitations.

I love the lint warning ceilings everywhere and the coverage requirements on common-utils in particular (everything there should be very unit-testable).

Replace the app package's single global coverageThreshold with path-scoped
floors on ./src/hooks/ and ./src/utils/ (plus the shared ./src/utils.ts).
React components now carry no coverage requirement -- unit tests on them tend
to be low-value and fragile, and E2E/Storybook cover them better -- while the
ratchet still holds on the genuinely unit-testable code.

Jest subtracts the globbed paths from the overall pool, so components are
ungated. Floors are pinned one point below measured (hooks/utils sit at
73-77% versus the old 53% global -- the gap was almost entirely components).
@jordan-simonovski

Copy link
Copy Markdown
Contributor Author

My 2 cents: I don't love coverage requirements on React components. Unit tests on React components can be helpful, but often are not, with other methods (E2E, storybooks) providing more value. Hooks and utils are decent candidates for coverage requirements. Of course with agents we can always just generate tests to hit coverage requirements, but I suspect they'll often be relatively low value and fragile.

If the team thinks this is valuable though, happy to get over my hesitations.

I love the lint warning ceilings everywhere and the coverage requirements on common-utils in particular (everything there should be very unit-testable).

@pulpdrew agreed. I've changed this to not take React components into consideration, but I've left in hooks and utils.

@pulpdrew pulpdrew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@jordan-simonovski
jordan-simonovski merged commit 68cf396 into main Jul 17, 2026
27 checks passed
alex-fedotyev added a commit that referenced this pull request Jul 19, 2026
The per-package eslint warning ceilings added in #2654 make any
net-new warning a lint failure. Merging main brought the ceilings
onto this branch, and the table color tests added 12 warnings that
pushed packages/app over its 740 cap.

Clear them at the source without touching any ceiling:
- replace empty-arrow mock callbacks with jest.fn()
- back the jsdom crypto.randomUUID shim with Object.defineProperty
  instead of an `as any` cast
- mock useSource via jest.mocked() to match the existing pattern in
  the same file
- scope one eslint-disable for the deliberately invalid palette token
  in the unknown-token fallback test

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-3 Standard — full human review required

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants