Page MenuHomePhabricator

Refactor some of the skip / xfail decorators to reuse more code.
ClosedPublic

Authored by zturner on Jan 26 2016, 5:32 PM.

Details

Reviewers
tfiala
labath
Summary

This is going to be part of a larger effort to clean up some of these decorators as the code is getting really unwieldy and difficult to understand and maintain. For now, this patch:

a) Changes the skipIf decorator to be called decorateTest and gives it a mode parameter that says whether we're skipping or xfailing. This allows the skip and xfail decorators to take the same set of arguments and use the same logic for checking the condition so that we only have 1 large and ugly function instead of 2 large and ugly functions that diverge from each other.

b) Update expectedFailureAll and skipIf to call decorateTest, so that all existing code using those decorators go through the common code path, and so that skipIf and expectedFailureAll now support the same set of arguments.

c) Change the way the skip / xfail "reason" is computed to be more friendly.

d) Delete this weird code that says if six.callable(bugnumber) everywhere since it appears to be an invalid assumption about what a bugnumber is (i.e. always a string, never a callable).

Ran the test suite before and after my CL, and the summary statistics at the end are identical in both cases.

In future patches I intend to:

a) Move all the decorators to another file somewhere so they are all isolated and not lost in a sea of 10,000 lines of other code.

b) Remove some of them and consolidate to using the "master" decorators that can are essentially a superset of some of the existing specialized decorators.

c) Add a command line option that causes the test suite to treat skips as xfails.

Diff Detail

Event Timeline

zturner updated this revision to Diff 46085.Jan 26 2016, 5:32 PM
zturner retitled this revision from to Refactor some of the skip / xfail decorators to reuse more code..
zturner updated this object.
zturner added reviewers: tfiala, labath.
zturner added a subscriber: lldb-commits.
zturner updated this revision to Diff 46086.Jan 26 2016, 5:43 PM

Add back the check for six.callable. Also added a detailed comment explaining what this atrocity actually does.

Note that this problem will go away by design once we reduce some of the more superfluous decorators.

labath edited edge metadata.Jan 27 2016, 2:38 AM

I like it. I noticed two small issues when running it though:

  • TestFdLeak calls @expectedFailure, the expectation fn needs to be updated to return a tuple
  • TestInferiorAssert calls matchAndroid, which now returns a tuple. condition needs updating.

LGTM after that.

packages/Python/lldbsuite/test/functionalities/jitloader_gdb/TestJITLoaderGDB.py
19

I'm not sure why I wrote it this way originally, but now it seems this could be written as @skipIf(bugnumber="...") # Skip to avoid crash

Yea so basically what it does is allow you to use the same decorator with
arguments or without arguments. Like this:

@expectedFailureWindows # Python actually calls
expectedFailureWindows(func)
@expectedFailureWindows(debug_info='dwarf') # Python calls
expectedFailureWindows(debug_info='dwarf')(func)

If the goal is to delete all the highly specialized decorators (after all,
expectedFailureWindows is just expectedFailureAll(oslist=['windows'])) then
this problem goes away by itself once everything is using
expectedFailureAll, since that will always be called with arguments.

tfiala accepted this revision.Jan 27 2016, 8:43 AM
tfiala edited edge metadata.

If the goal is to delete all the highly specialized decorators (after all,
expectedFailureWindows is just expectedFailureAll(oslist=['windows'])) then
this problem goes away by itself once everything is using
expectedFailureAll, since that will always be called with arguments.

Okay cool.

LGTM.

This revision is now accepted and ready to land.Jan 27 2016, 8:43 AM

Can this close?

zturner closed this revision.Mar 15 2016, 9:02 AM