-
-
Notifications
You must be signed in to change notification settings - Fork 56
run tests in parallel #81
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
base: main
Are you sure you want to change the base?
Conversation
Makefile
Outdated
| test: ## Run all wagtail tests or pass in a file with `make test file=wagtail.admin.tests.test_name` | ||
| docker compose exec -w /code/wagtail web python runtests.py $(file) $(FILE) | ||
| docker compose exec -w /code/wagtail web python runtests.py $(file) $(FILE) --parallel |
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.
We should probably add a separate command for this (or make it possible to pass additional arguments to make test to be passed on to runtests.py). Running tests in parallel is definitely much faster, but sometimes Django/unittest cannot produce a helpful traceback due to some pickling issues, in which case you'd still want to run it in serial.
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.
We could perhaps just add a separate command that runs in parallel for time being? E.g. have both make test and make test-parallel in the Makefile. @smark-1 could you change the PR so that we have these two commands?
Unless @laymonage has a goto solution for passing arguments from the make command, which to me seems like a slightly bigger task.
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 don't have the solution and yep it is quite a task in itself, so I just put it in parentheses in case we do want to figure that one out 😆
But yes, I'm happy with just a separate command!
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.
@saevarom I updated the PR. I am not sure what the best way to document this is.
test-parallelbe a new subsection in the docs?- Or should be mentioned as an comment above each
make test ...option? - Or a paragraph somewhere saying your can replace make test with make test-parallel to run faster but it sometimes cannot produce a helpful traceback?
- other?
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.
@laymonage if https://github.com/ionelmc/python-tblib is used/installed as another dependency would that fix the traceback/pickling issues? Would it then be make sense to set the tests to run in parallel by default?
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.
tblib is already included as part of Wagtail's testing dependencies, but from my experience there are still cases where it fails to produce a helpful traceback, particularly when there's an error during template rendering. See also: https://code.djangoproject.com/ticket/29023
when I run
docker compose exec -w /code/wagtail web python runtests.py --parallelthe tests run significantly faster then when I rundocker compose exec -w /code/wagtail web python runtests.pywithout--parallel.CPU, Memory, and Disk usage appear to be approximately the same. So adding this to the make file would be a big win for me.