So I trial-ran tox==4.0.0a6 on the latest master branch of aiosmtpd.
With tox3, I got the following summary conclusion:
qa: commands succeeded
SKIPPED: static: platform mismatch ('win32' does not match '^(?!win32)(?!cygwin)')
docs: commands succeeded
On tox4, the "static" testenv is just missing:
qa: OK (45.77=setup[2.86]+cmd[0.16,3.84,37.09,1.81] seconds)
docs: OK (9.81=setup[2.89]+cmd[0.17,3.73,1.25,1.77] seconds)
Scrolling up in Terminal, tox4 never mentioned anything about the missing "static" testenv:
qa: OK ✔ in 45.77 seconds
docs: install_package> python -I -m pip install --no-deps --force-reinstall --no-build-isolation -e C:\Repos\aiosmtpd
docs: commands[0]> python housekeep.py prep
It would be nice if this tox3 behavior (i.e., reporting on skipped testenvs, and why) is kept in tox4.
pepoluan
All 8 comments
Just to add: If tox4 cannot find a suitable Python interpreter, it _does_ report the skippage.
For example:
py36-diffcov: OK ✔ in 27.4 seconds
py37-nocov: skipped environment because could not find python interpreter with spec(s): py37
py37-nocov: SKIP ⚠ in 1.23 seconds
py37-cov: skipped environment because could not find python interpreter with spec(s): py37
py37-cov: SKIP ⚠ in 0.08 seconds
py37-diffcov: skipped environment because could not find python interpreter with spec(s): py37
py37-diffcov: SKIP ⚠ in 0.08 seconds
py38-nocov: install_deps> python -I -m pip install bandit colorama coverage-conditional-plugin 'coverage[toml]' diff_cover packaging pytest-cov pytest-mock pytest-print pytest-profiling pytest-sugar 'pytest>=6.0'
The summary on conclusion looks like this:
qa: OK (90.41=setup[24.50]+cmd[0.19,4.53,55.79,5.40] seconds)
docs: OK (41.70=setup[33.23]+cmd[0.20,4.14,2.86,1.27] seconds)
py36-nocov: OK (98.52=setup[32.03]+cmd[0.22,4.57,61.48,0.23] seconds)
py36-cov: OK (108.70=setup[24.96]+cmd[0.22,4.42,78.81,0.29] seconds)
py36-diffcov: OK (27.40=setup[25.30]+cmd[0.23,0.87,0.82,0.18] seconds)
py37-nocov: SKIP (1.23 seconds)
py37-cov: SKIP (0.08 seconds)
py37-diffcov: SKIP (0.08 seconds)
py38-nocov: OK (81.97=setup[22.87]+cmd[0.19,3.54,55.20,0.17] seconds)
py38-cov: OK (105.80=setup[23.02]+cmd[0.19,3.24,79.13,0.22] seconds)
py38-diffcov: OK (24.84=setup[22.91]+cmd[0.18,0.85,0.74,0.16] seconds)
py39-nocov: SKIP (0.08 seconds)
py39-cov: SKIP (0.08 seconds)
py39-diffcov: SKIP (0.09 seconds)
pypy3-nocov: SKIP (0.57 seconds)
pypy3-cov: SKIP (0.06 seconds)
pypy3-diffcov: SKIP (0.07 seconds)
congratulations :) (581.87 seconds)
I just had a quick look, and the reason for the different behavior is the following..
There is a very early check for the platform, and if it does not match, the env is not included in the to_run_list.
On the contrary, other envs are put in the above mentioned list, and then later in _evaluate a Skip exception is thrown and caught when the interpreter is missing. This is also the spot where the skip message gets logged.
@gaborbernat Do you think it's ok to postpone the platform check and do it in the _evaluate method?
Maybe a guard clause just before the try block, or inside the ensure_setup method? (given all info is available there, have to check that next time I have some spare minutes)
jugmac00
on 7 Mar 2021
Indeed, we need to translate the platform check to mean skip setup+run rather than not including it within the run list.
gaborbernat
on 7 Mar 2021
I'd like to work on this issue, if you are ok that I cannot promise a date when I find enough time. I try to do it next week.
jugmac00
on 7 Mar 2021
Feel free to ping me if you need it run on Windows, Cygwin, WSL, and FreeBSD.
pepoluan
on 7 Mar 2021
Meanwhile... got it working on a local branch.
❯ tox4 -e plat
plat: skipped environment because platform linux does not match win32
plat: SKIP (0.00 seconds)
congratulations :) (0.02 seconds)
❯ tox4 -e py33
py33: skipped environment because could not find python interpreter with spec(s): py33
py33: SKIP (0.01 seconds)
congratulations :) (0.04 seconds)
I am still not entirely sure where the best place is for the platform check. Somewhere in or near ...
For now, I removed the ToxEnv.active property, as we both do not need such an early check, and we need the result of the platform check to create a proper skip message.
As the active check then is no longer necessary, I tried to remove the for loop..
... but this broke a couple of tests, as state.tox_env(env) has some side effects. So for the moment I kept the loop - just for the side effects :-/
I played around putting the platform check either in _evaluate directly or in ensure_setup...
It would look like...
def ensure_setup(self, recreate: bool = False) -> None:
platform_str: str = self.conf["platform"]
if platform_str:
match = re.fullmatch(platform_str, self.runs_on_platform)
if match is None:
raise Skip("platform %s does not match %s" % (self.runs_on_platform, platform_str))
if self.setup_done is True:
return
...
I am not entirely sure whether it makes sense to do it here, e.g. is it ok to raise a Skip from this code?
The only other place I could imagine would be at the top of _evaluate like this...
def _evaluate(tox_env: RunToxEnv, recreate: bool, no_test: bool) -> Tuple[bool, int, List[Outcome]]:
skipped = False
code: int = 0
outcomes: List[Outcome] = []
platform_str: str = tox_env.conf["platform"]
if platform_str:
match = re.fullmatch(platform_str, tox_env.runs_on_platform)
if match is None:
LOGGER.warning(
"skipped environment because %s platform does not match %s" % (
tox_env.runs_on_platform, platform_str))
skipped = True
return skipped, code, outcomes
try:
try:
tox_env.ensure_setup(recreate=recreate)
code, outcomes = run_commands(tox_env, no_test)
except Skip as exception:
LOGGER.warning("skipped environment because %s", exception)
skipped = True
@gaborbernat Any opinion on this already?
Next, I'd rewrite the platform test and create a wip pr, so it should be easier to review.
jugmac00
on 11 Mar 2021
I created a WIP PR, #1970
jugmac00
on 11 Mar 2021
I made comments on the PR with directions on the best steps 👍🏻
gaborbernat
on 12 Mar 2021
Related issues
gaborbernat
·
4Comments
obestwalter
·
5Comments
pytoxbot
·
5Comments
moggers87
·
3Comments
lopuhin
·
3Comments
Most helpful comment
Indeed, we need to translate the platform check to mean skip setup+run rather than not including it within the run list.