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

test: run in parallel across 4 workers, 30s -> 12-14s #569

Merged
merged 1 commit into from
Oct 22, 2019

Conversation

redshiftzero
Copy link
Contributor

Description

this PR makes the change to run the test suite in parallel. We should also monitor pytest-dev/pytest-xdist#229 because right now one cannot run tests in random order as well as run in parallel (:cry:), currently favoring running tests fast but open to discussion

Test Plan

  1. run tests
  2. observe it takes 12-14 seconds

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

  • I have tested these changes in the appropriate Qubes environment
  • I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
  • These changes should not need testing in Qubes (test only)

@sssoleileraaa
Copy link
Contributor

yup, my local measurements show 29s -> 11.2s

If I had to pick, I would prefer tests execute in random order so we are more likely to detect if a test affects the state of another test. We could increase the number of threads from 4 to 20 to make it more random, but I'm worried that's not random enough. Another idea is we pass an argument to indicate when we want to run tests randomly or in parallel. So we could either run something like:

Option 1: make test n where n is the number of threads or make test which would default to passing --random-order-bucket=global instead of -n

Option 2: make test random or make test which would default to n=4

@sssoleileraaa
Copy link
Contributor

In case it helps the discussion, here's a sample of ordering when n=20 vs n=4:

first run (n=20)

[gw2] [ 16%] PASSED tests/test_resources.py::test_load_css 
[gw19] [ 16%] PASSED tests/test_queue.py::test_ApiJobQueue_login_if_queues_running 
[gw18] [ 16%] PASSED tests/test_resources.py::test_load_icon 
[gw19] [ 17%] PASSED tests/test_queue.py::test_ApiJobQueue_logout_removes_api_client 
[gw18] [ 17%] PASSED tests/test_resources.py::test_load_svg 
[gw2] [ 17%] PASSED tests/test_storage.py::test_get_local_sources 
[gw18] [ 17%] PASSED tests/test_resources.py::test_load_image 

second run (n=20)

[gw10] [ 17%] PASSED tests/test_resources.py::test_load_css 
[gw10] [ 17%] PASSED tests/test_storage.py::test_get_local_sources 
[gw19] [ 18%] PASSED tests/test_queue.py::test_ApiJobQueue_enqueue 
[gw17] [ 18%] PASSED tests/test_storage.py::test_update_local_storage 
[gw10] [ 18%] PASSED tests/test_storage.py::test_update_files 
[gw19] [ 18%] PASSED tests/test_storage.py::test_get_local_messages 
[gw17] [ 19%] PASSED tests/test_storage.py::test_update_sources 

first run (n=4)

[gw2] [ 23%] PASSED tests/test_resources.py::test_load_css 
[gw2] [ 23%] PASSED tests/test_storage.py::test_get_local_sources 
[gw2] [ 23%] PASSED tests/test_storage.py::test_get_local_messages 
[gw2] [ 24%] PASSED tests/test_storage.py::test_get_local_files 
[gw2] [ 24%] PASSED tests/test_storage.py::test_get_local_replies 
[gw2] [ 24%] PASSED tests/test_storage.py::test_get_remote_data_handles_api_error 
[gw2] [ 24%] PASSED tests/test_storage.py::test_get_remote_data 
[gw2] [ 25%] PASSED tests/test_storage.py::test_update_local_storage 

second run (n=4)

[gw1] [ 23%] PASSED tests/test_resources.py::test_load_css 
[gw1] [ 23%] PASSED tests/test_storage.py::test_get_local_sources 
[gw1] [ 23%] PASSED tests/test_storage.py::test_get_local_messages 
[gw1] [ 24%] PASSED tests/test_storage.py::test_get_local_files 
[gw1] [ 24%] PASSED tests/test_storage.py::test_get_local_replies 
[gw1] [ 24%] PASSED tests/test_storage.py::test_get_remote_data_handles_api_error 
[gw1] [ 24%] PASSED tests/test_storage.py::test_get_remote_data 
[gw1] [ 25%] PASSED tests/test_storage.py::test_update_local_storage 

@redshiftzero
Copy link
Contributor Author

pass an argument to indicate when we want to run tests randomly or in parallel

I dig it, then we can add a run step to run the tests randomly in CI. I'll update this PR

@redshiftzero redshiftzero force-pushed the parallelize-tests branch 2 times, most recently from f573f29 to 0a29aba Compare October 21, 2019 13:57
running test suite in parallel before we get too far

we should also monitor pytest-dev/pytest-xdist#229
because right now one cannot run tests in random order as well as
run in parallel

until then, I've added another makefile target that can run the tests
in parallel, and have left that running in CI.
@redshiftzero
Copy link
Contributor Author

OK I added back another makefile target to run tests in random order (that's the one that will run in CI since we can tolerate a bit longer test run there). And then for local development we can use make test which will run the tests in parallel.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

This is amazing. Approved :)

This only updates the dev requirements, that is why the debian package builds are still passing in CI. No update of the Python package index is required.

@kushaldas kushaldas merged commit f4dd71a into freedomofpress:master Oct 22, 2019
@redshiftzero redshiftzero deleted the parallelize-tests branch October 22, 2019 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants