[lldb] Don't ignore CLEANUP ERROR results when summarizing dotest results

[teemperor]

Summary

When we try to summarize dotest results into a single result status for lit, "CLEANUP ERROR:" are currently ignored
and we end up just returning "UNSUPPORTED". This changes the behaviour to mark the test as failed in lit.

This is pretty much the same concept as in D75031 (which was a similar bug).

[teemperor]

This isn't ready to merge yet. We currently have a few test failures that are currently just marked as unsupported because of this...

This is the current list on Linux:

Failed Tests (7):
  lldb-api :: commands/target/basic/TestTargetCommand.py
  lldb-api :: functionalities/step_scripted/TestStepScripted.py
  lldb-api :: functionalities/thread_plan/TestThreadPlanCommands.py
  lldb-api :: lang/c/unicode/TestUnicodeSymbols.py
  lldb-api :: tools/lldb-server/signal-filtering/TestGdbRemote_QPassSignals.py
  lldb-api :: tools/lldb-vscode/disconnect/TestVSCode_disconnect.py
  lldb-api :: tools/lldb-vscode/launch/TestVSCode_launch.py

[teemperor]

The current test fail log for that patch is here: https://teemperor.de/pub/CleanupError.log

[labath]

Sounds reasonable, and will probably help with tracking down all the zombies (I guess that's why you're doing it).

If we want to make this first-class failures, then we could probably change the python asserts we added in D81612 back into regular assertions. Speaking of that patch, it looks like a lot of the cleanup errors are due to the assertion added there. One way to deal with this could be to revert temporarily revert that patch until issues it uncovered are resolved.

[JDevlieghere]

In D83865#2153055, @labath wrote:

Sounds reasonable, and will probably help with tracking down all the zombies (I guess that's why you're doing it).

Not really, but I'm curious to hear how this could help.

If we want to make this first-class failures, then we could probably change the python asserts we added in D81612 back into regular assertions.

That doesn't make a difference, I tried that when landing the original patch. Both are swallowed by unittest2. I tried to achieve the same result as this patch by making the test fail when the check if false, but as you can tell from the output (CLEANUP ERROR shows up as this weird extra test/line) the test is considered complete by the time the tearDown happens .

[teemperor]

In D83865#2153390, @JDevlieghere wrote:

In D83865#2153055, @labath wrote:

If we want to make this first-class failures, then we could probably change the python asserts we added in D81612 back into regular assertions.

That doesn't make a difference, I tried that when landing the original patch. Both are swallowed by unittest2. I tried to achieve the same result as this patch by making the test fail when the check if false, but as you can tell from the output (CLEANUP ERROR shows up as this weird extra test/line) the test is considered complete by the time the tearDown happens .

That's right. I did change it though in D83876 because it improves the error message ( D83874 adds support for printing the backtrace, so we actually see whatever the assertions fails with).

[labath]

In D83865#2153390, @JDevlieghere wrote:

In D83865#2153055, @labath wrote:

Sounds reasonable, and will probably help with tracking down all the zombies (I guess that's why you're doing it).

Not really, but I'm curious to hear how this could help.

An exception being thrown during cleanup will cause the rest of the cleanup code to be skipped. That could result in some processes not being killed, for instance.

If we want to make this first-class failures, then we could probably change the python asserts we added in D81612 back into regular assertions.

That doesn't make a difference, I tried that when landing the original patch. Both are swallowed by unittest2. I tried to achieve the same result as this patch by making the test fail when the check if false, but as you can tell from the output (CLEANUP ERROR shows up as this weird extra test/line) the test is considered complete by the time the tearDown happens .

"CLEANUP ERROR" is not "FAILURE" but it's not "SUCCESS" either. We can't make it a FAILURE (well, maybe we could but it would be a hack), but we could change dotest.py to exit with a non-zero error code in case of cleanup failures. That would at least cause both lit and lldb-dotest to flag this situation as erroneous.

(In fact if we make dotest do that, then maybe we don't need to change this file...)

[JDevlieghere]

In D83865#2153417, @labath wrote:

In D83865#2153390, @JDevlieghere wrote:

In D83865#2153055, @labath wrote:

Sounds reasonable, and will probably help with tracking down all the zombies (I guess that's why you're doing it).

Not really, but I'm curious to hear how this could help.

An exception being thrown during cleanup will cause the rest of the cleanup code to be skipped. That could result in some processes not being killed, for instance.

That's what I figured, I read too much into the original wording.

If we want to make this first-class failures, then we could probably change the python asserts we added in D81612 back into regular assertions.

That doesn't make a difference, I tried that when landing the original patch. Both are swallowed by unittest2. I tried to achieve the same result as this patch by making the test fail when the check if false, but as you can tell from the output (CLEANUP ERROR shows up as this weird extra test/line) the test is considered complete by the time the tearDown happens .

"CLEANUP ERROR" is not "FAILURE" but it's not "SUCCESS" either. We can't make it a FAILURE (well, maybe we could but it would be a hack), but we could change dotest.py to exit with a non-zero error code in case of cleanup failures. That would at least cause both lit and lldb-dotest to flag this situation as erroneous.

(In fact if we make dotest do that, then maybe we don't need to change this file...)

If we can achieve that without making changes to unittest2 then I'd prefer that.

[labath]

In D83865#2153577, @JDevlieghere wrote:

In D83865#2153417, @labath wrote:

"CLEANUP ERROR" is not "FAILURE" but it's not "SUCCESS" either. We can't make it a FAILURE (well, maybe we could but it would be a hack), but we could change dotest.py to exit with a non-zero error code in case of cleanup failures. That would at least cause both lit and lldb-dotest to flag this situation as erroneous.

(In fact if we make dotest do that, then maybe we don't need to change this file...)

If we can achieve that without making changes to unittest2 then I'd prefer that.

That should be possible. We control both the exiting code and the test result listener.

lldb/test/API/lldbtest.py

def execute(self, test, litConfig):
 
         if exitCode:
             if 'XPASS:' in out or 'XPASS:' in err:
                 return lit.Test.XPASS, output
 
             # Otherwise this is just a failure.
             return lit.Test.FAIL, output
 
+        has_cleanup_err_tests = 'CLEANUP ERROR:' in out or 'CLEANUP ERROR:' in err
+        if has_cleanup_err_tests:
+            return lit.Test.FAIL, output
+
         has_unsupported_tests = 'UNSUPPORTED:' in out or 'UNSUPPORTED:' in err
         has_passing_tests = 'PASS:' in out or 'PASS:' in err
         if has_unsupported_tests and not has_passing_tests:
             return lit.Test.UNSUPPORTED, output
 
         passing_test_line = 'RESULT: PASSED'
         if passing_test_line not in out and passing_test_line not in err:
             return lit.Test.UNRESOLVED, output
 
         return lit.Test.PASS, output