Skip to content

test: Fix flaky test dependent on wrongly assumed Date.now() accuracy #1115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 7, 2025

Conversation

wojtekmaj
Copy link
Contributor

@wojtekmaj wojtekmaj commented Apr 4, 2025

Date.now() has millisecond precision but not microsecond precision, so the result might be 99ms due to rounding.
Date.now() is also not guaranteed to increment every millisecond - on some systems, it may tick every ~4ms.

You can see the flakiness e.g. here: https://github.com/toss/es-toolkit/actions/runs/14255327713

Copy link

vercel bot commented Apr 4, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
es-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 4, 2025 9:10pm

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.37%. Comparing base (4be718e) to head (33d2b48).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1115      +/-   ##
==========================================
- Coverage   99.40%   99.37%   -0.03%     
==========================================
  Files         405      405              
  Lines        3541     3541              
  Branches     1048     1048              
==========================================
- Hits         3520     3519       -1     
- Misses         21       22       +1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dayongkr
Copy link
Collaborator

dayongkr commented Apr 5, 2025

Thank you for identifying this issue!

If millisecond precision is causing problems, what do you think about using the Performance API(performance.now()), which offers sub-millisecond precision?

@wojtekmaj
Copy link
Contributor Author

I think it just moves the issue to after the comma :D You'd still need to round the result before asserting it. I personally don't think it's worth it.

@dayongkr
Copy link
Collaborator

dayongkr commented Apr 6, 2025

​Since Date.now() has millisecond precision, using a timing function with microsecond precision should resolve the issue. If not, we could consider using process.hrtime(), which offers nanosecond precision.​

Although You've added comments explaining this, explicitly lowering the test case threshold from 100 to 99 feels a bit cautious.. 🙏

@raon0211 raon0211 changed the title Fix flaky test dependent on wrongly assumed Date.now() accuracy test: Fix flaky test dependent on wrongly assumed Date.now() accuracy Apr 7, 2025
@raon0211 raon0211 merged commit ebb1265 into toss:main Apr 7, 2025
9 checks passed
@wojtekmaj wojtekmaj deleted the patch-1 branch April 7, 2025 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants