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

Improve startup time of pytest by optimising imports #712

Merged
merged 1 commit into from
Dec 6, 2023

Conversation

GernotMaier
Copy link
Contributor

closes #694

pytest on a simple test file can take >5s to start due to the time required for imports.

This PR optimises the import by moving the simtools.corsika.corsika_histograms import into the corresponding pytest fixture (so it is called only when needed.

This improves startup times for tests notably (at least on my laptop).

@GernotMaier GernotMaier self-assigned this Dec 6, 2023
@GernotMaier GernotMaier added the Less than 15-min Pull Request Review This should be a short pull request with an estimated review time of less than 15 min label Dec 6, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4817fc0) 82.22% compared to head (6f33a52) 82.22%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #712   +/-   ##
=======================================
  Coverage   82.22%   82.22%           
=======================================
  Files          41       41           
  Lines        6210     6210           
=======================================
  Hits         5106     5106           
  Misses       1104     1104           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +281 to +282
from simtools.corsika.corsika_histograms import CorsikaHistograms

Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we add the pylint exception here or it doesnt matter for the test directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not needed, as we do not run pylint over the test directory (we would get lots of complaints, e.g. from all the internal class functions we call in the test).

So no pylint comment required.

Copy link
Contributor

@VictorBarbosaMartins VictorBarbosaMartins left a comment

Choose a reason for hiding this comment

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

I already approve, but there is one comment, which is up to you to implement or not. Thanks!

@GernotMaier GernotMaier merged commit abed91f into main Dec 6, 2023
10 checks passed
@GernotMaier GernotMaier deleted the pytest-accelerate branch December 6, 2023 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Less than 15-min Pull Request Review This should be a short pull request with an estimated review time of less than 15 min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Slow starting of unit tests
2 participants