Improvements to testing blacklist

[fjricci]

Summary

This patch is necessary because individual test cases are not required
to have unique names. Therefore, test cases must now
be specified explicitly in the form <TestCase>.<TestMethod>.
Because it works by regex matching, passing just <TestCase> will
still disable an entire file.

This also allows for multiple exclusion files to be specified.

[fjricci]

Since this is strictly an improvement and simplification, and won't break anyone's workflow because it's still a new feature, I'll plan on merging this tomorrow unless I hear any objections.

[tfiala]

Hey @fjricci ,

What is the motivation for this change? It looks like the existing code works based on file names, which are required to be unique in the system. It looks like you're attempting to move it over to a classname.method scheme. Is that right? If so, classname.method is not guaranteed to be unique, either. It would have to include the module name, which is essentially the file name, which is required to be unique. (Maybe when you said <TestCase>.<TestMethod>, your <TestCase> element is fully qualified, including the module name? If so, then that should be guaranteed to be unique).

All the same, I'd like to better understand what you're looking to accomplish, since what I'm reading in the code seems to be doing the opposite of what I think you're trying to do. (I'm probably just misunderstanding).

Thanks!

[fjricci]

The problem with the existing code is that file names are required to be unique, but method names are not. So if the user wants to disable an individual test method with a non-unique name, there is no way to do so. This patch still allows the tests to be disabled by file name, but removes the ability to disable by only method name, instead requiring the method name as a modifier to the file.

I may have used the wrong terminology when I said <TestCase>.<TestMethod>. Here, I meant essentially what is printed by the test runner when a test fails. So, for example:

BadAddressBreakpointTestCase.test_bad_address_breakpoints_dwarf
LldbGdbServerTestCase.test_Hc_then_Csignal_signals_correct_thread_launch_llgs

These names are guaranteed to be unique, and so I think they're the better way to go. The user can still disable an entire file, by passing something something like:

TestConcurrentEvents

[fjricci]

For an example of something that couldn't be disabled with the original implementation, consider a test like:

CreateDuringStepTestCase.test_step_inst

Disabling by method name (test_step_inst) would also disable CreateDuringInstructionStepTestCase.test_step_inst.

[tfiala]

In D24988#559314, @fjricci wrote:

For an example of something that couldn't be disabled with the original implementation, consider a test like:

CreateDuringStepTestCase.test_step_inst

Disabling by method name (test_step_inst) would also disable CreateDuringInstructionStepTestCase.test_step_inst.

I see what you're saying there.

The part you're missing is that the Test Case class name itself does not have to be unique, either. i.e. You *can* have two CreateDuringStepTestCase classes in different files. Nothing uniquifies at that level.

You can convince yourself of this by doing what I just did:

$ cd packages/Python/lldbsuite/test
$ cp -r driver/batch_mode deleteme
\# now you have tests/deleteme with contents of batch_mode
$ cd deleteme
$ cp TestBatchMode.py TestBatchMode2.py
\# Now you have two files with the same exact contents, both of which will run just fine. i.e. you're still not unique with test case names - they can be the same.

\# edit Makefile to fix the redirection back to the master makefile rules

\# run the test suite and tell it to run deleteme dir
test/dotest.py --test-subdir deleteme

You'll get results like this:

$ test/dotest.py --executable `pwd`/build/Debug/lldb --test-subdir deleteme
Testing: 2 test suites, 8 threads
2 out of 2 test suites processed - TestBatchMode2.py
===================
Test Result Summary
===================
Test Methods:         18
Reruns:                0
Success:              18
Expected Failure:      0
Failure:               0
Error:                 0
Exceptional Exit:      0
Unexpected Success:    0
Skip:                  0
Timeout:               0
Expected Timeout:      0

That is why I'm saying you need to include the module name, which comes from the filename, or have it be something like FileBaseName:TestCase.test_method. I have to do this in the test runner architecture for this very reason. And you will find at least some test cases that are cut and pasted and therefore have duplicate test case names (at least, they used to exist, and nothing enforces them being different in the runner logic).

[fjricci]

In D24988#559775, @tfiala wrote:

In D24988#559314, @fjricci wrote:

For an example of something that couldn't be disabled with the original implementation, consider a test like:

CreateDuringStepTestCase.test_step_inst

Disabling by method name (test_step_inst) would also disable CreateDuringInstructionStepTestCase.test_step_inst.

I see what you're saying there.

The part you're missing is that the Test Case class name itself does not have to be unique, either. i.e. You *can* have two CreateDuringStepTestCase classes in different files. Nothing uniquifies at that level.

Ahh, I see. I didn't realize that we could have duplication in the test case names as well.

That is why I'm saying you need to include the module name, which comes from the filename, or have it be something like FileBaseName:TestCase.test_method. I have to do this in the test runner architecture for this very reason. And you will find at least some test cases that are cut and pasted and therefore have duplicate test case names (at least, they used to exist, and nothing enforces them being different in the runner logic).

I'll try this then.

[fjricci]

Match against filename + test case + method name

[fjricci]

Fix typo

[tfiala]

Much better. I see you found test.id(), which gets the module.class.test_method setup. That will be unique.

Thanks! LGTM.

[tfiala]

I also just added a test case to validate we get a non-None answer to test instance self.id() calls. We use it in several places, so might as well make that explicit.

That went in as r283156.

packages/Python/lldbsuite/test/configuration.py

 # prints machine-readable output similar to what clang tests produce.
 parsable = False
 
 # The regular expression pattern to match against eligible filenames as
 # our test cases.
 regexp = None
 
 # Sets of tests which are excluded at runtime
-skip_files = None
-skip_methods = None
-xfail_files = None
-xfail_methods = None
+skip_tests = None
+xfail_tests = None
 
 # By default, recorded session info for errored/failed test are dumped into its
 # own file under a session directory named after the timestamp of the test suite
 # run.  Use '-s session-dir-name' to specify a specific dir name.
 sdir_name = None
 
 # Valid options:
 # f - test file name (without extension)

packages/Python/lldbsuite/test/dotest.py

"""Parse an exclusion file, of the following format, where
        skip files
        <file name>
        <file name>
 
        xfail methods
        <method name>
     """
     excl_type = None
-    case_type = None
 
     with open(exclusion_file) as f:
         for line in f:
+            line = line.strip()
             if not excl_type:
-                [excl_type, case_type] = line.split()
+                excl_type = line
                 continue
 
-            line = line.strip()
             if not line:
                 excl_type = None
-            elif excl_type == 'skip' and case_type == 'files':
-                if not configuration.skip_files:
-                    configuration.skip_files = []
-                configuration.skip_files.append(line)
-            elif excl_type == 'skip' and case_type == 'methods':
-                if not configuration.skip_methods:
-                    configuration.skip_methods = []
-                configuration.skip_methods.append(line)
-            elif excl_type == 'xfail' and case_type == 'files':
-                if not configuration.xfail_files:
-                    configuration.xfail_files = []
-                configuration.xfail_files.append(line)
-            elif excl_type == 'xfail' and case_type == 'methods':
-                if not configuration.xfail_methods:
-                    configuration.xfail_methods = []
-                configuration.xfail_methods.append(line)
+            elif excl_type == 'skip':
+                if not configuration.skip_tests:
+                    configuration.skip_tests = []
+                configuration.skip_tests.append(line)
+            elif excl_type == 'xfail':
+                if not configuration.xfail_tests:
+                    configuration.xfail_tests = []
+                configuration.xfail_tests.append(line)
 
 
 def parseOptionsAndInitTestdirs():
     """Initialize the list of directories containing our unittest scripts.
 
     '-h/--help as the first option prints out usage info and exit the program.
     """
 
def parseOptionsAndInitTestdirs():
 
     if args.framework:
         configuration.lldbFrameworkPath = args.framework
 
     if args.executable:
         lldbtest_config.lldbExec = os.path.realpath(args.executable)
 
     if args.excluded:
-        parseExclusion(args.excluded)
+        for excl_file in args.excluded:
+            parseExclusion(excl_file)
 
     if args.p:
         if args.p.startswith('-'):
             usage(parser)
         configuration.regexp = args.p
 
     if args.q:
         configuration.parsable = True

 
 def visit_file(dir, name):
     # Try to match the regexp pattern, if specified.
     if configuration.regexp:
         if not re.search(configuration.regexp, name):
             # We didn't match the regex, we're done.
             return
 
-    if configuration.skip_files:
-        for file_regexp in configuration.skip_files:
+    if configuration.skip_tests:
+        for file_regexp in configuration.skip_tests:
             if re.search(file_regexp, name):
                 return
 
     # We found a match for our test.  Add it to the suite.
 
     # Update the sys.path first.
     if not sys.path.count(dir):
         sys.path.insert(0, dir)

packages/Python/lldbsuite/test/dotest_args.py

group.add_argument(
         metavar='filterspec',
         action='append',
         help='Specify a filter, which consists of the test class name, a dot, followed by the test method, to only admit such test into the test suite')  # FIXME: Example?
     X('-l', "Don't skip long running tests")
     group.add_argument(
         '-p',
         metavar='pattern',
         help='Specify a regexp filename pattern for inclusion in the test suite')
-    group.add_argument('--excluded', metavar='exclusion-file', help=textwrap.dedent(
+    group.add_argument('--excluded', metavar='exclusion-file', action='append', help=textwrap.dedent(
         '''Specify a file for tests to exclude. File should contain lists of regular expressions for test files or methods,
                                 with each list under a matching header (xfail files, xfail methods, skip files, skip methods)'''))
     group.add_argument(
         '-G',
         '--category',
         metavar='category',
         action='append',
         dest='categoriesList',

packages/Python/lldbsuite/test/test_result.py

def checkExclusion(self, exclusion_list, name):
                     return True
         return False
 
     def startTest(self, test):
         if configuration.shouldSkipBecauseOfCategories(
                 self.getCategoriesForTest(test)):
             self.hardMarkAsSkipped(test)
         if self.checkExclusion(
-                configuration.skip_methods,
-                test._testMethodName):
+                configuration.skip_tests,
+                strclass(test.__class__) + '.' + test._testMethodName):
             self.hardMarkAsSkipped(test)
 
         configuration.setCrashInfoHook(
             "%s at %s" %
             (str(test), inspect.getfile(
                 test.__class__)))
         self.counter += 1
         # if self.counter == 4:
         #    import crashinfo
         #    crashinfo.testCrashReporterDescription(None)
         test.test_number = self.counter
         if self.showAll:
             self.stream.write(self.fmt % self.counter)
         super(LLDBTestResult, self).startTest(test)
         if self.results_formatter:
             self.results_formatter.handle_event(
                 EventBuilder.event_for_start(test))
 
     def addSuccess(self, test):
         if self.checkExclusion(
-                configuration.xfail_files,
-                strclass(
-                    test.__class__)) or self.checkExclusion(
-                configuration.xfail_methods,
-                test._testMethodName):
+                configuration.xfail_tests,
+                strclass(test.__class__) + '.' + test._testMethodName):
             self.addUnexpectedSuccess(test, None)
             return
 
         super(LLDBTestResult, self).addSuccess(test)
         if configuration.parsable:
             self.stream.write(
                 "PASS: LLDB (%s) :: %s\n" %
                 (self._config_string(test), str(test)))
def addCleanupError(self, test, err):
                 (self._config_string(test), str(test)))
         if self.results_formatter:
             self.results_formatter.handle_event(
                 EventBuilder.event_for_cleanup_error(
                     test, err))
 
     def addFailure(self, test, err):
         if self.checkExclusion(
-                configuration.xfail_files,
-                strclass(
-                    test.__class__)) or self.checkExclusion(
-                configuration.xfail_methods,
-                test._testMethodName):
+                configuration.xfail_tests,
+                strclass(test.__class__) + '.' + test._testMethodName):
             self.addExpectedFailure(test, err, None)
             return
 
         configuration.sdir_has_content = True
         super(LLDBTestResult, self).addFailure(test, err)
         method = getattr(test, "markFailure", None)
         if method:
             method()