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

Fix stuck streamer test....again #1197

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Apr 20, 2023

Summary

  • test_stuck_streamer started failing in CI...although, I've only seen it fail for macOS
  • In the previous attempt to fix this, we talked about the code under test taking a total of 0.25 + 3 * 0.1 == 0.55 seconds while the monkeypatched output streamer persists for at least a second.
    • This time is actually slightly longer:
      • 0.1s for stop_func() to raise KeyboardInterrupt
      • 0.25s for the KeyboardInterrupt to propagate
      • 0.1s * 3 for each sleep() between checking if the output streamer exited
    • So, the total sleep time will take at least 0.65s
  • Obviously other things are happening and I wouldn't expect all that to add up to more than 0.35s in this code.
  • However, it does seem at times that macOS runners may be over-provisioned...and therefore experiencing heavy loads...so, perhaps among the context switches the runners are stalling under the load sometimes introducing just enough delay to fail the test.
    • In most of the cases of failures, the line Log stream hasn't terminated; log output may be corrupted. is missing from the output check
      • This implies the monkeypatched streamer thread finished before Subprocess checked if it was still alive...and the streamer should have still been alive since the code under test should only be taking 0.65s to check this while the streamer runs for 1.0s
    • In another case, the monkeypatched_streamer_was_improperly_awaited Event was set but the output must have included Log stream hasn't terminated; log output may be corrupted. (because that assert passed).
      • So, the streamer stopped waiting after 1.0s, set that failed flag, and then exited but not before Subprocess apparently saw the streamer still running....when it should have exited.

Changes

  • Based on this, I just increased the time the monkeypatched streamer waits for the should_exit flag to 5 seconds.
  • This should completely mitigate the possibility that the "code is just running slow" while under test.
  • And the additional wait time would only ever be experienced under test failure conditions; normally the test will complete with the expected 0.65s code runtime.
  • I also moved the logging check to the bottom of the test so any future failures show which flag isn't set correctly.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 force-pushed the more-stuck-streamer-test-fixes branch from a24f92a to 7628acd Compare April 20, 2023 18:38
@rmartin16 rmartin16 force-pushed the more-stuck-streamer-test-fixes branch 7 times, most recently from 1a34a89 to a2692eb Compare April 20, 2023 21:47
@rmartin16 rmartin16 marked this pull request as ready for review April 20, 2023 21:55
@rmartin16
Copy link
Member Author

Open to criticism of my analysis.

`monkeypatched_streamer_should_exit` signal
@rmartin16 rmartin16 force-pushed the more-stuck-streamer-test-fixes branch from a2692eb to f590e1d Compare April 20, 2023 22:17
Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

All makes sense - the full 5 second timeout shoudln't happen (unless something has gone badly wrong), so it's not so bad to increase the timeout.

@freakboy3742 freakboy3742 merged commit 0aa6527 into beeware:main Apr 21, 2023
@rmartin16 rmartin16 deleted the more-stuck-streamer-test-fixes branch April 21, 2023 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants