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

CliEnv('py37, py36') != CliEnv('py37,py36') #3207

Closed
0cjs opened this issue Jan 27, 2024 · 2 comments
Closed

CliEnv('py37, py36') != CliEnv('py37,py36') #3207

0cjs opened this issue Jan 27, 2024 · 2 comments

Comments

@0cjs
Copy link
Contributor

0cjs commented Jan 27, 2024

Issue

Looks like there might be more undocumented behaviour going on in CliEnv. In one of my checkouts I am regularly getting the following failure:

_____________________________________________ test_ini_exhaustive_parallel_values ______________________________________________
tests/config/cli/test_cli_ini.py:127: in test_ini_exhaustive_parallel_values
    assert vars(options.parsed) == {
E   AssertionError: assert {'colored': '...tualenv', ...} == {'colored': '...tualenv', ...}
E     Omitting 30 identical items, use -vv to show
E     Differing items:
E     {'env': CliEnv('py37, py36')} != {'env': CliEnv('py37,py36')}
E     Use -v to get more diff
        core_handlers = {'c': <function show_config at 0x7f141ee70220>, 'config': <function show_config at 0x7f141ee70220>, 'd': <function devenv at 0x7f141ee7b420>, 'de': <function depends at 0x7f141ee7b100>, ...}
        options    = Options(parsed=Parsed(colored='yes', verbose=5, quiet=1, exit_and_dump_after=0, config_file=None, work_dir=None, root_...e': <function legacy at 0x7f141ee700e0>}, log_handler=<ToxHandler <_io.FileIO name=6 mode='rb+' closefd=True> (DEBUG)>)
----------------------------------------------------- Captured stdout call -----------------------------------------------------
ROOT: 1125 D setup logging to DEBUG on pid 294190 [tox/report.py:221]

I can't seem to reproduce it in other checkouts on the same machine using the same version of Python. I do not know why.

Nor do I understand how we ended up with an environment name that apparently starts with a space: py36.

@0cjs
Copy link
Contributor Author

0cjs commented Jan 27, 2024

It turned out to be a change in my tree to CliEnv that triggered this; I got distracted by a completely unrelated problem in that checkout and had forgotten about it. Apparently even with the additional new tests in session/test_env_select.py, that still doesn't give full coverage for some behaviour that's being used by test_ini_exhaustive_parallel_values. There's something going on in the 200+ lines of code used by StrConvert().to(value, of_type=List[str], factory=None) that's not clearly defined. Sigh.

Maybe someone who knows what it is can add a test for it in the proper place.

0cjs added a commit to 0cjs/tox that referenced this issue Jan 27, 2024
It turns out that spaces on either side of environment names in a
comma-separated list are considered not part of the environment
name and removed. This was neither documented nor tested
directly; that's now fixed. (It was and is still also tested in
`config.cli.test_cli_ini.test_ini_exhaustive_parallel_values` as
well.)

The discovery of this was documented in issue tox-dev#3207.
gaborbernat pushed a commit that referenced this issue Jan 27, 2024
It turns out that spaces on either side of environment names in a
comma-separated list are considered not part of the environment
name and removed. This was neither documented nor tested
directly; that's now fixed. (It was and is still also tested in
`config.cli.test_cli_ini.test_ini_exhaustive_parallel_values` as
well.)

The discovery of this was documented in issue #3207.
0cjs added a commit to 0cjs/tox that referenced this issue Jan 28, 2024
No code changes; just a "move text" refactoring.

This makes the code quicker to understand by moving the exhaustive_ini
fixture next to the two tests using it, rather than having it separated
from them by several other test functions that don't use it. (This
separation is why I didn't immediately see an important point in that
in issue tox-dev#3207.)

There's also a slight re-ordering of tests; they're still in an order
that makes sense for testing the functionality from the smallest to
the largest bits.
@0cjs
Copy link
Contributor Author

0cjs commented Jan 28, 2024

Ok, I think that #3208 and #3209 essentially fix this issue in the narrower sense. (What's going into that function that causes the extra space is clear once you look at that fixture; it's just that it was way far away from where it was being used, so it wasn't obvious unless you knew to track it down.)

In the wider sense, there are probably other things like this, but I don't think it's worth keeping an issue open for them; they should just be fixed as we come across them.

gaborbernat pushed a commit that referenced this issue Jan 28, 2024
…3209)

No code changes; just a "move text" refactoring.

This makes the code quicker to understand by moving the exhaustive_ini
fixture next to the two tests using it, rather than having it separated
from them by several other test functions that don't use it. (This
separation is why I didn't immediately see an important point in that
in issue #3207.)

There's also a slight re-ordering of tests; they're still in an order
that makes sense for testing the functionality from the smallest to
the largest bits.
@0cjs 0cjs closed this as completed Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant