Skip to content
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

Fixing issue with Windows timeout test #1946

Closed
wants to merge 5 commits into from

Conversation

esantorella
Copy link
Member

@esantorella esantorella commented Jul 25, 2023

Context and explanation: Timing works differently on Windows, and its imprecision has caused test failures in the past. When we started testing against Python 3.11, Windows tests involving timing started failing flakily. Adding some print statements showed that when the tests failed, time elapsed was measured as 0.0 seconds when it should have been substantially more. This is odd, since timing (including monotonic) was supposed to be more precise in Python 3.11. I couldn't debug this too thoroughly since I don't have access to a Windows machine outside of GitHub Actions.

The fix: Increased sleep from 1e-3s to 1e-2s in a test where we are making sure something times out within a limit of 1e-4s.

Test plan:

I'm going to run units a few times to ensure that they pass consistently.

@esantorella esantorella self-assigned this Jul 25, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 25, 2023
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Merging #1946 (23cdfd7) into main (4871a9a) will not change coverage.
The diff coverage is n/a.

❗ Current head 23cdfd7 differs from pull request most recent head 2d72246. Consider uploading reports for the commit 2d72246 to get more accurate results

@@           Coverage Diff           @@
##             main    #1946   +/-   ##
=======================================
  Coverage   99.94%   99.94%           
=======================================
  Files         177      177           
  Lines       15693    15693           
=======================================
  Hits        15685    15685           
  Misses          8        8           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@esantorella
Copy link
Member Author

The bug is strange. Windows with Python 3.11 is measuring an elapsed time of 0.0 after sleeping for 1e-3s, but timing in Windows is supposed to be more accurate in 3.11.

@esantorella esantorella marked this pull request as draft July 25, 2023 23:11
@esantorella esantorella marked this pull request as ready for review July 26, 2023 19:25
@esantorella esantorella changed the title [WIP, don't commit] Fixing issue with Windows timeout test Fixing issue with Windows timeout test Jul 26, 2023
Copy link
Contributor

@saitcakmak saitcakmak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I think it's ok to wait an extra 0.01 secs. Debugging a windows specific Python issue is probably not worth the extra time investment.

@facebook-github-bot
Copy link
Contributor

@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@esantorella merged this pull request in cca54db.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants