Skip to content

test(publish): mock the audio delay clock - #4715

Merged
kixelated merged 3 commits into
quest/m1/test-flakes-2/READMEfrom
quest/m1/test-flakes-2/publish-audio-clock
Oct 3, 2026
Merged

kixelated merged 3 commits into
quest/m1/test-flakes-2/READMEfrom
quest/m1/test-flakes-2/publish-audio-clock

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

The audio delay test subtracts 100ms from process uptime. Running it alone can produce negative media timestamps, aborting varint encoding and hanging the capture feed until the test times out.

Approach

Mock performance.now() at 200ms and restore it after encoder cleanup. Assert the exact encoded timestamps and 100ms catalog delay. Remove the completed quest and its parent reference.

Validation: reproduced the original negative timestamp failure; the isolated audio test file now passes all 11 tests. nix develop --command just check and quest check pass. The existing just ci test job runs this test.

Impact

  • Public API: none.
  • Wire format: none.

Alternatives

Avoid sleeps or uptime-dependent thresholds; control the clock used by the existing regression test.

Follow-ups

None.

(Written by GPT-6)

kixelated and others added 2 commits October 2, 2026 07:39
Co-Authored-By: GPT-6 <noreply@openai.com>
Co-Authored-By: GPT-6 <noreply@openai.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Implemented the quest and left this checked PR as a draft for the parent quest workflow. The isolated audio test reproduced a negative timestamp varint failure and test timeout before the fix; it now passes all 11 tests with mock time. Full nix develop --command just check and quest graph validation pass. Public API and wire impact: none. No outstanding design decisions or suggested follow-ups. Recommended next action: /quest-merge after review.

(Written by GPT-6)

@kixelated
kixelated marked this pull request as ready for review October 2, 2026 14:47

@kixelated kixelated 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.

Automated review by review (OpenAI)

Reviewed commit: 71d3f4e

No actionable introduced bugs found. In js/publish/src/audio/encoder.test.ts:308–333, fixing performance.now() at 200ms keeps the captured timestamps nonnegative and makes the expected 100ms delay deterministic. The two expected encoded timestamps match the fake encoder's two-frame lag, and the estimator retains the maximum delay even as subsequent timestamps advance. The scoped encoder is disposed before the clock is restored, including assertion failures.

Direction is appropriate: control the clock the test measures and assert exact output, with no production clock abstraction or timing tolerance needed. Removing the completed quest and its parent reference matches that scope. Public API and wire behavior are unchanged.

Verification: static review of the full diff, test fixture, encoder output path, and delay estimator. GitHub reports the exact-head Check workflow passed; I did not run the isolated test or local checks myself.

(Written by review (OpenAI))

@kixelated

Copy link
Copy Markdown
Collaborator Author

Outcome: the isolated negative-timestamp failure was reproduced and fixed by mocking the test clock, with exact timestamp and delay assertions. All 11 isolated tests and the full Nix check passed. No public API/wire changes or follow-ups. Under /recommended, proceed toward merging after CI and final-head review.

(Written by GPT-6)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Mocks performance.now() so the js/publish audio delay regression no longer depends on process uptime, then deletes the completed quest.

Findings

No blocking issues found.

The failure mode in the quest (#4414 / negative timestamps when performance.now()*1000 - 100_000 goes negative early in the process) is real, and the fix matches the production clock path:

  • Estimator.flush defaults now = performance.now() * 1000 (js/publish/src/jitter.ts).
  • Mocking performance.now → 200 yields start = 100_000 µs and catalog delay: 100 against a sibling baseline.observe(0, …) — consistent with delay = ceil((minimum - earliest) / 1000).
  • Exact written expectation [[100_000, 1], [120_000, 1]] matches the fake LaggingAudioEncoder (LAG=2) after four 20 ms frames.
  • try/finally + mockRestore keeps the spy active through using env disposal (encoder cleanup), then restores it.

Check/Test green; quest file and parent README entry removed cleanly.

Non-blocking

  • Same parent README base as chore: complete tracing capture audit #4713; expect a trivial child-list conflict when landing the pair.
  • Constant mockReturnValue(200) is enough here because flush lateness is computed from the mocked clock and the stamped media timestamps; no wall-clock advance is required for this assertion.

MERGE on 71d3f4e5c75dc7904d9d5a4fa87c565c29a99f20
This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated

Copy link
Copy Markdown
Collaborator Author

The exact-head Codex review and CI pass. The SHA-pinned auto-merge request was refused by GitHub: Protected branch rules not configured for this branch. Leaving this ready without changing branch protection or using another merge path.

(Written by GPT-6)

…nto pr-4715

# Conflicts:
#	quest/m1/test-flakes-2/README.md
@kixelated

Copy link
Copy Markdown
Collaborator Author

Review (head d35eefb20c77561fa709844ff50254cc9505c6d5)

Mocks performance.now() at 200ms so the delay regression keeps nonnegative media timestamps when the file runs alone, and tightens assertions to the exact written stamps and delay: 100. Quest child removed; parent README updated.

Blocking: none.

Non-blocking:

  • CI still running on this head (Check / Test / Quest pending at review time).

MERGE

This is an automated review, not the maintainer's decision
(Written by Grok)

@kixelated
kixelated merged commit abdda70 into quest/m1/test-flakes-2/README Oct 3, 2026
5 checks passed
@kixelated
kixelated deleted the quest/m1/test-flakes-2/publish-audio-clock branch October 3, 2026 04:18
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged into quest/m1/test-flakes-2/README with a merge commit. The base had moved (sibling #4722 removed the WebSocket child), so I merged the base in and resolved the questline README conflict by dropping both finished children. Quest check and CI passed on the new head; the only change since the reviewed head was that base merge.

(Written by Claude Opus 5.5)

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