unittest: Detect duplicate method names

[vstinner]

Currently, when an unittest.TestCase has two test methods with the same name, only the second one is run: the first one is silently ignored. In the past, the Python test suite had many bugs like that: see issue #60283. I just found a new bug today in test_sqlite3: issue #105557.

It would be nice to automatically detect when a test method is redefined. The problem is to avoid false alarms on method redefined on purpose: right now, I don't know if there are legit use cases like that.

Here is a prototype:

import inspect
import warnings

class MyDict(dict):
    def __init__(self, class_name):
        self.class_name = class_name

    def __setitem__(self, name, value):
        # TestLoader.testMethodPrefix = 'test'
        if name.startswith('test'):
            try:
                old_value = self[name]
            except KeyError:
                pass
            else:
                if inspect.isfunction(old_value):
                    warnings.warn(f"BUG: {self.class_name}.{name}() method redefined", stacklevel=2)
        dict.__setitem__(self, name, value)

class MyMetaClass(type):
    @classmethod
    def __prepare__(metacls, class_name, bases, **kwds):
        return MyDict(class_name)

class TestCase(metaclass=MyMetaClass):
    pass

class Tests(TestCase):
    def __init__(self):
        self.attr = 1
        self.attr = 2

    def test_bug(self):  # first defintion
        pass

    def test_bug(self):  # second definion
        pass

class RealTests(Tests):
    def test_bug(self):  # implementation
        print("run tests")


tests = RealTests()

Output:

poc.py:36: UserWarning: BUG: Tests.test_bug() method redefined
  def test_bug(self):  # second definion

It only emits a warning on the second Tests.test_bug() method definition. It doesn't emit a warning when RealTests redefines test_bug(): is it a legit use case to override an "abstract" test method of a "base" test case class? This case can also be detected if needed.

[vstinner]

If someone wants to propose a PR based on my proof-of-concept, please go ahead. I'm not really interested to implement a fix for this issue right now. I just report the issue :-)

cc @erlend-aasland @serhiy-storchaka

[serhiy-storchaka]

Yes, this problem is known and was automatically detect was discussed before, but AFAIK it was not materialized in a PR.

The proposed solution would have false positive report for the following example:

def test_xxx(self):
    ...
if cond:
    test_xxx = decorator(test_xxx)

Also, it only works if tests with the same name are defined in the same class inherited from TestCase. It does not help if a test is overridden in a subclass or defined in a mixin class (or different mixin classes), which may be more common type of errors. But a solution which checks that the test does not override functions in superclasses would fail for many legitimate use cases.

[sobolevn]

This solution will also add a potential meta-class conflict for tools that already use metaclasses, like test plugins, custom loaders, etc.

[erlend-aasland]

It seems like this is not an easy feature to add :( Suggesting to close this issue.

[vstinner]

I understand that unittest.TestCase class namespace cannot be customed, because unittest is written to be extended, and metaclasses and other hacks are common.

If it's not possible to change unittest.TestCase class, would it make sense to have a different solution to at least detect duplicate method names in the Python test suite? Maybe using a custom Python source code parser?

Demo using ast.NodeVisitor:

import ast

code = """
class TestCase1:
    def test_x(self): pass
    def test_x(self): pass

class TestCase2:
    def test_x(self): pass
"""

tree = ast.parse(code)

METHOD_PREFIX = 'test'

class VisitClass:
    def __init__(self, name):
        self.name = name
        self.methods = set()

    def add_method(self, name):
        print(f"AST visit: method {self.name}.{name}")
        qualname = f"{self.name}.{name}"
        if name in self.methods:
            print(f"FOUND DUPLICATE METHOD: {qualname}")
        else:
            self.methods.add(name)


class Visitor(ast.NodeVisitor):
    def __init__(self):
        super().__init__()
        self.current_class = None

    def visit_ClassDef(self, node):
        self.current_class = VisitClass(node.name)
        super().generic_visit(node)

    def visit_FunctionDef(self, node):
        if self.current_class and node.name.startswith(METHOD_PREFIX):
            self.current_class.add_method(node.name)
        super().generic_visit(node)

Visitor().visit(tree)

This Proof-Of-Concept doesn't understand decorators or how to build test case cases using mixin classes and class inheritance. But it may be able to catch some obvious bugs. If such tool detects false positives later, it can be enhanced later, no?

[erlend-aasland]

Perhaps it can be made as a tool that is run by patchcheck?

[sobolevn]

pyflakes can do it:

Or even better flake8 (it has more configuration options and uses pyflakes inside):

[erlend-aasland]

@sobolevn, is it possible to configure flake8 to only detect duplicate test method names? If so, we could simply add that to the CI, with no changes needed in the code base.

[sobolevn]

to only detect duplicate test method names?

No, it will detect all duplicated functions and methods and some similar things: duplicated classes, imports, etc.

I think that all of these checks make sense in our case.

[erlend-aasland]

Related:

• list duplicate test names with patchcheck #60283

(Oh, Victor already referenced this issue; sorry 'bout the noise 😄 )

[vstinner]

pyflakes can do it

That's exactly how I found #105557 and why I created this issue... Maybe we can just use pyflakes in a CI job, only looking for this specific kind of issue?

[erlend-aasland]

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

+1

I created a follow-up issue in the core workflow repo. Let's close this and #60283 as superseded.