[Lldb-commits] [PATCH] D81697: Add support for batch-testing to the LLDB testsuite.

Thu Jul 2 08:38:02 PDT 2020

labath added a comment.

In D81697#2127061 <https://reviews.llvm.org/D81697#2127061>, @bbli wrote:

> Hi, bumping my post from two weeks ago. The main question I had was: would it be ok if I just brought over the Outcome object for the time being?

Umm... I don't know... Maybe? I suppose it depends on how invasive the change would be... If you're willing to give it a try, I am willing to look at the result, but I gotta warn you that I am pretty averse (and I know some of the other contributors are too) about modifying imported third-party code.

OTOH, if you're just looking to do something, I can think of other helpful fixes that would be a lot less controversial. For example, the thing that's annoying me now is that whenever a test fails in the "make" phase (e.g. a compile error), we only get an error message with the return status, and the original make invocation. It would be very helpful if that error could include the entire make output, including all the compile commands that it ran. In my book that would be more helpful (and a lot easier) than the batch mode.

I'd also be happy to help you with attempting to migrate to a newer unittest framework, if you're willing to give that a shot. Even if we don't end up doing it, I think just tracking down the original version this was forked from, and enumerating our changes to it (and their reasons) would be helpful.

In D81697#2091220 <https://reviews.llvm.org/D81697#2091220>, @bbli wrote:

> - Also, `testPartExecutor` will catch all exceptions, but I believe we want to exit early and propagate non `self.failureException` errors all the way up for `runMethod` to handle, correct?

I don't have a strong opinion on that and I'm happy to go with whatever the upstream version does. In theory a different exception should not mean a failure, but a problem in the test itself, but in practice it usually does not work out that way -- a lot of failures manifest themselves `AttributeError: 'NoneType' object has no attribute 'foo'` errors (because an object ended up being null when it shouldn't be) or similar. And I'm not sure that littering the test with self.assertNotNone(foo) is worth it...

> - Finally, by `expect_expr`, are you referring to the "expect*" methods that are defined in lldbtest.py? And are you suggesting to have the error handling context manager to be called inside these functions? Because wouldn't that lead to the same problem of having a function continue in the situation where the function has multiple `expect_expr` commands?

I was specifically referring to the `expect_expr` method, not the entire expect function family. You're right that this would mean that the caller of `expect_expr` would continue in its work, but what I wanted to do is make this an official part of the `expect_expr` contract. `expect_expr` is a fairly new function, and the way we currently use that is just to inspect values of various objects. There's no harm in batching those. We could just add some wording to its documentation to say that one should not use that function to mutate state as that could lead to very confusing error messages if that command fails.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81697/new/

https://reviews.llvm.org/D81697