Skip to content

Conversation

@hugovk
Copy link
Member

@hugovkhugovk commented Jan 9, 2025

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

  • You can disable colorizing for the whole class in setUpClass() instead of setUp().
  • Would not it be simpler to implement the core functionality as a generator-based context manager? You can use enterContext() or enterClassContext() with it.
  • You can use test.support.os_helper.EnvironmentVarGuard() to restore the environment and test.support.swap_attr() to restore _colorize.can_colorize.

You can also implement this as a mixin instead of patching a method (use a super() call in an overridden method). I do not say that it would be better, but it is just an alternative which you could have overlooked.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

Oh nice, the new code is more readable, I prefer context managers :-)

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. Much clearer now!

@hugovkhugovk enabled auto-merge (squash) January 13, 2025 11:01
@hugovkhugovk merged commit afb9dc8 into python:mainJan 13, 2025
40 checks passed
@hugovkhugovk deleted the 3.14-force_not_colorized_test_class branch January 13, 2025 11:05
@miss-islington-app

This comment was marked as outdated.

@miss-islington-app
Copy link

Sorry, @hugovk, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker afb9dc887c6e8ae17b6a54c6124399e8bdc82253 3.13 

@hugovk
Copy link
MemberAuthor

Thanks for the reviews!

hugovk added a commit to hugovk/cpython that referenced this pull request Jan 13, 2025
…lour (pythonGH-128687) (cherry picked from commit afb9dc8) Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@bedevere-app
Copy link

GH-128778 is a backport of this pull request to the 3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13 bugs and security fixes label Jan 13, 2025
hugovk added a commit that referenced this pull request Jan 13, 2025
…H-128687) (#128778) Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@hugovk@vstinner@serhiy-storchaka@erlend-aasland