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

Make test suite run faster #115

Merged
merged 25 commits into from May 9, 2021
Merged

Make test suite run faster #115

merged 25 commits into from May 9, 2021

Conversation

create-issue-branch[bot]
Copy link
Contributor

@create-issue-branch create-issue-branch bot commented May 4, 2021

closes #114

@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented May 4, 2021

Test results

  • Start of speed-up-tests branch off of main (8ed869d)
Results (296.89s):
     189 passed
  • Avoid redundant queries in test suite (6c7ad68)
Results (296.76s):
     189 passed
  • Reduce the use of autouse=True (30b3831)
Results (288.15s):
     189 passed
  • Clean redis only in API and data package (fc08c60)
Results (288.04s):
     189 passed

Results (289.62s):
     189 passed
  • Less setup of market prices (f7a8bff)
Results (288.22s):
     189 passed
  • Set up charging stations only when tests need them (2a8449b)
Results (292.38s):
     189 passed
  • Set up assets only when tests need them, and split off setup of asset types (reverted commit)
Results (268.68s):
     189 passed
  • Set up assets only when tests need them (stop autouse), and split off setup of asset types (2622d28)
Results (249.87s):
     189 passed
  • Setup database with module scope (31f4613)
Results (94.90s):
     189 passed
  • Refactor test db setup at different scopes (407da74)
Results (96.12s):
     189 passed

@Flix6x Flix6x marked this pull request as ready for review May 4, 2021
@Flix6x Flix6x requested a review from nhoening May 4, 2021
@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented May 4, 2021

To summarise: the main time savers are to:

  • have tests reuse a database where possible
  • avoid using autouse=True, especially in higher up conftest.py files.

Copy link
Contributor

@nhoening nhoening left a comment

I commend you for this effort! I didn't check every test in itself, but they do still all pass, which is also some feedback, and some do so really fast. Kudos.

One thing I would want to see a little different concerns being explicit (as per usual):

As I see it, the biggest organisational change is that you divided many test files into the ones with function-level db creation, and the one with module-level db creation.

OI think that's fine, as it can guide future test authors to make a decision about this. We could even explicitly state that this is a part of writing tests now. A policy. Could be mentioned in the main conftest.

And the filenames go a little like this: test_fresh_api_v1.py . I would be more explicit (what is "fresh_api"?), e.g. test_api_v1_fresh_db.py

@Flix6x
Copy link
Contributor

@Flix6x Flix6x commented May 8, 2021

I renamed according to your suggestions and updated the developer documentation in our main conftest.

@Flix6x Flix6x requested a review from nhoening May 8, 2021
@@ -18,6 +18,7 @@ Bugfixes
Infrastructure / Support
----------------------
* Make assets use MW as their default unit and enforce that in CLI, as well (API already did) [see `PR #108 <http://www.github.com/SeitaBV/flexmeasures/pull/108>`_]
* Recycle the test database to shave 2/3rd off of the time it takes for the FlexMeasures test suite to run [see `PR #115 <http://www.github.com/SeitaBV/flexmeasures/pull/115>`_]
Copy link
Contributor

@nhoening nhoening May 9, 2021

Choose a reason for hiding this comment

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

I'd say "Re-use the database between the automated tests, if possible. This shaves 2/3rd off the time ..."

@Flix6x Flix6x merged commit 6eeefa6 into main May 9, 2021
2 checks passed
@Flix6x Flix6x deleted the issue-114-Make_test_suite_run_faster branch May 9, 2021
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.

2 participants