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

Work around flakyness in spill hysteresis #5850

Closed
wants to merge 12 commits into from

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Feb 22, 2022

@crusaderky crusaderky self-assigned this Feb 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 22, 2022

Unit Test Results

       12 files  ±0         12 suites  ±0   7h 14m 0s ⏱️ - 8m 4s
  2 622 tests +1    2 541 ✔️ +4    81 💤 ±0  0  - 3 
15 656 runs  +6  14 764 ✔️ +6  892 💤 +3  0  - 3 

Results for commit 8152265. ± Comparison against base commit 0e12374.

♻️ This comment has been updated with latest results.

@crusaderky
Copy link
Collaborator Author

@crusaderky crusaderky marked this pull request as ready for review February 24, 2022 11:31
@crusaderky crusaderky added the flaky test Intermittent failures on CI. label Feb 24, 2022
Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

I would like to start a discussion about how we write tests for memory related things.

  • This particular test is very complex and slow
  • We're allocating a lot of memory. I would prefer keeping unit tests low on resources even if 1GB should be not a problem
  • Relying on OS RSS reporting appears to be fragile. We're still xfailing on OSX.

I believe this is a situation where I would prefer mocking. The one measurement we rely on that is critical and the culprit for all the flakiness is proc.memory_info().rss. This is a very stable API and we could easily mock this by encapsulating it in a function. In the end, what we want to test is our spill logic assuming psutil/OS does properly report RSS. We do not want to test how the OS manages memory, this is outside of our control.

@crusaderky
Copy link
Collaborator Author

Superseded by #5870

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky test Intermittent failures on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test_spill_hysteresis flaky on ubuntu
2 participants