-
Notifications
You must be signed in to change notification settings - Fork 181
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
Run pytest with different Python versions in GitHub actions #1033
Run pytest with different Python versions in GitHub actions #1033
Conversation
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.
Thanks for working on this. Looks good, added some minor things.
.github/workflows/ci.yml
Outdated
@@ -6,6 +8,10 @@ jobs: | |||
build: | |||
runs-on: ubuntu-latest | |||
|
|||
strategy: | |||
matrix: | |||
python-version: ["3.7", "3.8", "3.9", "3.10"] |
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.
Just the first and the last versions should be enough. Too many builds add noise and consume resources. Or we may want to add something else to the matrix (like CPython/PyPy), and then the matrix will explode (like 8 builds just for that change.
.github/workflows/ci.yml
Outdated
@@ -19,4 +25,4 @@ jobs: | |||
run: flake8 . |
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.
I think it's probably better to have flake8 and pytest in separate builds. Doesn't make so much sense to test flake8 in different Python versions.
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.
Agree perhaps we could use precommit similar to pandas? https://github.com/pandas-dev/pandas/blob/main/.github/workflows/code-checks.yml#L35 but also happy to not overcomplicate things for now
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.
Thank you for the recommendations, I’m going to implement it.
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.
Thanks @LucyJimenez, added some comments.
.github/workflows/ci.yml
Outdated
@@ -3,20 +3,25 @@ name: CI | |||
on: [push, pull_request] | |||
|
|||
jobs: | |||
build: | |||
test: | |||
name: test w/ Python |
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.
Just tests should be enough. To show that it's not pypy we should say CPython, but pypy is likely to be in the matrix, so better to just name this tests
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.
You are right! I changed it.
Additional data that could be interesting about the test:
|
Thanks @LucyJimenez, really nice |
xref: #1036
On this PR: