tests(core): Fix flaky metric sequence number test#19754
Conversation
Mock `timestampInSeconds` to return a fixed value so the sequence counter does not reset due to millisecond boundary crossings between calls. Fixes #19749 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| it('increments the sequence number across consecutive metrics', () => { | ||
| // Mock timestampInSeconds to return a fixed value so the sequence number | ||
| // does not reset due to millisecond boundary crossings between calls. | ||
| vi.spyOn(timeModule, 'timestampInSeconds').mockReturnValue(1234.567); |
There was a problem hiding this comment.
l: can we use vi.useFakeTimers() instead of mocking our timer?
There was a problem hiding this comment.
any specific reason you would prefer this? I think it would probably work as well but I don't think it matters here since this is some abstractions down from what we are testing
There was a problem hiding this comment.
Basically just that it's the more "vitest-esque" way of influencing time in a test. Plus that it's more robust since this globally mocks timers. So if we were to get our time source from something other than timestampInSeconds for some reason, this test wouldn't break. But honestly, this is LOGAF-super-l, so feel free to leave it as-is. Using fake timers is much more relevant in cases where we actually have to wait for a certain timespan.
There was a problem hiding this comment.
I'll let you make the call, I'm fine with either way
There was a problem hiding this comment.
Makes sense, thanks for the detailed explanation! I will leave it as is for this one but will keep it in mind for the future :)
Mock `timestampInSeconds` in the "increments the sequence number across consecutive metrics" test to return a fixed value. The test was flaky because consecutive calls could land on different milliseconds, causing the sequence counter to reset unexpectedly. Closes #19749 --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Mock
timestampInSecondsin the "increments the sequence number across consecutive metrics" test to return a fixed value. The test was flaky because consecutive calls could land on different milliseconds, causing the sequence counter to reset unexpectedly.Closes #19749