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.

Sign up for GitHub

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

Jump to bottom

Fix tests when NO_COLOR is set in the calling environment #12415

Merged
merged 3 commits into from
Mar 3, 2024
Merged

Fix tests when NO_COLOR is set in the calling environment #12415

merged 3 commits into from
Mar 3, 2024

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Nov 25, 2023

Add FORCE_COLOR and NO_COLOR variables to the isolate() fixture to ensure that these two variables do not affect internal test output. This fixes the following two test failures when pytest is called with NO_COLOR set:

FAILED tests/unit/test_exceptions.py::TestDiagnosticPipErrorPresentation_ASCII::test_complete_color - AssertionError: assert '\x1b[1merror...ing harder.\n' == '\x1b[1;31mer...ing harder.\n'
FAILED tests/unit/test_exceptions.py::TestDiagnosticPipErrorPresentation_Unicode::test_complete_color - AssertionError: assert '\x1b[1merror...ing harder.\n' == '\x1b[1;31mer...ing harder.\n'

webknjaz
webknjaz reviewed Dec 23, 2023
View reviewed changes
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Cool! This is something I was going to suggest at some point.

To make this change truly tested in pip's own CI, how about including a commit reverting 1b33f4b?

@webknjaz
Copy link
Member

(context comment)

I recently discovered that there are bugs related with FORCE_COLOR and NO_COLOR, coming from different places in pip and the underlying Rich library: #12405.

As of today, using both FORCE_COLOR and NO_COLOR forces pip to use ANSI-escapes that make parts of output bold, while keeping it monochrome. And this breaks tools that read piped output.
I even started discussions on the trackers of the corresponding standards: https://github.com/jcs/no_color/issues/267 / donatj/force-color.org#8. Rich hasn't recognized this behavior as a bug, so far.

mgorny added 2 commits February 4, 2024 16:49
@mgorny
Fix tests when NO_COLOR is set in the calling environment
49d233a 
Add `FORCE_COLOR` and `NO_COLOR` variables to the `isolate()` fixture
to ensure that these two variables do not affect internal test output.
This fixes the following two test failures when pytest is called with
`NO_COLOR` set:

```
FAILED tests/unit/test_exceptions.py::TestDiagnosticPipErrorPresentation_ASCII::test_complete_color - AssertionError: assert '\x1b[1merror...ing harder.\n' == '\x1b[1;31mer...ing harder.\n'
FAILED tests/unit/test_exceptions.py::TestDiagnosticPipErrorPresentation_Unicode::test_complete_color - AssertionError: assert '\x1b[1merror...ing harder.\n' == '\x1b[1;31mer...ing harder.\n'
```
@mgorny
Revert "Stop forcing color in CI"
f84d782 
This reverts commit 1b33f4b.  Now
the test suite strips `FORCE_COLOR` internally, so it should be fine
to run with colors enabled for nox.
@mgorny
Copy link
Contributor Author

mgorny commented Feb 4, 2024

To make this change truly tested in pip's own CI, how about including a commit reverting 1b33f4b?

I'm sorry about forgetting about this pull request. I've added the revert now and rebased.

@mgorny
Copy link
Contributor Author

mgorny commented Feb 4, 2024

Ok, so the problem with tests/functional/test_inspect.py::test_inspect_basic is that simple_script fixture is initiated "outside" isolate fixture, and effectively copies os.environ prior to the variables being stripped. How would you prefer that being fixed?

The options I can think of are:

  1. Reducing simple_script to function-scope fixture (it's used by one test anyway).
  2. Modifying PipTestEnvironment to unconditionally set NO_COLOR for executed scripts, or to unset FORCE_COLOR.

@webknjaz
Copy link
Member

webknjaz commented Feb 4, 2024

(1) sounds reasonable, if you ask me.

@mgorny
Lower the scope of simple_script fixture to apply isolate to it
1836651 
Lower the scope of `simple_script` fixture used in `test_inspect`
from `session` to (implicit) `function`, to make it match the `isolate`
fixture and therefore make the fixture run in isolation.
Per the documentation [1], pytest initializes higher-scope fixtures
first, and lower-scope fixtures cannot be used in them.  Since within
the same scope, autouse fixtures are always loaded before regular
fixtures, no further adjustment is necessary.  This should have no ill
side effects, as the fixture is used by a single test only.

[1] https://docs.pytest.org/en/stable/reference/fixtures.html#fixture-instantiation-order
@mgorny
Copy link
Contributor Author

mgorny commented Feb 5, 2024

Ok, I think it's good now.

@webknjaz
Copy link
Member

webknjaz commented Feb 6, 2024

@pradyunsg I think this should be mergeable

@webknjaz webknjaz requested a review from pradyunsg March 3, 2024 18:51
@pradyunsg pradyunsg merged commit 13139cb into pypa:main Mar 3, 2024
26 checks passed
@mgorny mgorny deleted the no-color branch March 4, 2024 05:34
@mgorny
Copy link
Contributor Author

mgorny commented Mar 4, 2024

Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@webknjaz webknjaz webknjaz left review comments

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
bot:chronographer:provided
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mgorny @webknjaz @pradyunsg