gh-60283: Check for redefined test names in CI

[hugovk]

Fixespython/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

[AlexWaygood]

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

[hugovk]

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

[AlexWaygood]

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!

[AlexWaygood]

🥳

[miss-islington]

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

[miss-islington]

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

[miss-islington]

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

[bedevere-app]

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

[bedevere-app]

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

[hugovk]

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 ruffRun Ruff on Lib/test/....................................................Passed

❯ pre-commit run --all-files ruff --verboseRun Ruff on Lib/test/....................................................Passed- hook id: ruff- duration: 0.02serror: 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-8warning: Failed to lint Lib/test/encoded_modules/module_koi8_r.py: stream did not contain valid UTF-8warning: Failed to lint Lib/test/encoded_modules/module_iso_8859_1.py: stream did not contain valid UTF-8error: Failed to parse Lib/test/support/socket_helper.py:300:33: f-string: expecting '}'

❯ ruff Lib/testwarning: Failed to lint Lib/test/encoded_modules/module_iso_8859_1.py: stream did not contain valid UTF-8warning: Failed to lint Lib/test/encoded_modules/module_koi8_r.py: stream did not contain valid UTF-8warning: Failed to lint Lib/test/badsyntax_pep3120.py: stream did not contain valid UTF-8error: 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)?

[AlexWaygood]

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 👍

[erlend-aasland]

Thanks for doing this!