This is an archive of the discontinued LLVM Phabricator instance.

Make many of the skip decorators use common code
ClosedPublic

Authored by zturner on Jan 29 2016, 4:09 PM.

Details

Summary

Trying to reduce the complexity of all our decorators, I decided to make most of them use skipTestIfFn. Unfortunately there's some issues with my patch. I'm not really an expert with all these decorators and multiple levels of decorating, but something about that is causing some issues.

For example, I get this now when running the test suite:

UNEXPECTED SUCCESS: test_Hc_then_Csignal_signals_correct_thread_launch_debugserver_dwarf (tools\lldb-server\TestLldbGdbServer.py)
UNEXPECTED SUCCESS: test_with_attach_to_process_with_id_api_dwarf (python_api\hello_world\TestHelloWorld.py)
UNEXPECTED SUCCESS: test_with_attach_to_process_with_name_api_dwarf (python_api\hello_world\TestHelloWorld.py)

But the tests are still failing. Would someone mind helping me figure out what's wrong here?

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 46449.Jan 29 2016, 4:09 PM
zturner retitled this revision from to Make many of the skip decorators use common code.
zturner updated this object.
zturner added reviewers: labath, tberghammer, tfiala.
zturner added a subscriber: lldb-commits.
labath edited edge metadata.Feb 1 2016, 2:43 AM

I am confused as to how could this even work, since all your decorators seem to be ignoring the function they are decorating. I'm pretty sure this is going to cause some very weird things to happen things to come out of the decoration process, which might explain your confusion. Try giving it a go by passing the function explicitly.

packages/Python/lldbsuite/test/lldbtest.py
560 ↗(On Diff #46449)

Please don't remove these. They make sure that you do not attempt to apply this decorator to a class. If the decorator is not ready for that (and the current skipIfTestFn does not appear to be), it will just cause the whole cause the whole class to be silently ignored (we've had this happen before).

I think it's ok to centralize the check in skipTestIfFn.

562 ↗(On Diff #46449)

You have dropped "func" completely here. Should this be skipTestIfFn(should_skip_debugserver_tests, func)? (Same goes for other decorators.)

zturner marked an inline comment as done.Feb 1 2016, 4:00 PM
zturner added inline comments.
packages/Python/lldbsuite/test/lldbtest.py
560 ↗(On Diff #46449)

Why don't we want this behavior? With this check, an exception is being thrown if the condition is true. Currently the test suite is not failing anywhere with this exception, meaning the check is never true. And it would only be true if we put the decorator on the actual class. Which we already do in some places (see TestAddDsymCommand.py) and is actually a very useful way of skipping or xfailing an entire class. I want to be able to skip / xfail entire classes *more* easily, not less easily.

zturner marked an inline comment as done.Feb 1 2016, 4:11 PM

I guess we need to be consistent. Do we want to be able to decorate entire classes, or not? If we do, then we should remove the check. If we don't then we should leave the check. But since many tests are already using these decorators at the class level (TestEvents.py, TestAddDsymCommand.py, TestObjCMethods2.py, TestLoadUnload.py, and many more) those decorators cannot yet be ported to using common code until we decide which we want.

zturner updated this revision to Diff 46599.Feb 1 2016, 5:17 PM
zturner edited edge metadata.

I think this should fix most of the issues. A few of the decorators I wasn't able to touch because of the issues with class-level decorators. But I think this is a good start. My test statistics before and after this patch are the same.

If someone else could run test suite before and after this patch and let me know how the numbers look on your platform I would appreciate it.

Suggestions welcome.

tfiala edited edge metadata.Feb 1 2016, 7:12 PM
tfiala added a subscriber: tfiala.

I'll see if I can run this tonight. If not tonight, I'll do it first
thing in the morning.

labath added a comment.Feb 2 2016, 4:52 AM

LGTM from my side after the two fixes in no_debug_info_test and skipUnlessListedRemote

packages/Python/lldbsuite/test/lldbtest.py
555 ↗(On Diff #46599)

This will not cause the class to be skipped, but it will still not achive the desired effect when used on a class. Please leave the safeguard in.

I like the simplification of the wrapper btw.

560 ↗(On Diff #46599)

I would very much welcome this behavior, however not all of our decorators are ready for that. The point is not that the class would be skipped if the condition is true (this is what we want), the problem is that the class would be skipped even when the condition is false. So, putting a wrong kind of a decorator on a class causes the whole class to be ignored (because what the decorator returns is not a python class, which means unittest cannot find the test methods inside), and reduces the test coverage for everyone. We've already had a case when a test wasn't running for several months before I noticed (see r257901) that it had a bad decorator applied at the class level.

The test you mention, TestAddDsymCommand.py, is fine because it eventually calls into unittest2.skipUnless, which knows how to handle this case properly, but skipTestIfFn is bad because it returns a function-like object instead of a class. Also, all our XFAIL decorators are bad, and afaik not even unittest has the support for xfail at class level.

So if you can make the decorators work properly on classes (in case of skip, you probably just need to route everything to unittest.skipIf, but it's not going to be that trivial in the case of xfails), I am all for using them, but until then, the exception safeguard needs to stay.

824 ↗(On Diff #46599)

Actually, the original behavior of the decorator was to *not* skip the test when the test is *not* remote, so please put False here for now.
However, I find the whole decorator weird and I'll get rid of it completely after you are done here.

1102 ↗(On Diff #46599)

This return statement is the root cause of the problem. If func is a class, you will replace it by a strange function-like object.

zturner added inline comments.Feb 2 2016, 7:58 AM
packages/Python/lldbsuite/test/lldbtest.py
1102 ↗(On Diff #46599)

This return statement is newly added anyway (and looks to be a mistake). Does this mean if I remove the return statement, the all of the skip decorators will be able to be used at class level?

tfiala added a comment.EditedFeb 2 2016, 8:02 AM

We've already had a case when a test wasn't running for several months
before I noticed (see r257901) that it had a bad decorator applied at the
class level.

I've been thinking about this in the background lately. Several times over
the last couple years I found decorator logic that had errors in them
(literally, the code was throwing an exception within the decorator logic,
such as using a module that isn't imported), but this will go by silently
unless some work is put into logging errors there.

If we haven't already, we should probably have some kind of exception
wrapper around our decorators that catches non-unittest-related (i.e.
unexpected) exceptions and somehow makes them more prevalent - maybe a hard
error on the test or an abort or something.

labath added a comment.Feb 2 2016, 8:21 AM

If we haven't already, we should probably have some kind of exception
wrapper around our decorators that catches non-unittest-related (i.e.
unexpected) exceptions and somehow makes them more prevalent - maybe a hard
error on the test or an abort or something.

+1

In the new test runner, the raise Exception thing will not actually make the test as a whole fail, but at least it will generate a lot of noise in the output, which will hopefully make someone notice it, so I guess it's better than nothing...

packages/Python/lldbsuite/test/lldbtest.py
1102 ↗(On Diff #46599)

I'm not exactly sure what you mean. You can't simply remove the return statement, as that would make the decorator not work. I would try to make this decorator call unittest2.skipIf, which knows how to do the right thing. So, something like

def skipTestIfFn(expected_fn, bugnumber=None):
  skip, reason = ...
  return unittest2.skipIf(skip, reason)

would work (I think), but you could run into some complications as now expected_fn gets evaluated at decorator application time rather than during test invocation.

You can try this out for your self. Remove the exception guard, apply the decorator on a class with the condition that is false, and make sure that the tests *do* run.

But maybe we should do that as a separate patch ?

If we haven't already, we should probably have some kind of exception wrapper around our decorators that catches non-unittest-related (i.e. unexpected) exceptions and somehow makes them more prevalent - maybe a hard error on the test or an abort or something.

Yea, part of why I wanted to do this work in the first place is because I want to make some sweeping changes to all decorators as well, and it's impossible when everything is calling directly into unittest2 or just doing its own thing in whatever way. So having the code all in one place makes this very easy.

packages/Python/lldbsuite/test/lldbtest.py
1102 ↗(On Diff #46599)

Doing it in a separate patch makes sense. In any case, my point here is that this code was already not returning the result of calling the function (just look at the left side of the diff). I think when we get here func is always the underlying function (i.e. the actual test function), and those don't return anything, they just run the test.

I would have to step through it in a debugger to really be sure though, the whole decorator model is still kind of hard to wrap my head around with the numerous layers of nested functions.

labath added inline comments.Feb 2 2016, 9:16 AM
packages/Python/lldbsuite/test/lldbtest.py
1102 ↗(On Diff #46599)

Ok, I see what you mean now. The return in return func(...) is probably not needed, but it definitely doesn't hurt either. The problem is one level up, in skipTestIfFn_impl. Whatever is the result of skipTestIfFn_impl(X) gets used instead of the real X. So if X is the whole class then you must return something that looks like a class there. Alas, we are returning wrapper, which is a function, and things go downhill from there...

This revision was automatically updated to reflect the committed changes.
zturner marked 3 inline comments as done.