Pin the suite's clock so a rendered elapsed time stops drifting - #6043
Open
backspace wants to merge 7 commits into
Open
Pin the suite's clock so a rendered elapsed time stops drifting#6043backspace wants to merge 7 commits into
backspace wants to merge 7 commits into
Conversation
The seam and the guard gave card code one clock to read and kept it that way. Neither changed anything on its own: unpinned, `now()` is `Date.now()`, so every rendered age still depended on when the suite ran. This sets the pin, which is the point of the other two. What it buys is that "3d ago", a countdown, an age and an "expires soon" warning become functions of their own data. A visual comparison of them is then a comparison of the code, rather than a race against whichever threshold each value happens to be nearest — and the way to quiet one stops being to hide it, which is what trades the coverage away. Realms the tests build in the browser stamp their files with the same instant, so they still read as `today`, exactly as they did when both sides were the real clock. Nothing about those moves. Realms served from the index cache carry content-derived mtimes — fixed dates scattered across decades — and their ages are now measured from the pinned instant rather than from today, which shifts some by a unit once. That is the one-time cost of their never shifting again. The adapter's token-expiry comparison is deliberately left on the real clock. It is auth arithmetic rather than a rendered value, and freezing it would change when a test's tokens are considered live, which is a different question from what a card displays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… clock Two things the first run found. Stamping every fixture write with the pinned instant broke change detection. The indexer decides what a from-scratch pass must revisit by comparing a file's mtime against the one on its index row and skipping where they match, so a write that leaves the mtime alone is an edit it cannot see — `full indexing skips over unchanged items in index` caught exactly that. Stamps now advance a second at a time from twelve hours below the pinned instant. Below matters: a mtime after it is in the future, which renders as an absolute date rather than an age. Within the same day matters too, since that is what keeps these files reading as `today`, as they did when both the clock and the stamp were the real one. The calendar test asked the real clock what "today" is while the field it renders asked the pinned one, so the two disagreed about which days a `today` sentinel disables. It reads the pin now. The instant moves to mid-month for the same test: its `getDate() > 1` guard means that pinned to the first, the assertion would have skipped rather than failed, which is a quieter way to lose a check than leaving it broken. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pinning the suite's clock made a file read "Last saved in 6 days" — a saved timestamp in the future. The fixtures carried the pinned instant while `formatLastSavedText` measured them against the wall clock, so the two disagreed by exactly the distance between them. The seam covered card code and stopped there, but the host app renders elapsed time from the same timestamps. So the implementation moves to runtime-common, which both sides already depend on: `base/helpers/clock` is now the path card code imports it by, and the host app imports it directly. A global is still what actually crosses the boundary, because the card loader hands card code its own module instances and a module-level variable would not be shared at all. `formatLastSavedText` already took its `now` as a parameter; only the default changes, so nothing that passes one behaves differently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`runtime-common` is loaded as Node ESM, where a relative specifier resolves literally, so `./clock` names a file that does not exist. Every other relative export in the barrel carries `.ts` — 126 of them — and this one did not, which is the whole failure: the prerender service could not start, and twenty jobs reported failure without running a single assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c92ad4813a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 43m 22s ⏱️ + 23m 11s Results for commit 32bdae0. ± Comparison against earlier commit 7663940. For more details on these errors, see this check. Realm Server Test Results 1 files ± 0 208 suites +18 1h 15m 30s ⏱️ + 10m 43s Results for commit 32bdae0. ± Comparison against earlier commit 7663940. |
Two constraints on a fixture's mtime pull against each other, and the twelve- hour floor satisfied only one of them. They have to advance, or the indexer cannot see an edit: it decides what a from-scratch pass revisits by comparing a file's mtime against its index row and skipping where they match. They also have to stay within a minute of the pinned instant, because `formatLastSavedText` calls anything closer than that "just now", and the inspector asserts a seeded fixture reads that way — which it did when both the clock and the stamp were the real one, the files having been written moments before the assertion. A minute of one-second steps is sixty values, which one shard would exhaust at once. Per adapter it is ample: a realm seeds once and a test writes a handful of times. Sequences in different realms overlap, which costs nothing, because the comparison that matters is between a file and its own index row. The clock-override tests now restore the suite's pin rather than deleting it. Deleting dropped every later test in that shard back onto the wall clock while its fixtures still carried pinned timestamps — the same mismatch this branch exists to remove, reintroduced by its own teardown. Imports move to the groups `import/order` expects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`import/order` sorts relative specifiers by kind: `./stream` and `./test-clock` are siblings, `.` is an index import, and siblings come first with a blank line between the groups. Putting the new import next to the index one split a group and inverted the order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…to cs-12770-pin-test-clock # Conflicts: # packages/base/helpers/clock.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The seam beneath this gave card code one clock to read and kept it that way, but changed nothing on its own: unpinned,
now()isDate.now(), so every rendered age still depended on when the suite ran. This sets the pin, which is what the other two were for."3d ago", a countdown, an age and an"expires soon"warning become functions of their own data. A visual comparison of them is then a comparison of the code, rather than a race against whichever threshold each value happens to be nearest — and the way to quiet one stops being to hide it, which is what trades the coverage away.The parts that are not obvious
Fixture stamps have to move with the clock, and they cannot all be one value. Realms the tests build in the browser stamp their files with the pinned instant, so they still read
todayexactly as they did when both sides were the real clock. But the indexer decides what a from-scratch pass must revisit by comparing a file's mtime against the one on its index row and skipping where they match, so a write that leaves the mtime alone is an edit it cannot see. Stamps therefore advance a second at a time from twelve hours below the pinned instant: below, because a mtime after it is in the future and renders as an absolute date rather than an age; within the same day, because that is what keeps them reading astoday.The clock has to cross the card-loader boundary. The host app renders elapsed time from the same timestamps card code does —
formatLastSavedTextamong them — so a pin that reached only card code produced"Last saved in 6 days", a saved timestamp in the future. The implementation lives inruntime-common, which both sides already depend on, and a global is what actually crosses, because the loader hands card code its own module instances.The instant is mid-month on purpose. A calendar test asserts that days earlier in the current month are disabled by a
todaysentinel, and guards itself withgetDate() > 1. Pinned to the first, that assertion would skip rather than fail — a quieter way to lose a check than leaving it broken.The adapter's token-expiry comparison stays on the real clock. It is auth arithmetic rather than a rendered value, and freezing it changes when a test's tokens are considered live, which is a different question from what a card displays.
What this costs
Nothing, so far. Every Percy build on this branch that produced snapshots reported
changed=0— the pinned instant is close enough to the present that no content-derived mtime crossed a unit boundary. If a later instant ever does move a baseline, that is the one-time price of those labels never moving again on their own.Verification state
Two runs found real bugs, both fixed here: fixture mtimes defeating the indexer's change detection, and the calendar test measuring from a different clock than the component it renders.
Three further runs produced no test evidence at all — two lost to GitHub artifact upload failures, one to a missing file extension in a re-export, fixed. So the host-side clock fix and that extension are correct by inspection but have not yet had a full green run behind them. That is what this PR's first run is for.