-
Notifications
You must be signed in to change notification settings - Fork 531
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
Reduce GitHub Actions unit test durations by removing tox and using test parallelization #1345
Conversation
Relates-to commit 12831dc.
…ted when checking for them
…i/reduced-unittest-duration-poc
…ittest-duration-poc
…educed-unittest-duration-poc
Note: removal of |
This seems like a great improvement, running the parallel tests on my local machine cut runtime by over 35 seconds. I wonder if it would be worth including information about this alternative testing method in the readme if it gets merged. |
I'm not sure how to decide about that, to be honest. It's good that the library should (and does, afaik) be testable with a super-simple, standard workflow (create a venv, source that, pip install, run |
We could include something in the existing **Check that everything is working by running the tests**
~```shell
$ python -m unittest
~```
This will run all the tests for all the scrapers. You should not see any errors or failures
**OPTIONAL: To run the full test suite in parallel**
~```shell
$ pip install unittest-parallel
$ unittest-parallel --level test
~``` |
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.
Great improvements all around!
Once #1343 is merged this will be a great addition.
Conflicts: .github/workflows/unittests.yaml
Co-authored-by: Joey <[email protected]>
Thanks again - and agreed with the idea of adding the parallel-tests mode to the dev documentation. Merging this soon. |
(ah, ok; some refactoring of the documentation required first, by the looks of it. I'll spend some time on that during the week) |
In fact... on second thoughts; let's merge the test-parallelism for continuous integration first, because that affects open pull requests and merges. I'll include the documentation change during the week. |
This reverts commit cf6aa49.
Note: this change appears to have been incompatible with the It seems that this legacy test is the problem there: recipe-scrapers/tests/legacy/__init__.py Line 50 in 11e7ed2
I'll investigate that soon. In some ways it appears that it could be similar to #1342 (per-class test setup). |
I removed
unittest-parallel
from our test suite a couple of years ago in #660, under the justification that it introduced unnecessarily complexity.However: to support a potential increase in the number of pull requests we run tests for, and based on noticing an increase in the number of continuous integration machine-minutes spent, I'd like to see if we can reduce the duration of our GitHub Actions unit test workflows, and believe that
unittest-parallel
may be an effective way to do that. So here it is again, although only as a dependency during the relevant workflow.Depends-upon / includes: