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

Reenable sleep ns spec on MacOS #1212

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Conversation

headius
Copy link
Contributor

@headius headius commented Nov 6, 2024

Investigating why this spec fails on MacOS in hopes of re-enabling it.

@headius
Copy link
Contributor Author

headius commented Nov 6, 2024

This failure seems to be much more than just unreliability in Kernel#sleep. The test attempts to sleep 100 times for 0.1ms each time and confirm that the total time spent is between 10 and 30 ms. The logging I added shows that CRuby instead sleeps for anywhere from 89ms to over 350ms, on average an order-of-magnitude difference.

I'm attempting to disable parallel specs on MacOS to see if that helps this produce more reasonable results.

@herwinw
Copy link
Member

herwinw commented Nov 6, 2024

FYI: #1213 has a change that improves the " Expected false == true" error message to show the actual values. Feel free to integrate that in your branch if that makes debugging easier.. Otherwise, I'll just leave that PR on Draft until this issue is fixed, to prevent adding another merge conflict.

@eregon
Copy link
Member

eregon commented Nov 7, 2024

Let's close, it doesn't even pass reliably on Linux

@eregon eregon closed this Nov 7, 2024
@headius
Copy link
Contributor Author

headius commented Nov 7, 2024

Please don't close my PRs.

@headius headius reopened this Nov 7, 2024
This spec was originally added to confirm that sub-millisecond
sleeps actually did sleep; on JRuby and TruffleRuby before fixes
a sleep of 0.0001s would immediately return, because we both used
a sleep function with a minimum resolution of 0.001s. Ideally 100
sleeps of 0.0001s should not exceed 0.03s, but since that's not
the goal of this spec and since it makes the spec flaky under load
or on slower systems, it seems best to remove this check.

The remaining check just confirms that 100x sleep of 0.0001s does
actually sleep for at least 0.01s. Any Ruby failing the spec now
would indicate they are not actually sleeping for 0.0001s and they
need a fix.
@headius headius marked this pull request as ready for review November 7, 2024 19:45
@headius
Copy link
Contributor Author

headius commented Nov 7, 2024

@eregon I have made the change I suggested in #1207 (comment), removing the < 0.03 check that caused the spec to be unpredictable on some combinations of Ruby and platform. I believe it should now reliably pass on all Ruby implementations that properly implement sub-millisecond sleep times.

@headius headius requested a review from eregon November 7, 2024 19:52
Copy link
Member

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thanks, let's try this.

@eregon eregon merged commit bbcd077 into ruby:master Nov 7, 2024
14 checks passed
@headius headius deleted the sleep_ns_spec_on_macos branch November 7, 2024 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants