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

Partial match on testenv results in use of base testenv #3219

Open
stephenfin opened this issue Feb 15, 2024 · 4 comments
Open

Partial match on testenv results in use of base testenv #3219

stephenfin opened this issue Feb 15, 2024 · 4 comments
Labels
help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.

Comments

@stephenfin
Copy link
Contributor

Issue

Consider the following toy tox.ini:

[testenv]
skip_install = true
allowlist_externals =
  echo
commands =
  echo "base testenv"

[testenv:functional{-py310}]
commands =
  echo "functional testenv"

This should result in a single testenv functional-py310 (there's only one factor) plus the default pyNN testenvs. Attempts to use a testenv with the functional factor but a different pyNN factor (e.g. tox -e functional-py312) or without a pyNN factor (e.g. tox -e functional) should fail since it doesn't match an env making it no different to e.g. tox -e doesnotexist.

In tox 3, this was the case:

❯ virtualenv .venv --python=python3.8
❯ source .venv/bin/activate
❯ pip install 'tox<3'

❯ tox -e functional-py310 -q
functional testenv
__________________________________________________________________ summary __________________________________________________________________
  functional-py310: commands succeeded
  congratulations :)

❯ tox -e functional-py312 -q
ERROR: unknown environment 'functional-py312

❯ tox -e functional -q
ERROR: unknown environment 'functional'

❯ tox -e doesnotexist -q
ERROR: unknown environment 'doesnotexist'

Once we switch to tox 4, this is no longer the case:

❯ tox -e functional-py310 -q
functional testenv
  functional-py310: OK (0.22 seconds)
  congratulations :) (0.29 seconds)

❯ tox -e functional-py312 -q
base testenv
  functional-py312: OK (0.13 seconds)
  congratulations :) (0.18 seconds)

❯ tox -e functional -q
base testenv
  functional: OK (0.14 seconds)
  congratulations :) (0.20 seconds)

Totally undefined envs fail as expected (though with an uglier error than previously):

❯ tox -e doesnotexist -q
ROOT: HandledError| provided environments not found in configuration file:
doesnotexist

This can result in us running a wholly different set of tests/commands if e.g. a user uses a pyNN factor that is not defined inline or does not use a factor (with the idea being you'd use the Python version tox is installed under).

Environment

Using latest tox 3 and 4 available from pip (currently 3.28.0 and 4.12.1, respectively).

Minimal example

See above.

stephenfin added a commit to stephenfin/tox that referenced this issue Feb 15, 2024
To determine whether we have been given a valid *env*, we compare the
*factors* of the *env* to our known set of *factors*. If a testenv
contains a series of valid factors, we assume the testenv is valid. This
is an incorrect assumption: a testenv is only valid if it defined in the
configuration file *or* is one of the "special" auto-generated testenvs
like e.g. 'py310'. Correct this mistake.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: tox-dev#3219
stephenfin added a commit to stephenfin/tox that referenced this issue Feb 15, 2024
To determine whether we have been given a valid *env*, we compare the
*factors* of the *env* to our known set of *factors*. If a testenv
contains a series of valid factors, we assume the testenv is valid. This
is an incorrect assumption: a testenv is only valid if it defined in the
configuration file *or* is one of the "special" auto-generated testenvs
like e.g. 'py310'. Correct this mistake.

While we're here, we make use of verbose mode to explain our regex for
matching "magic" factors better.

Signed-off-by: Stephen Finucane <[email protected]>
Closes: tox-dev#3219
@stephenfin
Copy link
Contributor Author

stephenfin commented Feb 15, 2024

I'd like to know what is the expected behaviours in the below:

[testenv]
skip_install = true
allowlist_externals =
  echo
commands =
  foo: echo "foo"
  bar: echo "bar"
  baz: echo "baz"
  fizz-buzz: echo "fizz-buzz"

[testenv:foo-bar]
commands =
  echo "foo-bar"

[testenv:fizz-buzz]
commands = 
  echo "basic sparkle"

[testenv:fizz-buzz-bang]
commands = 
  echo "extra sparkle"
tox -e foo
tox -e bar
tox -e foo-bar
tox -e bar-foo
tox -e foo-bar-baz
tox -e fizz
tox -e fizz-buzz
tox -e fizz-buzz-bang
tox -e fizz-bang-buzz

@gaborbernat
Copy link
Member

tox -e foo-bar
foo-bar
tox -e foo
foo
tox -e bar
bar
tox -e bar-foo
# undefined, but probably bar
tox -e fizz
# empty output, matches nothing
tox -e fizz-buzz
basic sparkle
tox -e fizz-buzz-bang
extra sparkle
tox -e fizz-bang-buzz
# undefined, I'd expect empty output, matches nothing, but can see a case for fizz-buz

@stephenfin
Copy link
Contributor Author

stephenfin commented Feb 15, 2024

Okay, that broadly aligns with what I'd expect. So I think the issue here is that we're equating a factor to be both a thing defined inside e.g. [testenv] commands or as any piece of an testenv, when in fact factors should really be only the former plus the dynamic pyNM stuff. In the above tox.ini, I think we have the following static testenvs:

  • foo-bar
  • fizz-buzz
  • fizz-buzz-bang

We then have the following factors:

  • foo
  • bar
  • baz
  • fizz-buzz (question: should this be illegal? In my experience, factors must usually consist of alphnumeric characters and periods)
  • the dynamic Python factors like pyN, pyNM, pyN.M, pyN.M-alpha etc.

This results in a large matrix of dynamic testenvs:

  • foo
  • foo-bar
  • baz-foo-bar
  • py310-foo
  • py3.10-rc1-foo-bar-baz
  • ...

So the crucial bit here that we probably don't consider bang to be a factor since it's only defined as part of a static testenv (the fizz-buzz-bang testenv). We also don't consider fizz or buzz to be factors since fizz-buzz is the factor. Correct? If so, we (read: I, in a future patch, time permitting) probably want to start disambiguating between factors and testenvs, allowing the latter to be used for generating dynamic testenvs but not the former (either in whole or in part by splitting on -)?

@stephenfin
Copy link
Contributor Author

stephenfin commented Feb 15, 2024

So the crucial bit here that we probably don't consider bang to be a factor since it's only defined as part of a static testenv (the fizz-buzz-bang testenv). We also don't consider fizz or buzz to be factors since fizz-buzz is the factor. Correct? If so, we (read: I, in a future patch, time permitting) probably want to start disambiguating between factors and testenvs, allowing the latter to be used for generating dynamic testenvs but not the former (either in whole or in part by splitting on -)?

To be clear, this is only for determining whether a provided testenv in valid or not. Obviously a user could do the following:

[testenv:fizz-buzz{,-bang}]
commands = 
  !bang: echo "basic sparkle"
  bang: echo "extra sparkle"

So bang is a factor in that context. It's just not a factor that can be considered for generating dynamic testenvs. A user still shouldn't be able to run tox -e bang: they could only run tox -e fizz-buzz-bang (or something else entirely).

openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Feb 16, 2024
* Update openstacksdk from branch 'master'
  to 568921ce5b50275061c5dd0ba7180842f3c157a7
  - tox: Correct functional test factors
    
    We are running functional tests in zuul without a 'pyNN' factor (e.g.
    'tox -e functional'). For this to work, we need to allow an empty
    factor, i.e. we want:
    
      [testenv:functional{,-py310}]
    
    rather than:
    
      [testenv:functional{-py310}]
    
    (note the missing comma)
    
    Unfortunately we missed this as tox 4 has a currently unaddressed
    regression [1] that results in it running the base testenv in the case
    there is only a partial factor match. That needs to be fixed for avoid
    this biting us again the future, but we can at least fix it for now.
    
    [1] tox-dev/tox#3219
    
    Change-Id: Ib9f65a4523222f1224d51534c5061f90501b59d3
    Signed-off-by: Stephen Finucane <[email protected]>
openstack-mirroring pushed a commit to openstack/openstacksdk that referenced this issue Feb 16, 2024
We are running functional tests in zuul without a 'pyNN' factor (e.g.
'tox -e functional'). For this to work, we need to allow an empty
factor, i.e. we want:

  [testenv:functional{,-py310}]

rather than:

  [testenv:functional{-py310}]

(note the missing comma)

Unfortunately we missed this as tox 4 has a currently unaddressed
regression [1] that results in it running the base testenv in the case
there is only a partial factor match. That needs to be fixed for avoid
this biting us again the future, but we can at least fix it for now.

[1] tox-dev/tox#3219

Change-Id: Ib9f65a4523222f1224d51534c5061f90501b59d3
Signed-off-by: Stephen Finucane <[email protected]>
@gaborbernat gaborbernat added the help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted. label Mar 5, 2024
openstack-mirroring pushed a commit to openstack/openstacksdk that referenced this issue Oct 3, 2024
Without this comma, tox runs the standard testenv instead of this one.
This is due to how tox calculates factors (parts of a testenv name) and
checks for their existence [1].

[1] tox-dev/tox#3219

Change-Id: I3fc33a26b0d7f057d7bd6a094fc2ceeaa88f177b
Signed-off-by: Stephen Finucane <[email protected]>
openstack-mirroring pushed a commit to openstack/openstack that referenced this issue Oct 3, 2024
* Update openstacksdk from branch 'master'
  to c2470a21de72969812642ed4acf1c7c49b45dd6e
  - tox: Fix functional tests
    
    Without this comma, tox runs the standard testenv instead of this one.
    This is due to how tox calculates factors (parts of a testenv name) and
    checks for their existence [1].
    
    [1] tox-dev/tox#3219
    
    Change-Id: I3fc33a26b0d7f057d7bd6a094fc2ceeaa88f177b
    Signed-off-by: Stephen Finucane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help:wanted Issues that have been acknowledged, a solution determined and a PR might likely be accepted.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants