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

Detect duplicate method names in CI #505

Closed
erlend-aasland opened this issue Jun 27, 2023 · 14 comments · Fixed by python/cpython#109161
Closed

Detect duplicate method names in CI #505

erlend-aasland opened this issue Jun 27, 2023 · 14 comments · Fixed by python/cpython#109161

Comments

@erlend-aasland
Copy link

[...] Maybe we can just use pyflakes in a CI job, only looking for this specific kind of issue?

Originally posted by @vstinner in python/cpython#105560 (comment)
@hugovk
Copy link
Member

hugovk commented Jun 27, 2023 •
edited
Loading

We can do something like this with Flake8's F811 check:

Currently detects 132 things:

Details 
Lib/collections/__init__.py:338:5: F811 redefinition of unused 'OrderedDict' from line 83
Lib/collections/__init__.py:538:5: F811 redefinition of unused '_count_elements' from line 531
Lib/ctypes/__init__.py:305:20: F811 redefinition of unused 'pointer' from line 255
Lib/functools.py:226:5: F811 redefinition of unused 'cmp_to_key' from line 206
Lib/functools.py:266:5: F811 redefinition of unused 'reduce' from line 237
Lib/functools.py:342:5: F811 redefinition of unused 'partial' from line 276
Lib/functools.py:643:5: F811 redefinition of unused '_lru_cache_wrapper' from line 526
Lib/heapq.py:587:5: F811 redefinition of unused '_heapreplace_max' from line 191
Lib/heapq.py:591:5: F811 redefinition of unused '_heapify_max' from line 198
Lib/heapq.py:595:5: F811 redefinition of unused '_heappop_max' from line 181
Lib/idlelib/run.py:639:5: F811 redefinition of unused 'main' from line 110
Lib/multiprocessing/synchronize.py:46:1: F811 redefinition of unused 'SemLock' from line 28
Lib/pickle.py:1769:5: F811 redefinition of unused 'PicklingError' from line 77
Lib/pickle.py:1769:5: F811 redefinition of unused 'UnpicklingError' from line 84
Lib/poplib.py:454:5: F811 redefinition of unused 'sys' from line 19
Lib/socket.py:563:5: F811 redefinition of unused 'array' from line 551
Lib/statistics.py:1231:5: F811 redefinition of unused '_normal_dist_inv_cdf' from line 1155
Lib/test/test_buffer.py:767:5: F811 redefinition of unused 'genslices' from line 702
Lib/test/test_buffer.py:768:5: F811 redefinition of unused 'genslices_ndim' from line 706
Lib/test/test_buffer.py:769:5: F811 redefinition of unused 'permutations' from line 21
Lib/test/test_buffer.py:4725:5: F811 redefinition of unused 'test_multiple_inheritance_buffer_last' from line 4697
Lib/test/test_capi/test_misc.py:1826:9: F811 redefinition of unused 'json' from line 9
Lib/test/test_capi/test_misc.py:1905:9: F811 redefinition of unused 'json' from line 9
Lib/test/test_ctypes/test_arrays.py:192:13: F811 redefinition of unused 'T' from line 189
Lib/test/test_ctypes/test_arrays.py:195:13: F811 redefinition of unused 'T' from line 192
Lib/test/test_ctypes/test_arrays.py:204:13: F811 redefinition of unused 'T' from line 200
Lib/test/test_ctypes/test_arrays.py:208:13: F811 redefinition of unused 'T' from line 204
Lib/test/test_ctypes/test_arrays.py:212:13: F811 redefinition of unused 'T' from line 208
Lib/test/test_ctypes/test_functions.py:49:13: F811 redefinition of unused 'X' from line 44
Lib/test/test_ctypes/test_functions.py:53:13: F811 redefinition of unused 'X' from line 49
Lib/test/test_ctypes/test_functions.py:57:13: F811 redefinition of unused 'X' from line 53
Lib/test/test_curses.py:1371:5: F811 redefinition of unused 'test_move_left' from line 1362
Lib/test/test_dataclasses.py:589:13: F811 redefinition of unused 'A' from line 584
Lib/test/test_dataclasses.py:599:13: F811 redefinition of unused 'A' from line 589
Lib/test/test_dataclasses.py:753:21: F811 redefinition of unused 'Point' from line 744
Lib/test/test_dataclasses.py:764:21: F811 redefinition of unused 'Point' from line 753
Lib/test/test_dataclasses.py:774:17: F811 redefinition of unused 'C' from line 769
Lib/test/test_dataclasses.py:2011:9: F811 redefinition of unused 'C' from line 2005
Lib/test/test_dataclasses.py:2570:13: F811 redefinition of unused 'C' from line 2561
Lib/test/test_dataclasses.py:2579:13: F811 redefinition of unused 'C' from line 2570
Lib/test/test_dataclasses.py:2588:13: F811 redefinition of unused 'C' from line 2579
Lib/test/test_dataclasses.py:3005:13: F811 redefinition of unused 'C' from line 2997
Lib/test/test_dataclasses.py:3011:9: F811 redefinition of unused 'C' from line 3005
Lib/test/test_dataclasses.py:4282:13: F811 redefinition of unused 'A' from line 4277
Lib/test/test_dataclasses.py:4287:13: F811 redefinition of unused 'A' from line 4282
Lib/test/test_dataclasses.py:4429:13: F811 redefinition of unused 'A' from line 4420
Lib/test/test_dataclasses.py:4438:13: F811 redefinition of unused 'A' from line 4429
Lib/test/test_grammar.py:1222:9: F811 redefinition of unused 'time' from line 1221
Lib/test/test_grammar.py:1226:9: F811 redefinition of unused 'path' from line 1225
Lib/test/test_grammar.py:1226:9: F811 redefinition of unused 'argv' from line 1225
Lib/test/test_grammar.py:1227:9: F811 redefinition of unused 'path' from line 1226
Lib/test/test_grammar.py:1227:9: F811 redefinition of unused 'argv' from line 1226
Lib/test/test_grammar.py:1610:9: F811 redefinition of unused 'sys' from line 8
Lib/test/test_import/__init__.py:541:13: F811 redefinition of unused 'x' from line 536
Lib/test/test_keywordonlyarg.py:173:13: F811 redefinition of unused 'f' from line 169
Lib/test/test_monitoring.py:973:5: F811 redefinition of unused 'test_line_then_instruction' from line 950
Lib/test/test_monitoring.py:978:5: F811 redefinition of unused 'test_instruction_then_line' from line 955
Lib/test/test_pkg.py:200:9: F811 redefinition of unused 't5' from line 193
Lib/test/test_subclassinit.py:233:13: F811 redefinition of unused 'MyClass' from line 219
Lib/test/test_subclassinit.py:244:9: F811 redefinition of unused 'MyClass' from line 233
Lib/test/test_subclassinit.py:266:9: F811 redefinition of unused 'MyClass' from line 257
Lib/test/test_tokenize.py:12:1: F811 redefinition of unused 'os_helper' from line 9
Lib/test/test_traceback.py:921:1: F811 redefinition of unused 'CPythonTracebackErrorCaretTests' from line 910
Lib/test/test_unittest/testmock/testpatch.py:1960:13: F811 redefinition of unused 'test' from line 1950
Lib/test/test_yield_from.py:921:9: F811 redefinition of unused 'two' from line 890
Lib/test/test_zoneinfo/test_zoneinfo.py:1109:9: F811 redefinition of unused '_add' from line 1081
Lib/test/test_zoneinfo/test_zoneinfo.py:[113](https://github.com/hugovk/cpython/actions/runs/5390304327/jobs/9785454219#step:4:113)3:9: F811 redefinition of unused '_add' from line 1109
Lib/test/test_zoneinfo/test_zoneinfo.py:1158:9: F811 redefinition of unused '_add' from line 1133
Lib/test/test_zoneinfo/test_zoneinfo.py:1182:9: F811 redefinition of unused '_add' from line 1158
Lib/test/test_zoneinfo/test_zoneinfo.py:1195:9: F811 redefinition of unused '_add' from line 1182
Lib/test/test_zoneinfo/test_zoneinfo.py:1208:9: F811 redefinition of unused '_add' from line 1195
Lib/test/test_zoneinfo/test_zoneinfo.py:1229:9: F811 redefinition of unused '_add' from line 1208
Lib/test/test_zoneinfo/test_zoneinfo.py:1262:9: F811 redefinition of unused '_add' from line 1229
Lib/test/test_zoneinfo/test_zoneinfo.py:1285:9: F811 redefinition of unused '_add' from line 1262
Lib/test/time_hashlib.py:60:5: F811 redefinition of unused 'creatorFunc' from line 9
Lib/tkinter/__init__.py:98:6: F811 redefinition of unused '_flatten' from line 87
Lib/tkinter/__init__.py:120:6: F811 redefinition of unused '_cnfmerge' from line 102
Lib/warnings.py:564:5: F811 redefinition of unused 'warn' from line 296
Lib/warnings.py:564:5: F811 redefinition of unused 'warn_explicit' from line 342
Tools/c-analyzer/c_parser/parser/_func_body.py:46:1: F811 redefinition of unused 'parse_function_body' from line 41
Tools/c-analyzer/distutils/msvccompiler.py:327:5: F811 redefinition of unused 'MacroExpander' from line 98
Tools/cases_generator/lexer.py:253:5: F811 redefinition of unused 'sys' from line 6

@hugovk
Copy link
Member

hugovk commented Jun 27, 2023

Or the same thing using Ruff to check F811, which is quicker:

Time to install and run pre-commit on CI:

This was referenced Jun 27, 2023
Closed
Closed
This was referenced Jun 28, 2023
Closed
Closed
Closed
Closed
@sobolevn
Copy link
Member

Going throught the list. I found several entries that are real problems (by "real problems" I mean: things that alter the runtime behavior in unexpected ways):

  1. Lib/test/test_traceback.py:921 test_traceback does not run one set of CPythonTracebackErrorCaretTests cpython#106185
  2. Lib/test/test_monitoring.py test_monitoring has duplicated tests cpython#106193
  3. Lib/test/test_curses.py:1371 test_curses has duplicated tests cpython#106194
  4. Lib/test/test_buffer.py:4725 test_multiple_inheritance_buffer_last is duplicated in test_buffer cpython#106197

Plus, there are multiple imports that can be removed. I will send a PR for that later.
But, the amount of things that are just false positives is overwhelming.

@sobolevn sobolevn mentioned this issue Jun 28, 2023
Closed
@hugovk
Copy link
Member

hugovk commented Jun 28, 2023

Thanks for checking these. We should backport the test fixes to at least 3.11.

And maybe for the security-only releases too? It's usually better to run tests than accidentally skip them :)

@AlexWaygood
Copy link
Member

I'd suggest that we only run pyflakes/the Ruff equivalent on Lib/test/. Accidentally skipping tests due to duplicate method names (or duplicate class names) is a common problem that we've hit multiple times in the past, so it would be good to have a CI check to guard against it. But I don't think we have the same issue with other Python files in the CPython repo (which are generally tested more rigorously than the tests themselves).

But, the amount of things that are just false positives is overwhelming.

I looked at this a while back -- I think a common source of false positives was things like this in tests:

def test_bad_class(self):
    with self.assertRaises(TypeError):
        class C(BadBaseClass): pass
    with self.assertRaises(TypeError):
        class C(OtherBadBadClass): pass  # pyflakes reports redefinition of unused class "C"

That kind of thing is trivial to resolve -- the second C definition can just be renamed to C1 in the test.

@sobolevn
Copy link
Member

I think that our own ast-based linter is better:

  1. It is quite easy to write
  2. We can fully customize it
  3. Many core-devs have expertise in ast
  4. The problem does actually exist
  5. We won't have to adapt any existing code to fit our rules (except ones I've fixed in the PRs above)

I can write a prototype by today's evening (right after my lecture today).

@AlexWaygood
Copy link
Member

I think that our own ast-based linter is better:

I predict that at some point somebody will propose writing tests for the custom linter, at which point we'll be writing a test for the test testing the tests 😆

But, sounds good!

@vstinner
Copy link
Member

Thanks for checking these. We should backport the test fixes to at least 3.11. And maybe for the security-only releases too? It's usually better to run tests than accidentally skip them :)

Sometimes, when tests which were disabled for long time are enabled again, they start to fail randomly and need be fixed. I dislike the idea of enabling in stable branches.

@hugovk
Copy link
Member

hugovk commented Jun 28, 2023

I slightly prefer using an existing linter (if suitable) than a custom one:

  • less stuff for us to maintain
  • well tested
  • many existing tools are well known in the community
  • usually highly configurable

Although potentially they might not run on pre-release Python code that is under development, like failing to parse new syntax.

@sobolevn
Copy link
Member

I remembered that @vstinner already had a prototype, so here's mine based on python/cpython#105560 (comment)

Code: https://gist.github.com/sobolevn/967ac61b8b2575e26a33f5a6bf5df201

Output:

» ./python.exe check_test_duplicates.py Lib/test                          
Lib/test/test_curses.py:1371 Found duplicate method test_move_left in TextboxTest class
Lib/test/test_monitoring.py:973 Found duplicate method test_line_then_instruction in TestInstallIncrementallly class
Lib/test/test_monitoring.py:978 Found duplicate method test_instruction_then_line in TestInstallIncrementallly class
Lib/test/test_traceback.py:921 Found duplicate CPythonTracebackErrorCaretTests class
Lib/test/test_importlib/test_util.py:614 Found duplicate MagicNumberTests class
Lib/test/test_importlib/test_abc.py:148 Found duplicate MetaPathFinder class
Lib/test/test_importlib/test_abc.py:167 Found duplicate PathEntryFinder class
Lib/test/test_importlib/test_abc.py:218 Found duplicate ResourceLoader class
Lib/test/test_importlib/test_abc.py:238 Found duplicate InspectLoader class
Lib/test/test_importlib/test_abc.py:268 Found duplicate ExecutionLoader class
Lib/test/test_importlib/test_abc.py:622 Found duplicate SourceLoader class

Cases listed in Lib/test/test_importlib/test_abc.py are indeed duplicates, but they use very interesting magic. I can addapt my solution to fix that.

@gpshead
Copy link
Member

gpshead commented Jun 28, 2023

For tests specifically, couldn't a unittest.TestCase metaclass be made that detects duplicate method definitions at class definition time? (turning that into at least a warning if not an error)

@sobolevn
Copy link
Member

@gpshead technically, we can. It might break some 3rd party unittest plugin that use metaclasses for their own stuff, but people can eventually accommodate them.

But, unittest.TestCase won't solve duplicated classes. We can try tweaking unittest.Loader for that as well.

@vstinner
Copy link
Member

@gpshead: i wrote python/cpython#105560 metaclass to detect duplicates.

This was referenced Sep 8, 2023
Merged
Closed
Merged
@hugovk
Copy link
Member

hugovk commented Sep 8, 2023

Please see python/cpython#109161 to check for F811 with Ruff via pre-commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gh-60283: Check for redefined test names in CI hugovk/cpython
6 participants
@gpshead @vstinner @hugovk @sobolevn @erlend-aasland @AlexWaygood