[lldb][test] Remove duplicate testcase names in API test-suite #97043

Michael137 · 2024-06-28T11:53:36Z

In one of my recent PRs I mistakenly had two test-cases with the same name, preventing one of them to run. Since it's an easy mistake to make (e.g., copy pasting existing test-cases), I ran following sanity-check script over lldb/test/API, which found couple of tests which were losing coverage because of this (or in some cases simply had duplicate tests):

import ast
import sys

filename = sys.argv[1]
print(f'Checking {filename}...')
tree = ast.parse(open(filename, 'r').read())

for node in ast.walk(tree):
    if not isinstance(node, ast.ClassDef):
        continue

    func_names = []
    for child in ast.iter_child_nodes(node):
        if isinstance(child, ast.FunctionDef):
            func_names.append(child.name)

    seen_func_names = set()
    duplicate_func_names = []
    for name in func_names:
        if name in seen_func_names:
            duplicate_func_names.append(name)
        else:
            seen_func_names.add(name)

    if len(duplicate_func_names) != 0:
        print(f'Multiple func names found:\n\t{duplicate_func_names}\n\tclass {node.name}\n\tfile: {filename}')

This patch fixes these cases.

In one of my recent PRs I mistakenly had two test-cases with the same name, preventing one of them to run. Since it's an easy mistake to make (e.g., copy pasting existing test-cases), I ran following sanity-check script over `lldb/test/API`, which found couple of tests which were losing coverage because of this (or in some cases simply had duplicate tests): ``` import ast import sys filename = sys.argv[1] print(f'Checking {filename}...') tree = ast.parse(open(filename, 'r').read()) for node in ast.walk(tree): if not isinstance(node, ast.ClassDef): continue func_names = [] for child in ast.iter_child_nodes(node): if isinstance(child, ast.FunctionDef): func_names.append(child.name) seen_func_names = set() duplicate_func_names = [] for name in func_names: if name in seen_func_names: duplicate_func_names.append(name) else: seen_func_names.add(name) if len(duplicate_func_names) != 0: print(f'Multiple func names found:\n\t{duplicate_func_names}\n\tclass {node.name}\n\tfile: {filename}') ``` This patch fixes these cases.

llvmbot · 2024-06-28T11:54:11Z

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

In one of my recent PRs I mistakenly had two test-cases with the same name, preventing one of them to run. Since it's an easy mistake to make (e.g., copy pasting existing test-cases), I ran following sanity-check script over lldb/test/API, which found couple of tests which were losing coverage because of this (or in some cases simply had duplicate tests):

import ast
import sys

filename = sys.argv[1]
print(f'Checking {filename}...')
tree = ast.parse(open(filename, 'r').read())

for node in ast.walk(tree):
    if not isinstance(node, ast.ClassDef):
        continue

    func_names = []
    for child in ast.iter_child_nodes(node):
        if isinstance(child, ast.FunctionDef):
            func_names.append(child.name)

    seen_func_names = set()
    duplicate_func_names = []
    for name in func_names:
        if name in seen_func_names:
            duplicate_func_names.append(name)
        else:
            seen_func_names.add(name)

    if len(duplicate_func_names) != 0:
        print(f'Multiple func names found:\n\t{duplicate_func_names}\n\tclass {node.name}\n\tfile: {filename}')

This patch fixes these cases.

Full diff: https://github.com/llvm/llvm-project/pull/97043.diff

11 Files Affected:

(modified) lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py (+1-1)
(modified) lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py (-2)
(modified) lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py (-37)
(modified) lldb/test/API/functionalities/completion/TestCompletion.py (-6)
(modified) lldb/test/API/functionalities/gdb_remote_client/TestIOSSimulator.py (+1-1)
(modified) lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py (+1-1)
(modified) lldb/test/API/lang/cpp/diamond/TestCppDiamond.py (+1-1)
(modified) lldb/test/API/lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py (-1)
(modified) lldb/test/API/lang/objcxx/cxx-bridged-po/TestObjCXXBridgedPO.py (+1-1)
(modified) lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py (+1-1)
(modified) lldb/test/API/test_utils/TestDecorators.py (+2-2)

diff --git a/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py b/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
index 583c68d7bfacc..dbcd2d60d021a 100644
--- a/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
+++ b/lldb/test/API/commands/log/invalid-args/TestInvalidArgsLog.py
@@ -25,7 +25,7 @@ def test_disable_empty(self):
         )
 
     @no_debug_info_test
-    def test_enable_empty(self):
+    def test_enable_invalid_path(self):
         invalid_path = os.path.join("this", "is", "not", "a", "valid", "path")
         self.expect(
             "log enable lldb all -f " + invalid_path,
diff --git a/lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py b/lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py
index 9678bd42999b3..571024417560f 100644
--- a/lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py
+++ b/lldb/test/API/commands/thread/backtrace/TestThreadBacktraceRepeat.py
@@ -16,8 +16,6 @@ def test_thread_backtrace_one_thread(self):
         """Run a simplified version of the test that just hits one breakpoint and
         doesn't care about synchronizing the two threads - hopefully this will
         run on more systems."""
-
-    def test_thread_backtrace_one_thread(self):
         self.build()
         (
             self.inferior_target,
diff --git a/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py b/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
index c41e85fd670ba..12f99f07c78a8 100644
--- a/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ b/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -31,43 +31,6 @@ def testStartMultipleLiveThreads(self):
 
         self.traceStopProcess()
 
-    @skipIf(oslist=no_match(["linux"]), archs=no_match(["i386", "x86_64"]))
-    @testSBAPIAndCommands
-    def testStartMultipleLiveThreadsWithStops(self):
-        self.build()
-        exe = self.getBuildArtifact("a.out")
-
-        self.dbg.CreateTarget(exe)
-
-        self.expect("b main")
-        self.expect("b 6")
-        self.expect("b 11")
-
-        self.expect("r")
-        self.traceStartProcess()
-
-        # We'll see here the first thread
-        self.expect("continue")
-
-        # We are in thread 2
-        self.expect("thread trace dump instructions", substrs=["main.cpp:9"])
-        self.expect("thread trace dump instructions 2", substrs=["main.cpp:9"])
-
-        # We stop tracing it
-        self.expect("thread trace stop 2")
-
-        # The trace is still in memory
-        self.expect("thread trace dump instructions 2", substrs=["main.cpp:9"])
-
-        # We'll stop at the next breakpoint, thread 2 will be still alive, but not traced. Thread 3 will be traced
-        self.expect("continue")
-        self.expect("thread trace dump instructions", substrs=["main.cpp:4"])
-        self.expect("thread trace dump instructions 3", substrs=["main.cpp:4"])
-
-        self.expect("thread trace dump instructions 2", substrs=["not traced"])
-
-        self.traceStopProcess()
-
     @skipIf(oslist=no_match(["linux"]), archs=no_match(["i386", "x86_64"]))
     @testSBAPIAndCommands
     def testStartMultipleLiveThreadsWithStops(self):
diff --git a/lldb/test/API/functionalities/completion/TestCompletion.py b/lldb/test/API/functionalities/completion/TestCompletion.py
index d304e12a80c2d..95873405eab84 100644
--- a/lldb/test/API/functionalities/completion/TestCompletion.py
+++ b/lldb/test/API/functionalities/completion/TestCompletion.py
@@ -196,12 +196,6 @@ def test_disassemble_dash_f(self):
     def test_plugin_load(self):
         self.complete_from_to("plugin load ", [])
 
-    def test_log_enable(self):
-        self.complete_from_to("log enable ll", ["lldb"])
-        self.complete_from_to("log enable dw", ["dwarf"])
-        self.complete_from_to("log enable lldb al", ["all"])
-        self.complete_from_to("log enable lldb sym", ["symbol"])
-
     def test_log_enable(self):
         self.complete_from_to("log disable ll", ["lldb"])
         self.complete_from_to("log disable dw", ["dwarf"])
diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestIOSSimulator.py b/lldb/test/API/functionalities/gdb_remote_client/TestIOSSimulator.py
index f45f014016a90..7ea305621010d 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestIOSSimulator.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestIOSSimulator.py
@@ -79,7 +79,7 @@ def test_tvos_sim(self):
         )
 
     @skipIfRemote
-    def test_tvos_sim(self):
+    def test_watchos_sim(self):
         self.platform_test(
             host="macosx",
             process="watchossimulator",
diff --git a/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py b/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
index 859e87cf75a59..e03e8e0df7782 100644
--- a/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
+++ b/lldb/test/API/functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py
@@ -17,7 +17,7 @@ def test_python_os_plugin(self):
         self.build()
         self.run_python_os_funcionality()
 
-    def run_python_os_step(self):
+    def test_run_python_os_step(self):
         """Test that the Python operating system plugin works correctly when single stepping a virtual thread"""
         self.build()
         self.run_python_os_step()
diff --git a/lldb/test/API/lang/cpp/diamond/TestCppDiamond.py b/lldb/test/API/lang/cpp/diamond/TestCppDiamond.py
index 49a4740d0caf0..27062c0666a1a 100644
--- a/lldb/test/API/lang/cpp/diamond/TestCppDiamond.py
+++ b/lldb/test/API/lang/cpp/diamond/TestCppDiamond.py
@@ -109,7 +109,7 @@ def test(self):
 
     @expectedFailureAll
     @no_debug_info_test
-    def test(self):
+    def test_invalid_member(self):
         self.build()
         lldbutil.run_to_source_breakpoint(
             self, "// breakpoint 1", lldb.SBFileSpec("main.cpp")
diff --git a/lldb/test/API/lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py b/lldb/test/API/lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py
index 311a59da0edb2..e2a5af13de6b6 100644
--- a/lldb/test/API/lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py
+++ b/lldb/test/API/lang/cpp/member-and-local-vars-with-same-name/TestMembersAndLocalsWithSameName.py
@@ -3,7 +3,6 @@
 from lldbsuite.test.lldbtest import *
 import lldbsuite.test.lldbutil as lldbutil
 
-
 class TestMembersAndLocalsWithSameName(TestBase):
     def test_when_stopped_in_method(self):
         self._load_exe()
diff --git a/lldb/test/API/lang/objcxx/cxx-bridged-po/TestObjCXXBridgedPO.py b/lldb/test/API/lang/objcxx/cxx-bridged-po/TestObjCXXBridgedPO.py
index 87d8b3d84b8c7..0317d0a4880b1 100644
--- a/lldb/test/API/lang/objcxx/cxx-bridged-po/TestObjCXXBridgedPO.py
+++ b/lldb/test/API/lang/objcxx/cxx-bridged-po/TestObjCXXBridgedPO.py
@@ -7,7 +7,7 @@
 class TestObjCXXBridgedPO(TestBase):
     @skipIfDarwinEmbedded
     @skipIf(macos_version=[">=", "13.0"])
-    def test_bridged_type_po(self):
+    def test_bridged_type_po_old(self):
         self.build()
         lldbutil.run_to_source_breakpoint(
             self, "break here", lldb.SBFileSpec("main.mm")
diff --git a/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py b/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py
index 46f96d89221d5..66df22cd9f99f 100644
--- a/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py
+++ b/lldb/test/API/lang/rust/enum-structs/TestRustEnumStructs.py
@@ -287,7 +287,7 @@ def test_niche_layout_with_fields_3_a(self):
             (137, 1),
         )
 
-    def test_niche_layout_with_fields_3_a(self):
+    def test_niche_layout_with_fields_3_c(self):
         # static NICHE_W_FIELDS_3_C: NicheLayoutWithFields3 = NicheLayoutWithFields3::C(false);
         value = self.getFromGlobal("NICHE_W_FIELDS_3_C").getCurrentValue()
         self.assertEqual(
diff --git a/lldb/test/API/test_utils/TestDecorators.py b/lldb/test/API/test_utils/TestDecorators.py
index eb09db69de349..4810e281c8e79 100644
--- a/lldb/test/API/test_utils/TestDecorators.py
+++ b/lldb/test/API/test_utils/TestDecorators.py
@@ -29,7 +29,7 @@ def test_add_test_categories(self):
         self.assertIsNone(self.getDebugInfo())
 
     @expectedFailureAll
-    def test_xfail_regexp(self):
+    def test_xfail_empty(self):
         """Test that expectedFailureAll can be empty (but please just use expectedFailure)"""
         self.fail()
 
@@ -92,7 +92,7 @@ def test_decorator_skip2(self):
         # self.assertNotEqual(self.getDebugInfo(), "dwarf")
 
     @expectedFailureAll
-    def test_xfail_regexp(self):
+    def test_xfail_empty(self):
         """Test that xfail can be empty"""
         self.fail()

JDevlieghere · 2024-06-28T14:43:28Z

LGTM. This has definitely come up in the past. If you feel motivated, I'm sure there must be a way to detect this issue in Python and we could have assert/warning/error that captures this at the dotest level.

Michael137 · 2024-06-28T17:22:10Z

LGTM. This has definitely come up in the past. If you feel motivated, I'm sure there must be a way to detect this issue in Python and we could have assert/warning/error that captures this at the dotest level.

Agreed, making it part of dotest would be amazing. Maybe someone with better python knowledge has some ideas (@medismailben @kastiglione ?). In the meantime I'll have a think of how one might do that

medismailben · 2024-06-28T18:13:34Z

LGTM. This has definitely come up in the past. If you feel motivated, I'm sure there must be a way to detect this issue in Python and we could have assert/warning/error that captures this at the dotest level.

Agreed, making it part of dotest would be amazing. Maybe someone with better python knowledge has some ideas (@medismailben @kastiglione ?). In the meantime I'll have a think of how one might do that

I think instead of having this be part of our test-suite, I'd run it as part of PR testing in a GitHub action

JDevlieghere · 2024-06-28T18:37:39Z

LGTM. This has definitely come up in the past. If you feel motivated, I'm sure there must be a way to detect this issue in Python and we could have assert/warning/error that captures this at the dotest level.

Agreed, making it part of dotest would be amazing. Maybe someone with better python knowledge has some ideas (@medismailben @kastiglione ?). In the meantime I'll have a think of how one might do that

I think instead of having this be part of our test-suite, I'd run it as part of PR testing in a GitHub action

The test suite would/will run as part of PR testing. Are you saying you would only run it at PR time? Why wait and not find out at your test? We already collect all the test names to run them as separate lit tests (similar but different problem) and I bet we also already iterate over the tests inside a file too to build the variants.

medismailben · 2024-06-28T19:06:22Z

LGTM. This has definitely come up in the past. If you feel motivated, I'm sure there must be a way to detect this issue in Python and we could have assert/warning/error that captures this at the dotest level.

Agreed, making it part of dotest would be amazing. Maybe someone with better python knowledge has some ideas (@medismailben @kastiglione ?). In the meantime I'll have a think of how one might do that

I think instead of having this be part of our test-suite, I'd run it as part of PR testing in a GitHub action

The test suite would/will run as part of PR testing. Are you saying you would only run it at PR time? Why wait and not find out at your test? We already collect all the test names to run them as separate lit tests (similar but different problem) and I bet we also already iterate over the tests inside a file too to build the variants.

I don't mind running as part of the test suite but I think it would be harder to integrate with it rather than running the script standalone as part of the CI. Also I think these categories of errors are more related to linting than actually test failures.

test_run_python_os_step in TestPythonOSPlugin.py fails on Windows after PR #97043. The test passes when run individually using dotest.py. I have marked this skipped for windows to make LLDB AArch64 Windows buildbot happy. a4c1813 https://lab.llvm.org/buildbot/#/builders/141/builds/379

…97043) In one of my recent PRs I mistakenly had two test-cases with the same name, preventing one of them to run. Since it's an easy mistake to make (e.g., copy pasting existing test-cases), I ran following sanity-check script over `lldb/test/API`, which found couple of tests which were losing coverage because of this (or in some cases simply had duplicate tests): ``` import ast import sys filename = sys.argv[1] print(f'Checking {filename}...') tree = ast.parse(open(filename, 'r').read()) for node in ast.walk(tree): if not isinstance(node, ast.ClassDef): continue func_names = [] for child in ast.iter_child_nodes(node): if isinstance(child, ast.FunctionDef): func_names.append(child.name) seen_func_names = set() duplicate_func_names = [] for name in func_names: if name in seen_func_names: duplicate_func_names.append(name) else: seen_func_names.add(name) if len(duplicate_func_names) != 0: print(f'Multiple func names found:\n\t{duplicate_func_names}\n\tclass {node.name}\n\tfile: {filename}') ``` This patch fixes these cases.

test_run_python_os_step in TestPythonOSPlugin.py fails on Windows after PR llvm#97043. The test passes when run individually using dotest.py. I have marked this skipped for windows to make LLDB AArch64 Windows buildbot happy. llvm@a4c1813 https://lab.llvm.org/buildbot/#/builders/141/builds/379

labath · 2024-07-08T08:16:46Z

LGTM. This has definitely come up in the past. If you feel motivated, I'm sure there must be a way to detect this issue in Python and we could have assert/warning/error that captures this at the dotest level.

Agreed, making it part of dotest would be amazing. Maybe someone with better python knowledge has some ideas (@medismailben @kastiglione ?). In the meantime I'll have a think of how one might do that

I think instead of having this be part of our test-suite, I'd run it as part of PR testing in a GitHub action

The test suite would/will run as part of PR testing. Are you saying you would only run it at PR time? Why wait and not find out at your test? We already collect all the test names to run them as separate lit tests (similar but different problem) and I bet we also already iterate over the tests inside a file too to build the variants.

Yes, but we do that on a fully parsed representation of the class, where the name conflicts would not be visible. Michael's script constructs the ast of the python file, and walks that manually. In that sense this check does seem more of like a thing that a linter would do, but I don't know if there are existing linters that would flag this sort of thing..

Michael137 requested review from kastiglione, adrian-prantl, labath and medismailben June 28, 2024 11:53

Michael137 requested a review from JDevlieghere as a code owner June 28, 2024 11:53

llvmbot added the lldb label Jun 28, 2024

fixup! revert redundant whitespace change

d3d54d8

Michael137 requested a review from DavidSpickett June 28, 2024 13:05

JDevlieghere approved these changes Jun 28, 2024

View reviewed changes

Michael137 merged commit a4c1813 into llvm:main Jun 28, 2024
6 checks passed

Michael137 deleted the lldb/duplicate-test-names branch June 28, 2024 19:08

Michael137 mentioned this pull request Sep 20, 2024

[lldb] Add support for disabling frame recognizers #109219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[lldb][test] Remove duplicate testcase names in API test-suite #97043

[lldb][test] Remove duplicate testcase names in API test-suite #97043

Uh oh!

Michael137 commented Jun 28, 2024

Uh oh!

llvmbot commented Jun 28, 2024

Uh oh!

JDevlieghere commented Jun 28, 2024

Uh oh!

Michael137 commented Jun 28, 2024

Uh oh!

medismailben commented Jun 28, 2024

Uh oh!

JDevlieghere commented Jun 28, 2024

Uh oh!

medismailben commented Jun 28, 2024

Uh oh!

Uh oh!

labath commented Jul 8, 2024

Uh oh!

Uh oh!

[lldb][test] Remove duplicate testcase names in API test-suite #97043

[lldb][test] Remove duplicate testcase names in API test-suite #97043

Uh oh!

Conversation

Michael137 commented Jun 28, 2024

Uh oh!

llvmbot commented Jun 28, 2024

Uh oh!

JDevlieghere commented Jun 28, 2024

Uh oh!

Michael137 commented Jun 28, 2024

Uh oh!

medismailben commented Jun 28, 2024

Uh oh!

JDevlieghere commented Jun 28, 2024

Uh oh!

medismailben commented Jun 28, 2024

Uh oh!

Uh oh!

labath commented Jul 8, 2024

Uh oh!

Uh oh!