-
Notifications
You must be signed in to change notification settings - Fork 263
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
Hypothesis warnings and speed up some tests #1238
Conversation
OK so we still need to limit the version of hypothesis somewhat, else it reports slow tests as errors. |
On further investigation, it appears that the colorbar location is more dynamic in mpl 3+. Calling |
e3c1896
to
b8f7867
Compare
49eda27
to
8d5318a
Compare
This PR has revealed a tangential issue (causing a test to intermittently fail) that was somewhat tricky to track down. The strategy ShortMem is listed as I fixed it and some other tests. Even though the strategy only uses the last 10 rounds, its behavior changes at round 10, therefore it has to know the exact number of rounds played, i.e. the full history. |
I also re-pinned hypothesis. I was getting some odd coverage issues we using a newer version. Essentially coverage thought that some of the custom hypothesis decorators were not covered even though they have direct tests. |
I noticed that: how weird... I'll have a proper look through everything later today @marcharper, nice work 👍 |
) | ||
@example( | ||
min_memory_depth=float("inf"), | ||
max_memory_depth=float("inf"), | ||
memory_depth=float("inf"), | ||
strategies=strategy_lists(min_size=20, max_size=20), |
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.
This should be an actual list of strategies here for the @example
. To do this properly with hypothesis I believe we should remove the example case here, and write a second test OR we could go with just strategies=short_run_time_strategies
(the correct import will be needed at the top). I've run this locally and it seems fine without a large speedup.
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.
It's strange because these tests are running fine for me locally but not on Travis, and we didn't explicitly list strategies in this example previously.
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.
Strange that it runs locally... Perhaps a slightly different version of hypothesis has implemented the ability to use strategies inside examples... Would be neat if it did...
Previously I think we were just using all_strategies
from the global space.
.travis.yml
Outdated
@@ -24,7 +24,7 @@ script: | |||
- cd docs; make clean; make html | |||
# Run the test suit with coverage | |||
- cd .. | |||
- travis_wait 60 coverage run --source=axelrod -m unittest discover | |||
- travis_wait 60 coverage run --source=axelrod -m unittest discover -v |
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.
Double checking that the verbose tag isn't just something you wanted for debugging @marcharper? I don't necessarily mind either way (it does make scrolling through the travis log a big long but no big deal).
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.
I'll take it out
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.
I'm fine with adding it back in, I did add it just for debugging Travis
Very nice work @marcharper, this is a welcome speedup/refactor of the tests 👍 |
The first commit removes the hypothesis decorator kwarg
max_iterations
that give deprecation warnings (and are reported as no-ops). I changed hypothesis to be pinned at >= 3.2 instead of ==. [Edit: see below, also limited to <=3.68] See #1214 .The second commit reduces the run time of some of the longer tests. These changes cut the local total test run time down by 65% (excluding doctests, which take about 60 seconds to run). For example some tests now avoid the Meta strategies (like the filter set tests). Others now only test a random sample of strategies. (We may need to add some random seeding.) There are still tests that run larger tournaments so I think the risk for this change is low.
Also, I marked one test as skippable if the matplotlib version is 3+, and removed the restriction in requirements.txt. Alternatively we could require matplotlib 3+ and update the test. It's just the colorbar position, so I think we can live without the test for a short while. See #1209 .
Closes #1209