Page MenuHomePhabricator

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

Authored by teemperor on Jul 15 2020, 5:09 AM.

Details

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

Diff Detail

Event Timeline

teemperor created this revision.Jul 15 2020, 5:09 AM

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

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

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.

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 .

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

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

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.

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