-
Notifications
You must be signed in to change notification settings - Fork 230
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
CI improvements #906
CI improvements #906
Conversation
As commented in pytest-dev#905, an annotated tag needs a configured user.
@@ -43,5 +43,7 @@ jobs: | |||
|
|||
- name: Push tag | |||
run: | | |||
git config user.name "pytest bot" |
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.
Oops forgot about this 👍
run: | | ||
tox -e ${{ matrix.tox_env }} | ||
tox run -e ${{ matrix.tox_env }} --installpkg `find dist/*.tar.gz` |
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.
Testing the actual package (alike) we intent to ship is a good pattern, and from the tox output seems to work as intended.
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.
at first glance we only test the sdist and not the wheel
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.
Indeed, but AFAIK this is how it always has been: tox generates both wheel and sdist, but only installs sdist:
λ tox -e py310
...
py310: install_package_deps> python -I -m pip install execnet>=1.1 filelock pytest>=6.2.0
py310: install_package> python -I -m pip install --force-reinstall --no-deps E:\projects\pytest-xdist\.tox\.tmp\package\4\pytest-xdist-3.3.1.dev2+g6d39025.tar.gz
py310: commands[0]> pytest
...
Testing both seems a bit overkill, if that's what you are suggesting.
run: | | ||
tox -e ${{ matrix.tox_env }} | ||
tox run -e ${{ matrix.tox_env }} --installpkg `find dist/*.tar.gz` |
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.
at first glance we only test the sdist and not the wheel
Btw, I could swear I have left a comment regarding this, but here it goes: I think the next step is to have another job which depends on
This new job would of course replace the What do you folks think? |
Improvements based on #905: