-
-
Notifications
You must be signed in to change notification settings - Fork 155
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
First implementation of tox_to_nox for tox 4 #687
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.
Hey @frenzymadness thanks for the PR! I agree with the approach of parsing the config from the command line, I think that's realistically the best we can do for tox 4 compatibility.
Just a few comments from my side but this looks like a great start 👍🏻 Will need some tests before it's ready to merge but appreciate the draft to sign off the approach
Right now, all the tests are happy with both tox versions. I skipped just the test of the invalid identified in the env name because it seems that tox 4 does not support it. Another problem is how to include tests with tox 3 into nox's tests. I was thinking about parametrizing the main test session but it seems to be a waste of resources to run all the tests again with tox 3 if the vast majority doesn't depend on tox. My solution is to downgrade tox to version <4 and then run the pytest again in the same session but only the tests of tox_to_nox. |
Just looks like a couple of uncovered sections in tox_to_nox |
What do you mean? I'm looking at both jinja2 templates side-by-side and they both seem to handle the same configuration options from tox.ini. |
Sorry I meant in the tests running in CI, e.g.: https://github.com/wntrblm/nox/actions/runs/4004488540/jobs/6875460666 coverage is saying some things aren't covered 👍🏻 |
The problem reported by coverage was actually caused by two conditions that were never false so I can remove them, nice. All linter should be happy now, at least they are happy on my machine. |
And now we have a different problem with dependencies. argcomplete 2.0.0 requires We can force install tox 4 in a dedicated step in Python 3.7 env but there is a risk that it breaks nox or its dependencies. I'd set a lower coverage limit for Python 3.7 or skip the coverage there entirely. What do you think? |
@FollowTheProcess what do you think about the proposed solutions? |
Force installation of Python 3.7 should fix the problem and it seems that it does not break anything at least for now. |
Hmm, so this would mean anyone installing Nox on 3.7 would never get tox 4 right? So if that's the case I don't think we need to be super strict on getting to 100% because users would never hit that branch either? I'd personally be happy to mark that branch as excluded from coverage when on 3.7 (provided users installing Nox on 3.7 would never get tox 4 because of the dependency conflict above) with a nice explanatory comment etc. I think coverage allows you to do this, not 100% sure |
Let's see what will CI say now. |
The problem here is in the way how the Github Actions are configured. If you run nox test locally, it's green because the coverage report is combined from all runs with all Python versions and therefore the lower overage from Python 3.7 is not a problem. But in GH actions, the coverage report is divided into jobs for each tested Python version which makes the job for Python 3.7 fail on too low coverage. I see two ways forward:
What do you prefer? |
I've implemented what I think is the simplest solution here - no |
But now we are back at the problem with coverage in GH action. If we split the jobs in GH action in the same way we have them in the parametrized sessions in the noxfile, the coverage will fail for all of them because they won't be tested with both tox versions at the same time. I'm going to enable GH actions in my fork and try to implement a single job for the coverage reporting. |
610ed30
to
ea72b2c
Compare
ec6c215
to
f30616c
Compare
09e1556
to
0a31782
Compare
Finally, after many failures, it's done. Now, each session in noxfile has its own job in GH actions. The produced coverage files are uploaded by the individual jobs. When all testing jobs finish, the coverage job starts, downloads the files, combines them and provides the report. Locally, it no longer makes sense to run coverage if you test only one environment, because it cannot be 100 %, so the notification of coverage session has been removed. You can see the last run in my fork here: https://github.com/frenzymadness/nox/actions/runs/4224370541 |
tox 3 is dead. Please go forward and merge this. Thank you. |
@FollowTheProcess I think it might be worth 'resurrecting' this and merging. There are some straightforward conflicts to handle, and this would be an improvement for users. If you are also on board I can get this over the line if needed. Thanks @frenzymadness for picking this up. |
@@ -37,7 +51,7 @@ def wrapjoin(seq: Iterator[Any]) -> str: | |||
|
|||
def fixname(envname: str) -> str: | |||
"""Replace dashes with underscores and check if the result is a valid identifier.""" | |||
envname = envname.replace("-", "_") | |||
envname = envname.replace("-", "_").replace("testenv:", "") |
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'm not sure why this is needed, and if it would have a negative effect on old tox (tox <= 3)
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.
Verified purpose.
Merge resulted in duplicate entries.
Address lint ordering, toml
Address lint.
Yeah, sorry been swamped for a while and not got chance to be around as much as I'd have liked! I'm on board 👍🏻 I'll admit though I've never used tox so not super familiar with the syntax conversions |
@@ -37,7 +51,7 @@ def wrapjoin(seq: Iterator[Any]) -> str: | |||
|
|||
def fixname(envname: str) -> str: | |||
"""Replace dashes with underscores and check if the result is a valid identifier.""" | |||
envname = envname.replace("-", "_") | |||
envname = envname.replace("-", "_").replace("testenv:", "") |
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.
Verified purpose.
## [0.0.20](v0.0.19...v0.0.20) (2024-03-04) ### Chores * **deps:** bump wntrblm/nox from 2023.04.22 to 2024.03.02 ([#57](#57)) ([d177375](d177375)), closes [wntrblm/nox#762](wntrblm/nox#762) [wntrblm/nox#787](wntrblm/nox#787) [wntrblm/nox#730](wntrblm/nox#730) [wntrblm/nox#780](wntrblm/nox#780) [wntrblm/nox#770](wntrblm/nox#770) [wntrblm/nox#707](wntrblm/nox#707) [wntrblm/nox#687](wntrblm/nox#687) [wntrblm/nox#756](wntrblm/nox#756) [wntrblm/nox#652](wntrblm/nox#652) [wntrblm/nox#712](wntrblm/nox#712) [wntrblm/nox#781](wntrblm/nox#781) [wntrblm/nox#786](wntrblm/nox#786) [wntrblm/nox#684](wntrblm/nox#684) [wntrblm/nox#723](wntrblm/nox#723) [wntrblm/nox#725](wntrblm/nox#725) [wntrblm/nox#714](wntrblm/nox#714) [wntrblm/nox#715](wntrblm/nox#715) [wntrblm/nox#696](wntrblm/nox#696) [wntrblm/nox#774](wntrblm/nox#774) [wntrblm/nox#782](wntrblm/nox#782) [wntrblm/nox#722](wntrblm/nox#722) [wntrblm/nox#724](wntrblm/nox#724) [wntrblm/nox#721](wntrblm/nox#721) [wntrblm/nox#744](wntrblm/nox#744) [wntrblm/nox#738](wntrblm/nox#738) [wntrblm/nox#762](wntrblm/nox#762) [wntrblm/nox#787](wntrblm/nox#787) [wntrblm/nox#730](wntrblm/nox#730) [wntrblm/nox#780](wntrblm/nox#780) [wntrblm/nox#770](wntrblm/nox#770) [wntrblm/nox#707](wntrblm/nox#707) [wntrblm/nox#687](wntrblm/nox#687) [wntrblm/nox#756](wntrblm/nox#756) [wntrblm/nox#652](wntrblm/nox#652) [wntrblm/nox#712](wntrblm/nox#712) [wntrblm/nox#781](wntrblm/nox#781) [wntrblm/nox#786](wntrblm/nox#786) [wntrblm/nox#684](wntrblm/nox#684) [wntrblm/nox#723](wntrblm/nox#723) [wntrblm/nox#725](wntrblm/nox#725) [wntrblm/nox#714](wntrblm/nox#714) [wntrblm/nox#715](wntrblm/nox#715) [wntrblm/nox#696](wntrblm/nox#696) [wntrblm/nox#774](wntrblm/nox#774) [wntrblm/nox#782](wntrblm/nox#782) [wntrblm/nox#722](wntrblm/nox#722) [wntrblm/nox#724](wntrblm/nox#724) [wntrblm/nox#721](wntrblm/nox#721) [wntrblm/nox#744](wntrblm/nox#744) [#789](https://github.com/msclock/sphinx-deployment/issues/789) [#787](https://github.com/msclock/sphinx-deployment/issues/787) [#786](https://github.com/msclock/sphinx-deployment/issues/786) [#781](https://github.com/msclock/sphinx-deployment/issues/781) [#784](https://github.com/msclock/sphinx-deployment/issues/784) [#780](https://github.com/msclock/sphinx-deployment/issues/780) [#730](https://github.com/msclock/sphinx-deployment/issues/730) [#782](https://github.com/msclock/sphinx-deployment/issues/782) [#783](https://github.com/msclock/sphinx-deployment/issues/783) [#762](https://github.com/msclock/sphinx-deployment/issues/762) ### CI * set proper permission for preview job ([#56](#56)) ([d65e8a1](d65e8a1))
Fixes: #673
It seems to me that the easiest way forward is to call
tox config
, parse the output and then preprocess it into a dictionary so it can be easily used in the jinja template.What do you think?
From this tox.ini:
it now generates this noxfile.py
I guess that:
Correct?