Fix handling of dependency markers pointing to tests with same name

- add performance test for dependency markers

Author: mrbean-bremen

.github/workflows/performance.yml

@@ -37,6 +37,7 @@ jobs:
       run: |
         python -m pip install wheel
         python -m pip install pytest
+        python -m pip install pytest-dependency
     - name: Run tests
       run: |
         python -m pytest -s perf_tests

CHANGELOG.md

@@ -2,8 +2,12 @@
 
 ## Unreleased
 
+### Fixes
+- fixed sorting of dependency markers that depend on an item with the same
+  name in different modules
+
 ### Infrastructure
-- add performance tests to prevent performance degradation
+- added performance tests to prevent performance degradation
 
 ## [Version 0.9.5](https://pypi.org/project/pytest-order/0.9.5/) (2021-02-16)
 Introduces hierarchical ordering option and fixes ordering of session-scoped

perf_tests/test_dependencies.py

@@ -0,0 +1,45 @@
+import os
+import shutil
+from unittest import mock
+
+import pytest
+
+import pytest_order
+from perf_tests.util import TimedSorter
+from tests.utils import write_test
+
+
+@pytest.fixture
+def fixture_path_relative(tmpdir_factory):
+    fixture_path = str(tmpdir_factory.mktemp("dep_perf_sparse"))
+    for module_index in range(10):
+        testname = os.path.join(
+            fixture_path, "test_dep_perf{}.py".format(module_index))
+        test_contents = """
+import pytest
+"""
+        for i in range(40):
+            test_contents += """
+@pytest.mark.dependency(depends=["test_{}"])
+def test_{}():
+    assert True
+""".format(i + 50, i)
+        for i in range(60):
+            test_contents += """
+@pytest.mark.dependency
+def test_{}():
+    assert True
+""".format(i + 40)
+        write_test(testname, test_contents)
+    yield fixture_path
+    shutil.rmtree(fixture_path, ignore_errors=True)
+
+
+@mock.patch("pytest_order.Sorter", TimedSorter)
+def test_performance_dependency(fixture_path_relative):
+    """Test performance of dependency markers that point to tests without
+    an order mark (same as for test_relative does for after markers)."""
+    args = ["--order-dependencies", fixture_path_relative]
+    TimedSorter.nr_marks = 400
+    pytest.main(args, [pytest_order])
+    assert TimedSorter.elapsed < 0.4

perf_tests/test_ordinal.py

@@ -31,5 +31,6 @@ def test_{}():
 @mock.patch("pytest_order.Sorter", TimedSorter)
 def test_performance_ordinal(fixture_path_ordinal):
     args = [fixture_path_ordinal]
+    TimedSorter.nr_marks = 1000
     pytest.main(args, [pytest_order])
     assert TimedSorter.elapsed < 0.02

perf_tests/test_relative.py

@@ -18,24 +18,27 @@ def fixture_path_relative(tmpdir_factory):
         test_contents = """
 import pytest
 """
-        for i in range(90):
+        for i in range(40):
             test_contents += """
 @pytest.mark.order(after="test_{}")
 def test_{}():
     assert True
-""".format(i + 10, i)
-        for i in range(10):
+""".format(i + 50, i)
+        for i in range(60):
             test_contents += """
 def test_{}():
     assert True
-""".format(i + 90)
+""".format(i + 40)
         write_test(testname, test_contents)
     yield fixture_path
     shutil.rmtree(fixture_path, ignore_errors=True)
 
 
 @mock.patch("pytest_order.Sorter", TimedSorter)
 def test_performance_relative(fixture_path_relative):
+    """Test performance of after markers that point to tests without
+    an order mark (the usual case)."""
     args = [fixture_path_relative]
+    TimedSorter.nr_marks = 400
     pytest.main(args, [pytest_order])
-    assert TimedSorter.elapsed < 1.2
+    assert TimedSorter.elapsed < 0.5

perf_tests/test_relative_dense.py

@@ -0,0 +1,44 @@
+import os
+import shutil
+from unittest import mock
+
+import pytest
+
+import pytest_order
+from perf_tests.util import TimedSorter
+from tests.utils import write_test
+
+
+@pytest.fixture
+def fixture_path_relative_dense(tmpdir_factory):
+    fixture_path = str(tmpdir_factory.mktemp("relative_dense_perf"))
+    for module_index in range(10):
+        testname = os.path.join(
+            fixture_path, "test_relative_dense_perf{}.py".format(module_index))
+        test_contents = """
+import pytest
+"""
+        for i in range(90):
+            test_contents += """
+@pytest.mark.order(after="test_{}")
+def test_{}():
+    assert True
+""".format(i + 10, i)
+        for i in range(10):
+            test_contents += """
+def test_{}():
+    assert True
+""".format(i + 90)
+        write_test(testname, test_contents)
+    yield fixture_path
+    shutil.rmtree(fixture_path, ignore_errors=True)
+
+
+@mock.patch("pytest_order.Sorter", TimedSorter)
+def test_performance_relative(fixture_path_relative_dense):
+    """Test performance of after markers that mostly point to tests with
+    another order mark, so items are evaluated multiple times."""
+    args = [fixture_path_relative_dense]
+    TimedSorter.nr_marks = 900
+    pytest.main(args, [pytest_order])
+    assert TimedSorter.elapsed < 1.2

perf_tests/util.py

@@ -5,11 +5,13 @@
 
 class TimedSorter(Sorter):
     elapsed = 0
+    nr_marks = 1000
 
     def sort_items(self):
         self.__class__.elapsed = 0
         start_time = time.time()
         items = super().sort_items()
-        self.__class__.elapsed += time.time() - start_time
+        self.__class__.elapsed = ((time.time() - start_time)
+                                  / self.nr_marks * 1000)
         print("Time per test: {:.3f} ms".format(self.__class__.elapsed))
         return items

pytest_order/sorter.py

@@ -197,14 +197,25 @@ def show_unresolved_dep_items(self):
             sys.stdout.flush()
 
 
-def label_from_alias(alias_names, label, scope):
+def scope_from_name(name):
+    if name == "module":
+        return MODULE
+    if name == "class":
+        return CLASS
+    return SESSION
+
+
+def label_from_alias(alias_names, label, scope, prefix):
     name = alias_names.get(label)
-    if name or scope in ("session", "package"):
+    if name or scope == SESSION:
         return name
+    prefixed_label = prefix + "::" + label
+    if prefixed_label in alias_names:
+        return alias_names[prefixed_label]
     for alias in alias_names:
         if "::" in alias:
             name = alias.split("::", 1)[1]
-            if scope == "class" and "::" in name:
+            if scope == CLASS and "::" in name:
                 name = name.split("::")[1]
         else:
             name = alias
@@ -220,10 +231,18 @@ def scoped_label(label, scope):
     if scope == CLASS:
         if "::" not in label:
             return label
-        return label.split("::")[0]
+        return label[:label.index("::")]
     return label
 
 
+def scoped_node_id(node_id, scope):
+    if scope == MODULE:
+        return node_id[:node_id.index("::")]
+    if scope == CLASS:
+        return node_id[:node_id.rindex("::")]
+    return ""
+
+
 class GroupSorter:
     def __init__(self, scope, groups, before_items, after_items, dep_items):
         self.scope = scope
@@ -291,13 +310,13 @@ def handle_after_items(self):
 
     def handle_dep_items(self):
         remove_labels = []
-        for (label, dep_scope), after in self.dep_items.items():
+        for (label, dep_scope, prefix), after in self.dep_items.items():
             group_index = self.group_index_from_label(
-                label, dep_scope)
+                label, dep_scope, prefix)
             if self.insert_after_dep_group(group_index, after):
-                remove_labels.append((label, dep_scope))
-        for (label, dep_scope) in remove_labels:
-            del self.dep_items[(label, dep_scope)]
+                remove_labels.append((label, dep_scope, prefix))
+        for (label, dep_scope, prefix) in remove_labels:
+            del self.dep_items[(label, dep_scope, prefix)]
 
     def insert_before_group(self, label, items):
         label = scoped_label(label, self.scope)
@@ -351,9 +370,9 @@ def insert_after_dep_group(self, group_index, items):
             return found
         return False
 
-    def group_index_from_label(self, label, scope):
+    def group_index_from_label(self, label, scope, prefix):
         for index, group in enumerate(self.groups):
-            if label_from_alias(group.aliases, label, scope):
+            if label_from_alias(group.aliases, label, scope, prefix):
                 return index
 
 
@@ -439,9 +458,11 @@ def handle_dependency_mark(self, item, has_order):
         if self.settings.order_dependencies or has_order:
             dependent_mark = mark.kwargs.get("depends")
             if dependent_mark:
-                scope = mark.kwargs.get("scope", "module")
+                scope = scope_from_name(mark.kwargs.get("scope", "module"))
+                prefix = scoped_node_id(item.node_id, scope)
                 for name in dependent_mark:
-                    self.dep_items.setdefault((name, scope), []).append(item)
+                    self.dep_items.setdefault(
+                        (name, scope, prefix), []).append(item)
                     item.is_rel_mark = True
         # we always collect the names of the dependent items, because
         # we need them in both cases
@@ -557,8 +578,9 @@ def handle_unhandled_items(self, items, out_items, scope):
 
     def handle_unhandled_dep_items(self, out_items):
         msg = ""
-        for (label, dep_scope), entries in self.dep_items.items():
-            out_items.setdefault((label, dep_scope), []).extend(entries)
+        for (label, dep_scope, prefix), entries in self.dep_items.items():
+            out_items.setdefault(
+                (label, dep_scope, prefix), []).extend(entries)
         return msg
 
     def number_of_rel_groups(self):
@@ -585,8 +607,8 @@ def handle_after_items(self, sorted_list):
     def handle_dep_items(self, sorted_list):
         remove_labels = []
         nr_unhandled = 0
-        for (label, dep_scope), after in self.dep_items.items():
-            name = label_from_alias(self.aliases, label, dep_scope)
+        for (label, dep_scope, prefix), after in self.dep_items.items():
+            name = label_from_alias(self.aliases, label, dep_scope, prefix)
             if name is None:
                 for item in after:
                     if item.is_rel_mark:
@@ -595,9 +617,9 @@ def handle_dep_items(self, sorted_list):
                         item.is_rel_mark = False
                         nr_unhandled += 1
             elif self.insert_after(name, after, sorted_list):
-                remove_labels.append((label, dep_scope))
-        for (label, dep_scope) in remove_labels:
-            del self.dep_items[(label, dep_scope)]
+                remove_labels.append((label, dep_scope, prefix))
+        for (label, dep_scope, prefix) in remove_labels:
+            del self.dep_items[(label, dep_scope, prefix)]
         return nr_unhandled
 
     def group_order(self):

tests/test_dependency.py

@@ -269,6 +269,27 @@ def test_f(self):
     ]
 
 
+def test_class_scope_dependencies(item_names_for, order_dependencies):
+    tests_content = """
+    import pytest
+
+    class TestA:
+        @pytest.mark.dependency(depends=["test_c"], scope='class')
+        def test_a(self):
+            assert True
+
+        def test_b(self):
+            assert True
+
+        @pytest.mark.dependency
+        def test_c(self):
+            assert True
+    """
+    assert item_names_for(tests_content) == [
+        "test_b", "test_c", "test_a"
+    ]
+
+
 @pytest.fixture
 def fixture_path_named(tmpdir_factory):
     fixture_path = str(tmpdir_factory.mktemp("named_dep"))
@@ -369,6 +390,56 @@ def test_dependency_in_modules(fixture_path_unnamed, capsys):
     assert "SKIPPED" not in out
 
 
+@pytest.fixture
+def fixture_path_modules_with_same_dep(tmpdir_factory):
+    fixture_path = str(tmpdir_factory.mktemp("modules_dep"))
+    testname = os.path.join(fixture_path, "test_module_dep1.py")
+    test_contents = """
+import pytest
+
+@pytest.mark.dependency(depends=['test_two'])
+def test_one():
+    assert True
+
+@pytest.mark.dependency
+def test_two():
+    assert True
+"""
+    write_test(testname, test_contents)
+    test_contents = """
+import pytest
+
+@pytest.mark.dependency(depends=['test_two'])
+def test_one():
+    assert True
+
+@pytest.mark.dependency
+def test_two():
+    assert True
+    """
+    testname = os.path.join(fixture_path, "test_module_dep2.py")
+    write_test(testname, test_contents)
+    yield fixture_path
+    shutil.rmtree(fixture_path, ignore_errors=True)
+
+
+def test_same_dependency_in_modules(
+        fixture_path_modules_with_same_dep, capsys):
+    # regression test - make sure that the same dependency in different
+    # modules works correctly
+    args = ["-v", "--order-dependencies", fixture_path_modules_with_same_dep]
+    pytest.main(args, [pytest_order])
+    out, err = capsys.readouterr()
+    expected = (
+        "test_module_dep1.py::test_two",
+        "test_module_dep1.py::test_one",
+        "test_module_dep2.py::test_two",
+        "test_module_dep2.py::test_one",
+    )
+    assert_test_order(expected, out)
+    assert "SKIPPED" not in out
+
+
 def test_unknown_dependency(item_names_for, order_dependencies, capsys):
     tests_content = """
     import pytest