Sitelet https://github.com/lightspeedwp/.github/issues/3572
Skip to content

test: metrics-collection-orchestrator duration assertion flakes under full-suite load #3572

Description

@eleshar

Problem

scripts/workflows/__tests__/metrics-collection-orchestrator.test.js asserts a wall-clock duration against a fixed floor:

const summary = orchestrator.generateSummary();

expect(summary.execution.duration).toBeGreaterThanOrEqual(100);   // line 321
expect(summary.execution.duration).toBeGreaterThan(0);

Under full-suite load the measured duration lands at 99 ms and the suite fails. It is a real assertion against a real threshold — the measured value is genuinely below the bound — but the bound is only met when the machine is not busy, so the test's outcome depends on CPU contention rather than on the code.

Evidence

Observed 2026-09-25 while validating #3571 on develop (25179a56):

Run Result
Full suite on clean develop 280 suites passed
Full suite on fix/inert-workflow-harness-3570 (5 runs) 1 failed, 4 passed
Additional full-suite runs on the same commit 1 failed, then passed
This suite in isolation (3 runs) 14 passed, 3/3

Failure text is always the same shape:

● MetricsCollectionOrchestrator › should track collection duration
  expect(received).toBeGreaterThanOrEqual(expected)
  Expected: >= 100
  Received:    99

The suite imports only ../metrics-collection-orchestrator.cjs and no workflow, composite or reachability code, so it is independent of #3571. It passes every time it is run alone and fails only when 281 suites compete for the runner.

Why this matters now

#3479 / #3487 make the Jest suite a required check that fails on any failure on the PR head. A test whose outcome depends on machine load will therefore block pull requests at random, on PRs that changed nothing here. It is the same class of defect as the flaky timer lower bound removed in #3487 (a0f623e14c), in a different file.

It is also the kind of failure that trains reviewers to re-run red checks instead of reading them.

Change

Pick one, and prefer whichever matches how generateSummary() actually derives the value:

  • Derive the expectation instead of hard-coding it. If duration is computed from timestamps captured in the test, assert the relationship (e.g. it is at least the mocked interval, or it is within a tolerance of a value the test controls) rather than a literal 100.
  • Or widen to a tolerance that cannot be missed under load, if the exact value genuinely is not important — with a comment saying so. A wall-clock assertion is only meaningful when the bound is far enough from the observed value that contention cannot reach it; a 1 ms margin is not.

Whichever is chosen, do not simply delete the assertion: duration being greater than zero is the actual behaviour worth protecting, and that half already passes.

Acceptance

  • The suite passes reliably in a full-suite run — at least 10 consecutive npx jest --config .jest.config.cjs runs with no failure
  • The assertion still fails if the duration is genuinely wrong (verify by fault injection, not by deleting the test)
  • A comment records why the bound is set where it is, so it is not tightened back into flakiness

Refs #3479, #3487. Found while validating #3571; not fixed there because it is unrelated to that change.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions