Fix time-distortion offset math and Clock.offset double-counting; restore + harden the test suite#2
Merged
Merged
Conversation
Constant distortions are linear, so the accumulated offset must not depend on how finely the clock is polled. Poll the README example once vs. every minute across the same span and assert identical results. This property guards Clock's offset-accumulation/overlap-clamping math against whole classes of regressions.
Distortion offsets are summed, so the order of the array passed to Clock must not affect the result. Walk a forward- and reverse-ordered clock through identical hourly checkpoints and assert they agree at every step.
A dilation that loses 3h followed by a compression that reclaims exactly 3h must return the clock to perfect sync and leave no residual drift. Also pins the symmetric lag profile (-1.5h at 4am and 8.5am, -3h peak at 7am) through the excursion.
The offset getter folded the elapsed [lastCheck, now] slice into _offset but left _lastCheck unadvanced; advancing it was done separately by markLastCheck() on the relativeTimeInMillis path. So reading offset on its own (or twice) re-accumulated the same interval, inflating the distortion. Advance _lastCheck inside offset so each slice is consumed exactly once, and drop the now-redundant markLastCheck(). relativeTimeInMillis is unchanged in behavior. Adds tests asserting repeated reads are stable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A distortion window is described as a wall-clock time-of-day and always resolved against the current calendar date, so a window that wraps past midnight goes dead once the date rolls over: its start is recomputed as tonight's (future) occurrence and the in-progress distortion drops out. Pin both the working before-midnight case and the broken after-midnight case so the gap is visible and a future date/timezone-aware version has a regression target. This is the README's overnight scenario.
The library does not validate distortion parameters (README roadmap #2). Pin current behavior: a zero reference duration makes distortTime return Infinity, two zero durations return NaN, and a negative relative duration silently produces an exaggerated dilation. End-to-end, a zero-reference window poisons relativeTimeInMillis with NaN permanently -- documented here as motivation for future validation.
ConstantTimeDilation and ConstantTimeCompression share one formula, so their names are advisory intent rather than enforced behavior. Pin that identical params yield identical results across the two classes, and that a "dilation" with relative > reference speeds the clock up while a "compression" with relative < reference slows it down.
durationInMillis is real elapsed time between two wall-clock times, so across a DST transition it diverges from the nominal wall-clock span. Assert America/New_York loses an hour on the spring-forward day (3h span => 2 real hours), gains one on fall-back (3h span => 4 real hours), and matches the span on an ordinary day. Guards the Temporal-backed behavior.
compareWithinWindow treats [start, end) as half-open: the start instant is WITHIN, the end instant is LATER. Clock depends on this so adjacent non-overlapping windows hand off without gap or overlap. Assert both boundaries to the millisecond.
Assert add advances the wall-clock time, wraps past midnight (a time-of- day carries no date, so 23:00 + 2h is 01:00), and preserves millisecond precision. Also assert forTodayInMillis resolves seconds and milliseconds.
The matrix tested 12/14/16/18, all now end-of-life, while the bug that broke the suite was a modern-Node issue. Switch to 18/20/22/24 (18 kept as a compatibility floor). Verified all four pass locally.
Pin actions/checkout (v7.0.0) and actions/setup-node (v6.4.0) to commit SHAs in the publish workflow, matching the CI workflow. This is the workflow that holds the npm token, so tag-hijack hardening matters most here. Also correct the misleading "GitHub Packages" comment to "npm registry", which is what it actually targets.
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.
Summary
This branch fixes two correctness bugs in the clock-distortion engine, repairs the
(previously unrunnable) test suite, and substantially hardens it. The README's own
documented example now actually reproduces.
Bugs fixed
1.
distortTimeused the wrong formula.ClockaddsdistortTime(dt)to an offsetthat is itself added to real time, so the correct contribution is the offset delta
dt * (relative/reference − 1). Both distortions computed something else(
−dt*(rel/ref)and+dt*(ref/rel)). The dilation half of the README worked only bycoincidence at a 2:1 ratio; compression never re-synced. Both classes now use the single
correct formula, and the full README hour-by-hour table reproduces exactly (including the
10:00am re-sync).
2.
Clock.offsetdouble-counted. The getter folded the elapsed[lastCheck, now]slice into
_offsetbut left_lastCheckunadvanced — that was done separately on therelativeTimeInMillispath. Readingoffseton its own (or twice) re-accumulated the sameinterval. It now advances
_lastCheckitself, consuming each slice exactly once;relativeTimeInMillisbehavior is unchanged.Test infrastructure
Fake timers fixed. Modern Node makes
globalThis.performancenon-configurable, whichJest 28's bundled fake-timers can't hijack — the entire suite threw on setup. Added
fakeTimers: { doNotFake: ['performance'] }. The suite now runs on Node 18 and 24.Tests that had been written to match the buggy output were corrected to assert the right
behavior (the old
'documentation example'never exercised compression;'returns the relative time'asserted 1.5× where its own comment said "double-time"; the isolateddistortTimetests encoded the wrong semantics).Hardening (new tests)
net-zero (with symmetric lag profile).
TimeWindowdurations (spring-forward 3h→2h real,fall-back 3h→4h real), half-open
[start, end)window boundaries,ClockTime.addmidnight wrap + sub-second precision.
the date rolls over (the README's overnight scenario); unvalidated degenerate durations
produce
Infinity/NaNand can poisonrelativeTimeInMillispermanently. Both arealready noted in the README roadmap and pinned here as regression targets.
case, as executable documentation.
Follow-ups surfaced (not in this PR)
publish#3).Verification
npm run test:ci: 47 tests, 10 suites, all green, 100% coverage, lint clean, on bothNode 18 and Node 24.