Skip to content

Conversation

@PGijsbers
Copy link
Collaborator

A few improvements to tests that should reduce test time without affecting test quality.

The test does not require the list of flows to be updated, to a single
cached version will do fine (this call otherwise would take ~40
seconds).
Downloading a run takes a non-significant amount of time (est. 300ms on
my current setup). It is unnecessary to compare against all >=100 runs,
while a handful should do fine (perhaps even just one should do).
The batch size required in some pages over 40 pages to be loaded, which
increased the workload unnecessarily. These changing preserve pagination
tests while lowering the amount of round trips required.
Since it is already covered by test_run_and_upload_randomsearch.
Speeds up ~25x, and reduces network traffic.
Loading a page takes ~600ms. I don't think testing with 3 pages is any
worse than 10. I also think this is an ideal candidate of test that
could be split up into (1) testing the url is generated correctly, (2)
testing a pre-cached result is parsed correctly and (3) testing the url
gives the expected response (the actual integration test).
If the test is that swapped parameters work, we don't need a complicated pipeline or dataset.
@PGijsbers PGijsbers requested a review from mfeurer October 27, 2020 19:28
@PGijsbers
Copy link
Collaborator Author

I think this seems like a reasonable set of changes for one PR, so I would like to have some feedback (or approval) 👍

Copy link
Collaborator

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

This looks great, but I have once change request. Also, I'd like to see what this does in practice. And finally, there are some failures on appveyor :(

@PGijsbers PGijsbers merged commit 07e87ad into develop Oct 29, 2020
@PGijsbers PGijsbers deleted the speed_up_tests branch October 29, 2020 09:49
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.

3 participants