-
Notifications
You must be signed in to change notification settings - Fork 117
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
[QA] Fix flaky data_manager
unit tests
#497
Conversation
data_manager
unit tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petar-qb this is reallyawesome work 💯 Not only have you fixed the flakiness, the data manager tests have gone from taking ~35s (which was always a source of mild irritation for me) to around 3.5s! I've tested the latest version I pushed by running pytest test_data_manager.py
100 times. Before the change, that would fail ~10% of the time, and now there's not a single failure 🥳
I made a couple of commits, see what you think. My reasoning is:
- we should try to keep tests as close to real/unmocked behaviour as possible, and only tests in data manager use
time.sleep
- hence let's not make it
autouse=True
or increase the fixture scope outside where it's strictly needed and instead keep it as explicitly opt-in - set
tick=True
for similar reasons to ensure time is only frozen for the purpose of fast-forwardingtime.sleep
and does not artificially make subsequent calls to.load
seem simultaneous - monkeypatching the
time.sleep
was super clever but I would again prefer to be explicit, especially given thatfreezegun
comes with the.tick
method for exactly this, and monkeypatching is always quite fragile (e.g.from time import sleep; sleep()
isn't the same asimport time; time.sleep()
) - the downside is that any future tests that use
time.sleep
will not automatically benefit, but I think this is ok because I don't anticipate there being many such things (just tests of caching really), and I'd rather be explicit here rather than it being too magical
@antonymilne thanks for taking the PR to a new level. I really like how the tests cases are now much more realistic (with timeouts of 300s and 100s). I agree with all your bullets from the comment above, but unfortunately I can't approve the PR I created 😄. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you guys 💪
Description
See issue for more.
Changes introduced in this PR:
Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":