Skip to content
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

Merged
merged 7 commits into from
Feb 20, 2024

Conversation

frenzymadness
Copy link
Contributor

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:

[tox]
envlist = black,mypy,py{36,37,38,39,310,311,312}
isolated_build = True

[testenv]
commands =
    python -m platform
    pytest -v {posargs} test.py
deps =
    pytest

[testenv:black]
description="better than blue"
skip_install=true
deps=black
commands=black --check --diff --extend-exclude=".tox|test" ./

[testenv:mypy]
use_develop=true
deps=mypy
commands=mypy --strict -p marshalparser

it now generates this noxfile.py

import nox


@nox.session()
def DEFAULT(session):
    """"""
    session.install('.')

@nox.session(python='/home/lbalhar/.virtualenvs/nox/bin/python')
def testenv_black(session):
    """"better than blue""""
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('black')
    session.run('black --check --diff '--extend-exclude=.tox|test' ./')

@nox.session(python='/home/lbalhar/.virtualenvs/nox/bin/python')
def testenv_mypy(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('mypy')
    session.install('-e', '.')
    session.run('mypy --strict -p marshalparser')

@nox.session(python='py310')
def testenv_py310(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

@nox.session(python='py311')
def testenv_py311(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

@nox.session(python='py312')
def testenv_py312(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

@nox.session(python='py36')
def testenv_py36(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

@nox.session(python='py37')
def testenv_py37(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

@nox.session(python='py38')
def testenv_py38(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

@nox.session(python='py39')
def testenv_py39(session):
    session.env['PIP_DISABLE_PIP_VERSION_CHECK'] = '1'
    session.env['PYTHONIOENCODING'] = 'utf-8'
    session.install('pytest')
    session.install('.')
    session.run('python -m platform')
    session.run('pytest -v test.py')

I guess that:

  • the default env variables should be skipped.
  • the default environment should be skipped.

Correct?

Copy link
Collaborator

@FollowTheProcess FollowTheProcess left a 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

nox/tox_to_nox.py Outdated Show resolved Hide resolved
nox/tox_to_nox.py Outdated Show resolved Hide resolved
nox/tox_to_nox.py Outdated Show resolved Hide resolved
@frenzymadness
Copy link
Contributor Author

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.

@FollowTheProcess
Copy link
Collaborator

Just looks like a couple of uncovered sections in tox_to_nox

@frenzymadness
Copy link
Contributor Author

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.

@FollowTheProcess
Copy link
Collaborator

FollowTheProcess commented Jan 25, 2023

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 👍🏻

@frenzymadness
Copy link
Contributor Author

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.

@frenzymadness
Copy link
Contributor Author

And now we have a different problem with dependencies. argcomplete 2.0.0 requires importlib-metadata<5,>=0.23; python_version == "3.7" but the latest tox depends on importlib-metadata>=6; python_version < "3.8" which means that they cannot be installed together in an environment with Python 3.7 and therefore both pytest runs tests with tox 3 and hence the coverage is less than 100 %.

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?

@frenzymadness
Copy link
Contributor Author

@FollowTheProcess what do you think about the proposed solutions?

@frenzymadness frenzymadness marked this pull request as ready for review February 10, 2023 08:30
@frenzymadness
Copy link
Contributor Author

Force installation of Python 3.7 should fix the problem and it seems that it does not break anything at least for now.

@FollowTheProcess
Copy link
Collaborator

FollowTheProcess commented Feb 10, 2023

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

@frenzymadness
Copy link
Contributor Author

Let's see what will CI say now.

@frenzymadness
Copy link
Contributor Author

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:

  1. Do not run coverage for Python 3.7 on GH actions.
  2. Reconfigure GH actions to collect coverage for individual jobs and run coverage report for the collected data in a separate job.

What do you prefer?

@frenzymadness
Copy link
Contributor Author

I've implemented what I think is the simplest solution here - no session.notify("cover") for Python 3.7. That way, the coverage report is run only if called explicitly or if tests run for more than this one environment which should also fix the coverage so it should be green all the time.

noxfile.py Outdated Show resolved Hide resolved
@frenzymadness
Copy link
Contributor Author

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.

@frenzymadness frenzymadness marked this pull request as draft February 20, 2023 07:56
@frenzymadness frenzymadness force-pushed the tox4 branch 9 times, most recently from 610ed30 to ea72b2c Compare February 20, 2023 10:42
@frenzymadness frenzymadness force-pushed the tox4 branch 8 times, most recently from ec6c215 to f30616c Compare February 20, 2023 13:23
@frenzymadness frenzymadness force-pushed the tox4 branch 2 times, most recently from 09e1556 to 0a31782 Compare February 20, 2023 14:46
@frenzymadness frenzymadness marked this pull request as ready for review February 20, 2023 14:49
@frenzymadness
Copy link
Contributor Author

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

@mtelka
Copy link

mtelka commented Apr 24, 2023

tox 3 is dead. Please go forward and merge this. Thank you.

@crwilcox
Copy link
Collaborator

crwilcox commented Feb 16, 2024

@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:", "")
Copy link
Collaborator

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified purpose.

@FollowTheProcess
Copy link
Collaborator

FollowTheProcess commented Feb 19, 2024

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:", "")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified purpose.

@crwilcox crwilcox merged commit ef54d13 into wntrblm:main Feb 20, 2024
40 checks passed
github-actions bot pushed a commit to msclock/sphinx-deployment that referenced this pull request Mar 4, 2024
## [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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility with tox 4
4 participants