Closed erlend-aasland closed 1 year ago
We can do something like this with Flake8's F811 check:
Currently detects 132 things:
Or the same thing using Ruff to check F811, which is quicker:
Time to install and run pre-commit on CI:
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):
Lib/test/test_traceback.py:921
https://github.com/python/cpython/issues/106185Lib/test/test_monitoring.py
https://github.com/python/cpython/issues/106193Lib/test/test_curses.py:1371
https://github.com/python/cpython/issues/106194Lib/test/test_buffer.py:4725
https://github.com/python/cpython/issues/106197Plus, 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.
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 :)
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.
I think that our own ast
-based linter is better:
ast
I can write a prototype by today's evening (right after my lecture today).
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!
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.
I slightly prefer using an existing linter (if suitable) than a custom one:
Although potentially they might not run on pre-release Python code that is under development, like failing to parse new syntax.
I remembered that @vstinner already had a prototype, so here's mine based on https://github.com/python/cpython/issues/105560#issuecomment-1599842555
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.
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)
@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.
@gpshead: i wrote https://github.com/python/cpython/issues/105560 metaclass to detect duplicates.
Please see https://github.com/python/cpython/pull/109161 to check for F811 with Ruff via pre-commit.
Originally posted by @vstinner in https://github.com/python/cpython/issues/105560#issuecomment-1609308613