gh-60283: Check for redefined test names in CI by hugovk · Pull Request #109161 · python/cpython · GitHub

Fixes python/core-workflow#505.
Fixes #60283.

Add Ruff to the CI (via pre-commit) to check Lib/test/ for F811 "Redefinition of unused name". Ruff (47s) is faster than Flake8 (1m39s).

This will help find copy/pasted test_* method names, which results in one of them not being run.

It also finds redefined variables, which ISTR people were in favour of.

There are some files which fail to lint or parse, and have been excluded. Some of these are intentional and should never be checked (e.g. Lib/test/badsyntax_pep3120.py), some are due to new 3.12+ syntax not yet supported by the Ruff linter (e.g. Lib/test/test_type_aliases.py). I think we can be pragmatic and exclude any that add new syntax until such a time the linter supports the new version. (This may also help drive adoption of new syntax by the linter.)

This currently fails due to duplicated tests in test_monitoring, which will be fixed in #109139 and has been left as a demonstration in the meantime.

All other test_* duplicates have been fixed (see links in python/core-workflow#505).

Some files have duplicate variables, I've excluded those and they can be fixed and re-included in the future.

Issue: list duplicate test names with patchcheck #60283

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

LGTM, thanks! I guess we just have to wait for #109139 to be merged first?

Should we have changes to .pre-commit-config.yaml and .ruff.toml NOT trigger the main test build?

That would mean adding to this grep pattern:

cpython/.github/workflows/build.yml

Line 66 in 2dd6a86

    
                       git diff --name-only origin/$GITHUB_BASE_REF.. | grep -qvE '(\.rst$|^Doc|^Misc)' && echo "run_tests=true" >> $GITHUB_OUTPUT || true

Should we have changes to .pre-commit-config.yaml and .ruff.toml NOT trigger the main test build?

That sounds like a good idea to me!

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

🥳

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

Thanks @hugovk for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

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

(cherry picked from commit 3cb9a8e) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

GH-109365 is a backport of this pull request to the 3.12 branch.

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> (cherry picked from commit 3cb9a8e)

GH-109366 is a backport of this pull request to the 3.11 branch.

For the backport PRs, I updated the exclude lists, as they were slightly different.

And interestingly, we don't need to explicitly exclude the "Failed to lint/parse" files because they're printed to screen but don't fail the command.

For example, these all have return code = 0 = success:

❯ pre-commit run --all-files ruff
Run Ruff on Lib/test/....................................................Passed

❯ pre-commit run --all-files ruff --verbose
Run Ruff on Lib/test/....................................................Passed
- hook id: ruff
- duration: 0.02s

error: Failed to parse Lib/test/tokenizedata/badsyntax_3131.py:2:1: Got unexpected token €
error: Failed to parse Lib/test/test_fstring.py:671:28: f-string: expecting '}'
warning: Failed to lint Lib/test/badsyntax_pep3120.py: stream did not contain valid UTF-8
warning: Failed to lint Lib/test/encoded_modules/module_koi8_r.py: stream did not contain valid UTF-8
warning: Failed to lint Lib/test/encoded_modules/module_iso_8859_1.py: stream did not contain valid UTF-8
error: Failed to parse Lib/test/support/socket_helper.py:300:33: f-string: expecting '}'

❯ ruff Lib/test
warning: Failed to lint Lib/test/encoded_modules/module_iso_8859_1.py: stream did not contain valid UTF-8
warning: Failed to lint Lib/test/encoded_modules/module_koi8_r.py: stream did not contain valid UTF-8
warning: Failed to lint Lib/test/badsyntax_pep3120.py: stream did not contain valid UTF-8
error: Failed to parse Lib/test/support/socket_helper.py:300:33: f-string: expecting '}'
error: Failed to parse Lib/test/test_fstring.py:671:28: f-string: expecting '}'
error: Failed to parse Lib/test/tokenizedata/badsyntax_3131.py:2:1: Got unexpected token €

Shall we explicitly exclude them (which makes the verbose output cleaner, and in which case I'll update the backports) or keep the default behaviour (in which case I'll update main)?

I think it's better to explicitly exclude the unlintable files, as we do on main. It's maybe slightly harder to maintain, but the improvement in UI is worth it if contributors don't see all those warnings printed to the terminal 👍

Thanks for doing this!

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

…09365) * gh-60283: Check for redefined test names in CI (GH-109161) (cherry picked from commit 3cb9a8e) Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> * Update exclude list for 3.12 * Explicitly exclude files which failed to lint/parse * Sort to avoid future merge conflicts --------- Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com> Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

) Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> (cherry picked from commit 3cb9a8e)

hugovk added 2 commits September 8, 2023 17:37

Check for redefined names in CI

4cd4803

Temporarily exclude files with: F811 Redefinition of unused name

a37828c

hugovk requested a review from AlexWaygood as a code owner September 8, 2023 20:49

bedevere-bot added the awaiting core review label Sep 8, 2023

hugovk marked this pull request as draft September 8, 2023 20:49

bedevere-bot mentioned this pull request Sep 8, 2023

list duplicate test names with patchcheck #60283

Closed

bedevere-bot removed the awaiting core review label Sep 8, 2023

hugovk added awaiting core review skip news tests Tests in the Lib/test dir labels Sep 8, 2023

hugovk mentioned this pull request Sep 8, 2023

Detect duplicate method names in CI python/core-workflow#505

Closed

AlexWaygood reviewed Sep 8, 2023

View reviewed changes

Comment thread .ruff.toml Outdated

Comment thread .pre-commit-config.yaml Outdated

Move Ruff config file to relevant subdirectory to localise rules

0dcdd08

AlexWaygood reviewed Sep 9, 2023

View reviewed changes

Comment thread Lib/test/.ruff.toml Outdated

hugovk and others added 2 commits September 9, 2023 06:22

Comment the rule

d14c002

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

Move autofix setting from pre-commit to Ruff config

cb8237c

AlexWaygood approved these changes Sep 9, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 9, 2023

AA-Turner reviewed Sep 9, 2023

View reviewed changes

Comment thread .pre-commit-config.yaml

hugovk added 3 commits September 10, 2023 13:54

Re-order args

83904af

Remove --diff

8fd293a

hugovk added 3 commits September 10, 2023 14:56

Update config

66e1172

Don't validate intentionally invalid TOML

9b1b43a

Let tomllib look after its own test files

3839aea

AlexWaygood reviewed Sep 10, 2023

View reviewed changes

Comment thread .pre-commit-config.yaml

Clarify job scope

0bf3e3e

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>

hugovk requested a review from ezio-melotti as a code owner September 12, 2023 14:45

bedevere-app Bot added awaiting review and removed awaiting merge labels Sep 12, 2023

hugovk enabled auto-merge (squash) September 12, 2023 14:49

hugovk added triaged The issue has been accepted as valid by a triager. and removed triaged The issue has been accepted as valid by a triager. labels Sep 12, 2023

hugovk merged commit 3cb9a8e into python:main Sep 12, 2023

hugovk deleted the pre-commit-ruff-F811 branch September 12, 2023 15:28

bedevere-app Bot removed the awaiting review label Sep 12, 2023

hugovk added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Sep 13, 2023

miss-islington assigned hugovk Sep 13, 2023

bedevere-app Bot removed the needs backport to 3.12 only security fixes label Sep 13, 2023

bedevere-app Bot removed the needs backport to 3.11 only security fixes label Sep 13, 2023

vstinner pushed a commit to vstinner/cpython that referenced this pull request Sep 13, 2023

pythongh-60283 : Check for redefined test names in CI (python#109161 )

c17f5a3

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com> Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>

AlexWaygood mentioned this pull request Oct 9, 2023

Run ruff on Argument Clinic in CI #110558

Closed

AA-Turner mentioned this pull request Feb 3, 2025

Convert change detection to a Python script #129627

Merged

gh-60283: Check for redefined test names in CI by hugovk · Pull Request #109161 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-60283: Check for redefined test names in CI - #109161

gh-60283: Check for redefined test names in CI#109161hugovk merged 16 commits intopython:mainpython/cpython:mainfrom hugovk:pre-commit-ruff-F811hugovk/cpython:pre-commit-ruff-F811Copy head branch name to clipboard

Conversation

hugovk commented Sep 8, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hugovk commented Sep 10, 2023

Uh oh!

AlexWaygood commented Sep 10, 2023

Uh oh!

Uh oh!

AlexWaygood commented Sep 12, 2023

Uh oh!

miss-islington commented Sep 13, 2023

Uh oh!

miss-islington commented Sep 13, 2023

Uh oh!

miss-islington commented Sep 13, 2023

Uh oh!

bedevere-app Bot commented Sep 13, 2023

Uh oh!

bedevere-app Bot commented Sep 13, 2023

Uh oh!

hugovk commented Sep 13, 2023

Uh oh!

AlexWaygood commented Sep 13, 2023

Uh oh!

erlend-aasland commented Sep 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gh-60283: Check for redefined test names in CI#109161
hugovk merged 16 commits into
python:mainfrom
hugovk:pre-commit-ruff-F811

hugovk commented Sep 8, 2023 •
edited by bedevere-bot
Loading