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

Speed up tests ~2x #1352

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Speed up tests ~2x #1352

merged 1 commit into from
Nov 10, 2023

Conversation

adamchainz
Copy link
Contributor

Description of the Change

Speed up the test suite to be about twice as fast, using these techniques:

  1. Move model creation into setUpTestData(), so it runs only once per test case. See my post.
  2. Remove tearDown() methods that removed model instances. These were always unnecessary as Django resets the database for you.
  3. Move immutable RequestFactorys to class-level variables.

Measuring on the Python 3.12 and Django 5.0 environment (added in #1350), I got 207s before these changes and 105s afterwards, approximately twice as fast.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #1352 (8227484) into master (fa4bcdf) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1352   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files          32       32           
  Lines        2120     2120           
=======================================
  Hits         2068     2068           
  Misses         52       52           

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@adamchainz
Copy link
Contributor Author

setUpTestData copying was only added in Django 3.2, before that the djagno-testdata package is needed. I think we should merge #1350 first to drop Django 2.2 support, then I'll rebase this.

Copy link
Contributor

@dopry dopry left a comment

Choose a reason for hiding this comment

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

@adamchainz As a maintainer who updates a lot of branches and spends a lot of time waiting on CI spinners, I can't tell you how much I appreciate this!

@dopry dopry merged commit e15e245 into jazzband:master Nov 10, 2023
25 checks passed
@adamchainz adamchainz deleted the speed_up_tests branch November 12, 2023 08:45
@n2ygk
Copy link
Member

n2ygk commented Dec 18, 2023

@adamchainz @dopry not sure why but I've noticed that all the tests run and then the last two success jobs seem to hang for a long time waiting for a runner. Maybe because success is a different job from build? Is there a way to add the Success step into the build job somehow? I'm not sure how to skip the matrix part within the build job...

@dopry
Copy link
Contributor

dopry commented Dec 19, 2023

@n2ygk, Success needs to wait on the matrix build to complete. It can't be bundled into the same. can you open a new issue linking to an example?

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2023

@dopry not point opening a new issue if we are stuck with the current situation. Still better than the old way.

@dopry
Copy link
Contributor

dopry commented Dec 19, 2023

If you can't point to an example of the issue I can't troubleshoot it further and I suspect it is not directly related to reducing the overheat to setup test data, but is more likely related to capacity or organizational throttling by GH. Correlation is not causation.

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2023

@dopry #1376

@n2ygk
Copy link
Member

n2ygk commented Dec 19, 2023

But I think I should have been commenting on #1219

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