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
Tox cleanups and use tox in PR tests #73
Conversation
@@ -17,7 +17,7 @@ matrix: | |||
install: | |||
- pip install --upgrade tox coveralls | |||
|
|||
script: tox | |||
script: tox --recreate test |
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 is fine, but technically we are this close 👌 to not needing this file anymore! travis is still running builds but those builds only go on to power the coverage reporting service. Once we figure out how to use github actions for that, we can eliminate travis altogether.
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.
oh come to think of it, the current github actions are using the example build scripts that are provided by github, which, I believe, means that it ignores our linter configuration and potentially some other stuff? I guess we could ask ourselves whether we want to continue with tox, in which case we probably should switch the github actions to use tox, or otherwise we could eliminate tox altogether? But if we eliminate it, then it's more annoying to run the tests locally... so maybe we should make the github actions use tox?
I had no reason not to use tox with them initially, other than that I was more concerned with getting the actions to run properly at all, than i was with the specifics of how we run the 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.
oh, good point. I'd lean toward using tox for the github actions too.
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.
Switched the github action over to tox as part of this PR and it seems fine (amazingly). Tox is set up to use pylint
and the github action is using flake8
. We should drive the linting through tox as well so that it's easier for developers to lint their code without reference to the configuration in the github action.
@mchalek which tool would you prefer we standardize on? I don't really have a preference.
@@ -11,7 +11,7 @@ deps = pytest==3.8.1 | |||
extras = | |||
github | |||
commands = | |||
pytest -vv test | |||
pytest -vv {posargs} |
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.
yup love it 👍
run: | | ||
pytest test | ||
tox --recreate -e py`echo ${{ matrix.python-version }} | tr -d '.'` test |
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.
No one was more surprised this worked than me.
I tried moving the coveralls step into the github action and almost got it working, but it will be a little bit more involved than I thought so I'll defer that to separate work. I'll merge this later today or tomorrow. |
* Use {posargs} in tox.ini to be more flexible * use tox in the github pr test * remove some deps from github action, they are in tox * add comment * Move coveralls out of travis * coverage path * coverage path * Revert "coverage path" This reverts commit 5936537. * Revert "coverage path" This reverts commit 6fbd56c. * Revert "Move coveralls out of travis" This reverts commit 3c48d33.
{posargs}
lets a user easily runtox test/my/single/test.py