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

[#1570] Speed up CI #690

Merged
merged 7 commits into from
Sep 11, 2023
Merged

[#1570] Speed up CI #690

merged 7 commits into from
Sep 11, 2023

Conversation

pi-sigma
Copy link
Contributor

@pi-sigma pi-sigma commented Jun 22, 2023

Various tweaks to speed up the tests:

  • run tests in parallel
  • skip old data migration tests
  • use MD5 for password hashing
  • optimize postgres
  • disable logging for WARNING and below

@pi-sigma pi-sigma changed the title run tests in parallel refactor tests Jun 22, 2023
@pi-sigma
Copy link
Contributor Author

pi-sigma commented Jun 22, 2023

There are two issues with running our tests in parallel:

  1. Elasticsearch uses persistent connections for their search client which doesn't play well with multiprocessing (see here). Unfortunately, there is no public API to tweak things. One option would be to mess with the internals to ensure that each Django test worker is coupled with its own Elastic client. I've looked into this but decided not to pursue this avenue. The solution is bound to be messy and will likely break at some point. The second option was to split the test suite into two, run the "ordinary" tests in parallel, and do a second run for the tests that resist being run in parallel.

  2. Our playwright tests don't play nice with multiprocessing either as Django can't serialize the relevant test classes. For now I simply lumped them together with the Elastic test cases, but we should look into the source of this eventually.

@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2023

Codecov Report

Merging #690 (b9654a7) into develop (a0506b0) will decrease coverage by 2.74%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #690      +/-   ##
===========================================
- Coverage    96.31%   93.58%   -2.74%     
===========================================
  Files          699      697       -2     
  Lines        24670    24595      -75     
===========================================
- Hits         23762    23016     -746     
- Misses         908     1579     +671     
Files Changed Coverage Δ
...c/open_inwoner/accounts/tests/test_auth_2fa_sms.py 100.00% <ø> (ø)
src/open_inwoner/conf/ci.py 100.00% <100.00%> (ø)
src/open_inwoner/haalcentraal/tests/test_signal.py 100.00% <100.00%> (ø)
src/open_inwoner/plans/tests/test_views.py 75.78% <100.00%> (-24.22%) ⬇️
src/open_inwoner/search/tests/test_feedback.py 17.60% <100.00%> (-82.41%) ⬇️
src/open_inwoner/search/tests/test_logging.py 51.85% <100.00%> (-48.15%) ⬇️
src/open_inwoner/search/tests/test_page.py 19.73% <100.00%> (-80.27%) ⬇️
...n_inwoner/search/tests/test_search_autocomplete.py 20.68% <100.00%> (-79.32%) ⬇️
.../open_inwoner/search/tests/test_search_products.py 25.80% <100.00%> (-74.20%) ⬇️

... and 19 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pi-sigma pi-sigma changed the title refactor tests [#1570] Speed up CI Jun 23, 2023
@pi-sigma pi-sigma force-pushed the refactor-tests branch 4 times, most recently from 8120eca to e9a4692 Compare June 26, 2023 09:00
@pi-sigma pi-sigma force-pushed the refactor-tests branch 8 times, most recently from 6272330 to 29cd3f9 Compare August 11, 2023 07:46
@pi-sigma pi-sigma marked this pull request as ready for review August 11, 2023 08:29
@pi-sigma pi-sigma force-pushed the refactor-tests branch 4 times, most recently from 95697a0 to 2b7ea87 Compare August 17, 2023 11:02
if len(sys.argv) > 1 and sys.argv[1] == "test":
logging.disable(logging.CRITICAL)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is what we want. Supposedly we still want to see something when a real problem occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it so we keep logs for level >= ERROR

src/open_inwoner/accounts/tests/test_actions_expire.py Outdated Show resolved Hide resolved
@@ -1,3 +1,5 @@
from unittest import skip

Copy link
Member

Choose a reason for hiding this comment

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

I'd recomment to remove instead of skip these tests if they're no longer necessary

@pi-sigma pi-sigma force-pushed the refactor-tests branch 2 times, most recently from 8c3987a to e0e5870 Compare August 29, 2023 13:40
@@ -1,106 +0,0 @@
from django.contrib.gis.geos import Point
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a specific reason we had to let these go?

If the amount of migrations is an issue we could look into squashing the migrations. IIRC it also had some complications with data migrations.

@@ -13,7 +13,7 @@
{
"django": {
"handlers": ["django"],
"level": "WARNING",
"level": "ERROR",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having doubts about the logging levels.

Maybe we should move that to another issue en get the config changes and parallel test merged.

Copy link
Member

Choose a reason for hiding this comment

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

I agree

src/open_inwoner/conf/test.py Outdated Show resolved Hide resolved
@alextreme alextreme merged commit defa012 into develop Sep 11, 2023
14 checks passed
@alextreme alextreme deleted the refactor-tests branch September 11, 2023 12:05
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.

4 participants