Skip to content

gh-128595: Add test class helper to force no terminal colour - #128687

Merged
hugovk merged 10 commits into
python:mainfrom
hugovk:3.14-force_not_colorized_test_class
Jan 13, 2025
Merged

gh-128595: Add test class helper to force no terminal colour#128687
hugovk merged 10 commits into
python:mainfrom
hugovk:3.14-force_not_colorized_test_class

Conversation

@hugovk

@hugovkhugovk commented Jan 9, 2025•
edited
Loading

Copy link
Copy Markdown
Member

@hugovkhugovk added tests Tests in the Lib/test dir skip news needs backport to 3.13 bugs and security fixes labels Jan 9, 2025
@bedevere-appbedevere-appBot mentioned this pull request Jan 9, 2025
Closed
@hugovkhugovk mentioned this pull request Jan 9, 2025
Merged
serhiy-storchaka
serhiy-storchaka reviewed Jan 10, 2025
View reviewed changes

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Comment threadLib/test/test_code_module.py Outdated
erlend-aasland
erlend-aasland reviewed Jan 10, 2025
View reviewed changes
Comment threadLib/test/support/__init__.py Outdated
erlend-aasland
erlend-aasland reviewed Jan 10, 2025
View reviewed changes
Comment threadLib/test/support/__init__.py Outdated
vstinner
vstinner reviewed Jan 10, 2025
View reviewed changes
Comment threadLib/test/support/__init__.py Outdated
vstinner
vstinner reviewed Jan 10, 2025
View reviewed changes

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

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 :-)

Comment threadLib/test/support/__init__.py Outdated
Comment threadLib/test/support/__init__.py Outdated
serhiy-storchaka
serhiy-storchaka approved these changes Jan 10, 2025
View reviewed changes

@serhiy-storchakaserhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Much clearer now!

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

This comment was marked as outdated.

Sign in to view
@miss-islington-app

miss-islington-appBot commented Jan 13, 2025

Copy link
Copy Markdown

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

hugovk commented Jan 13, 2025

Copy link
Copy Markdown
MemberAuthor

Thanks for the reviews!

hugovk added a commit to hugovk/cpython that referenced this pull request Jan 13, 2025
@hugovk@erlend-aasland
[3.13]pythongh-128595: Add test class helper to force no terminal co…
131c9f0
…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

bedevere-appBot commented Jan 13, 2025

Copy link
Copy Markdown

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
@hugovk@erlend-aasland
[3.13]gh-128595: Add test class helper to force no terminal colour (G…
afcf238
…H-128687) (#128778)
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@hugovkhugovk mentioned this pull request Jan 22, 2025
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments
@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes
@erlend-aaslanderlend-aaslanderlend-aasland approved these changes
@iritkatrieliritkatrielAwaiting requested review from iritkatrieliritkatriel is a code owner

Assignees

@hugovkhugovk

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

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