-
Notifications
You must be signed in to change notification settings - Fork 141
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
Separate slow tests from quick ones #758
Separate slow tests from quick ones #758
Conversation
Codecov Report
@@ Coverage Diff @@
## main #758 +/- ##
=======================================
Coverage 97.25% 97.25%
=======================================
Files 41 41
Lines 7387 7388 +1
=======================================
+ Hits 7184 7185 +1
Misses 203 203
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Seems like a great idea! I think I don't quite understand how this changes our test strategy, but in principle, I'm all for it! :)
@@ -25,6 +25,9 @@ jobs: | |||
runs-on: ubuntu-latest | |||
steps: | |||
- uses: actions/checkout@v3 | |||
with: | |||
ref: ${{ github.event.pull_request.head.ref }} | |||
repository: ${{github.event.pull_request.head.repo.full_name}} |
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.
What does this do?
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.
This one fixes a problem we had with PRs from forks, while maintaining consistency in catching the correct commit message. There was some issue with the commit message, that sometimes was the correct one and sometimes the merge message (when github makes the merge with the current main before testing). Now it looks robust enough (I'm doing this PR from a fork to prove it)
tox_env: 'py311-test-cov' | ||
use_remote_data: true | ||
experimental: false | ||
slow: true |
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.
So does this take the previous matrix of operating systems and environments, and basically just writes them out individually? Does this mean we're no longer testing python 3.9, for example, and we're only testing Windows and MacOS on Python 3.11?
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.
Our matrix was never an actual matrix, but always a list of single tests, as far as I remember. At this moment, indeed, we are testing with py38, py310 and py311. I couldn't manage to make py312-dev work, so left it out. Would you prefer to also have py39?
@pupperemeritus taught me that there is a nice option in
pytest
, that allows to mark tests as slow and avoid their execution. Here, I'm separating the execution of quick tests from the slow ones, in two ways:@pytest.mark.slow
pytestmark = pytest.mark.slow
.To execute only the slow tests, it's sufficient to use the options
--run-slow -m
(the first option comes frompytest-astropy
)This way, we can execute only unmarked tests in most test suites, and leave only one/two with the full tests. This can give much quicker feedback if a PR breaks something.