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

Add tests for CliEnv #3204

Merged
merged 3 commits into from Jan 26, 2024
Merged

Conversation

0cjs
Copy link
Contributor

@0cjs 0cjs commented Jan 26, 2024

This is a start on issue #3199. The plan for that involves changing CliEnv; this starts documenting its current behaviour by adding tests for it.

The final reformatting commit may need to be squashed into the previous commit; see that commit message for details.

No news fragment because this isn't a user-visible change. No documentation yet because that will be informed by review of this commit.

  • ran the linter to address style issues (tox -e fix)
  • wrote descriptive pull request text
  • ensured there are test(s) validating the fix
  • added news fragment in docs/changelog folder
  • updated/extended the documentation

@0cjs 0cjs changed the title Dev/cjs/24a25/multiple e opts Add tests for CliEnv Jan 26, 2024
tests/session/test_env_select.py Outdated Show resolved Hide resolved
tests/session/test_env_select.py Outdated Show resolved Hide resolved
tests/session/test_env_select.py Outdated Show resolved Hide resolved
@0cjs 0cjs force-pushed the dev/cjs/24a25/multiple-e-opts branch from eabf094 to 330b4db Compare January 26, 2024 15:26
All but one of the tests in this test module are actually testing
env_select functionality through calling other code that in turn
uses env_select (CliEnv, EnvSelector and register_env_select_flags).

We move that one test to the top of the file, but we do not indicate
the boundary between the groups (or even explain that the grouping
exists) at the request of @gaborbernat.
Some of the behaviour here, such as treatment of zero-length environment
names specified by the user, is not really intentional, but apparently an
artifact of the method used to parse the environment name list supplied by
the user.
I find this makes the tests significantly less readable, so I'm commiting
this separately so we can decide what to do about it. The options are:

1. Squash this with the previous, just using the auto-enforced formatting.
2. Leave these commits as-is, so we at least can go back to the more
   readable original formatting.
3. Do whatever needs to be done to keep the auto-reformatter from touching
   that particular bit of code.

I have a preference for 3, but won't argue any other decision.
@0cjs 0cjs force-pushed the dev/cjs/24a25/multiple-e-opts branch from 330b4db to c53fefb Compare January 26, 2024 15:55
@0cjs 0cjs requested a review from gaborbernat January 26, 2024 15:59
@0cjs
Copy link
Contributor Author

0cjs commented Jan 26, 2024

I'm not clear on why this PR is still saying that there are changes requested. I clicked "See review" and it just takes me to requests that are from before I pushed up the fixes and that are marked outdated and resolved. Can someone explain to me what's happening here?

@gaborbernat
Copy link
Member

I'm not clear on why this PR is still saying that there are changes requested.

Until I approve, will stay in that status. That's how GitHub works.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@gaborbernat gaborbernat merged commit b3eb86a into tox-dev:main Jan 26, 2024
25 checks passed
@0cjs
Copy link
Contributor Author

0cjs commented Jan 26, 2024

Until I approve, will stay in that status. That's how GitHub works.

Ah, ok! I've seen something else similar; GitHub keeps saying things that make me wonder if I missed fixing something. But I'll just stop worrying about it. Please do poke me if you see a PR where I've responded to things but there are still things that I need to fix, thoug.

@0cjs 0cjs deleted the dev/cjs/24a25/multiple-e-opts branch January 26, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants