Page MenuHomePhabricator

Improvements to testing blacklist
ClosedPublic

Authored by fjricci on Sep 27 2016, 2:50 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

fjricci updated this revision to Diff 72715.Sep 27 2016, 2:50 PM
fjricci retitled this revision from to Improvements to testing blacklist.
fjricci updated this object.
fjricci added reviewers: zturner, labath, tfiala, jingham.
fjricci added subscribers: sas, lldb-commits.

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 edited edge metadata.EditedOct 3 2016, 10:22 AM

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!

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

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 added a comment.EditedOct 3 2016, 2:21 PM

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).

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 updated this revision to Diff 73362.Oct 3 2016, 3:41 PM
fjricci edited edge metadata.

Match against filename + test case + method name

fjricci updated this revision to Diff 73363.Oct 3 2016, 3:43 PM

Fix typo

tfiala accepted this revision.EditedOct 3 2016, 3:51 PM
tfiala edited edge metadata.

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

Thanks! LGTM.

This revision is now accepted and ready to land.Oct 3 2016, 3:51 PM
tfiala added a comment.Oct 3 2016, 3:59 PM

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.

This revision was automatically updated to reflect the committed changes.