Page MenuHomePhabricator

Remove expectedFailureWindows decorator
ClosedPublic

Authored by zturner on Feb 5 2016, 2:49 PM.

Details

Summary

Putting this up for review not because you guys are interested in windows, but just because I'm hoping to do this with more of the decorators. Assuming nobody has any issues with this, I will do the rest without review.

expectedFailureWindows is equivalent to using the general
expectedFailureAll decorator with oslist="windows".  Additionally,
by moving towards these common decorators we can solve the issue
of having to support decorators that can be called with or without
arguments.  Once all decorators are always called with arguments,
and this is enforced by design (because you can't specify the condition
you're decorating for without passing an argument) the implementation
of the decorators can become much simpler, which in turn will lead to
less difficulties when moving from `unittest2` to `unittest` as well as supporting
class level decorators for skip as well as xfail on all decorators.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 47052.Feb 5 2016, 2:49 PM
zturner retitled this revision from to Remove expectedFailureWindows decorator.
zturner updated this object.
zturner added reviewers: tfiala, labath.
zturner added a subscriber: lldb-commits.

One more thing. This patch also causes skips and xfails to include the bugnumber as part of the reason. So you can see bugnumbers on summary output. See the changes to decorators.py for the relevant code.

tfiala edited edge metadata.Feb 5 2016, 7:09 PM

I'll go ahead and run this before and after and compare totals.

tfiala added a comment.Feb 7 2016, 9:16 PM

I'm seeing 8 errors after applying this diff:

ERROR: test_expr_stripped_dwarf (lang/objc/hidden-ivars/TestHiddenIvars.py)
ERROR: test_frame_variable_stripped_dwarf (lang/objc/hidden-ivars/TestHiddenIvars.py)
ERROR: test_with_python_api_dsym (lang/cpp/class_static/TestStaticVariables.py)
ERROR: test_with_python_api_dwarf (lang/objc/objc-static-method-stripped/TestObjCStaticMethodStripped.py)
ERROR: test_with_python_api_dwarf (lang/objc/objc-ivar-stripped/TestObjCIvarStripped.py)
ERROR: test_with_python_api_dwarf (lang/cpp/class_static/TestStaticVariables.py)
ERROR: test_with_run_command_dsym (lang/cpp/scope/TestCppScope.py)
ERROR: test_with_run_command_dwarf (lang/cpp/scope/TestCppScope.py)

I'll have a peek and see what they say.

tfiala added a comment.Feb 7 2016, 9:22 PM

Here's one:

Traceback (most recent call last):
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1442, in dsym_test_method
    return attrvalue(self)
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/decorators.py", line 74, in wrapper
    xfail_reason = expected_fn(self)
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/decorators.py", line 171, in fn
    reason_str = reason_str + " [" + bugnumber + "]"
TypeError: cannot concatenate 'str' and 'function' objects

Here's another:

Traceback (most recent call last):
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1442, in dsym_test_method
    return attrvalue(self)
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/decorators.py", line 74, in wrapper
    xfail_reason = expected_fn(self)
  File "/Users/tfiala/src/lldb-tot/lldb/packages/Python/lldbsuite/test/decorators.py", line 171, in fn
    reason_str = reason_str + " [" + bugnumber + "]"
TypeError: cannot concatenate 'str' and 'int' objects
Config=x86_64-/usr/bin/clang
zturner added inline comments.Feb 7 2016, 9:27 PM
packages/Python/lldbsuite/test/decorators.py
170–171 ↗(On Diff #47052)

Can you try changing this to this and see what happens:

if bugnumber is not None and not six.callable(bugnumber):
    reason_str = reason_str + " [" + str(bugnumber) + "]"
tfiala added a comment.Feb 7 2016, 9:47 PM

Yep, I'll give that a shot now.

tfiala accepted this revision.Feb 7 2016, 10:06 PM
tfiala edited edge metadata.

LGTM with the change you suggested above. I tried that and it worked on OS X. The change itself looked fine as well.

packages/Python/lldbsuite/test/decorators.py
170–171 ↗(On Diff #47052)

Yep, that fixed the errors.

This revision is now accepted and ready to land.Feb 7 2016, 10:06 PM
labath accepted this revision.Feb 8 2016, 2:40 AM
labath edited edge metadata.

I agree with the idea in general, but I wanted to ask what is your plan with the android decorators: For them we use the additional api_levels flag, which does not exist on other platforms/decorators. I suppose we could add that flag to expectedFailureAll, but I am not sure if that would be a good idea...

Also, since we are doing all this refactoring, one more improvement I can think of is renaming expectedFailureAll to expectedFailure. It was named All because we already have an expectedFailure function, but I think that one is now more of an implementation detail and could be renamed to something else. Up to you...

I agree with the idea in general, but I wanted to ask what is your plan with the android decorators: For them we use the additional api_levels flag, which does not exist on other platforms/decorators. I suppose we could add that flag to expectedFailureAll, but I am not sure if that would be a good idea...

Also, since we are doing all this refactoring, one more improvement I can think of is renaming expectedFailureAll to expectedFailure. It was named All because we already have an expectedFailure function, but I think that one is now more of an implementation detail and could be renamed to something else. Up to you...

My suggestion for the android API level is to add an argument to expectedFailure where you can specify an arbitrary function and then we can write a function called android_device_matches(...) what will return a function checking for the API level. Then this can be used to create very specific xfail conditions what are checking some property of the target system (e.g. "@expectedFailure(fn=hardwareWatchpointsNotSupported)")

Something like this:

def android_device_matches(apis):
    def impl(apis):
        return get_device_api() in apis
    return impl

I agree with the idea in general, but I wanted to ask what is your plan with the android decorators: For them we use the additional api_levels flag, which does not exist on other platforms/decorators. I suppose we could add that flag to expectedFailureAll, but I am not sure if that would be a good idea...

Also, since we are doing all this refactoring, one more improvement I can think of is renaming expectedFailureAll to expectedFailure. It was named All because we already have an expectedFailure function, but I think that one is now more of an implementation detail and could be renamed to something else. Up to you...

My suggestion for the android API level is to add an argument to expectedFailure where you can specify an arbitrary function and then we can write a function called android_device_matches(...) what will return a function checking for the API level. Then this can be used to create very specific xfail conditions what are checking some property of the target system (e.g. "@expectedFailure(fn=hardwareWatchpointsNotSupported)")

Yes, that's one option I thought of. And the function could be specified with any combination of other parameters at the same time, and the result of the function would just be one value checked in determining whether to skip / xfail.

I agree with the idea in general, but I wanted to ask what is your plan with the android decorators: For them we use the additional api_levels flag, which does not exist on other platforms/decorators. I suppose we could add that flag to expectedFailureAll, but I am not sure if that would be a good idea...

Also, since we are doing all this refactoring, one more improvement I can think of is renaming expectedFailureAll to expectedFailure. It was named All because we already have an expectedFailure function, but I think that one is now more of an implementation detail and could be renamed to something else. Up to you...

My suggestion for the android API level is to add an argument to expectedFailure where you can specify an arbitrary function and then we can write a function called android_device_matches(...) what will return a function checking for the API level. Then this can be used to create very specific xfail conditions what are checking some property of the target system (e.g. "@expectedFailure(fn=hardwareWatchpointsNotSupported)")

Yes, that's one option I thought of. And the function could be specified with any combination of other parameters at the same time, and the result of the function would just be one value checked in determining whether to skip / xfail.

I agree but I also might consider going further where the only thing you can specify is a function and we remove all arguments. Then we implement functions like architectureMatches, targetOsMatches, hostOsMatches, etc.. and some logical function what can combine them (e.g. not, any_of, all_of). This way we just build up the condition in the decorator and we don't have a lot of check inside expectedFailure. What do you think?

I agree but I also might consider going further where the only thing you can specify is a function and we remove all arguments. Then we implement functions like architectureMatches, targetOsMatches, hostOsMatches, etc.. and some logical function what can combine them (e.g. not, any_of, all_of). This way we just build up the condition in the decorator and we don't have a lot of check inside expectedFailure. What do you think?

That does sound pretty appealing.

So then there would be some kind of combinator functions that do the equivalent of "or" and "and" logical operations? So you can get something like ((tes1() && test2()) or test3())?

-Todd

I agree but I also might consider going further where the only thing you can specify is a function and we remove all arguments. Then we implement functions like architectureMatches, targetOsMatches, hostOsMatches, etc.. and some logical function what can combine them (e.g. not, any_of, all_of). This way we just build up the condition in the decorator and we don't have a lot of check inside expectedFailure. What do you think?

That does sound pretty appealing.

So then there would be some kind of combinator functions that do the equivalent of "or" and "and" logical operations? So you can get something like ((tes1() && test2()) or test3())?

-Todd

I think we need an extra level of function nesting so the conditions only evaluated during the execution of the test. Because of it all test function will have to return a function (taking no argument) and we can't apply binary operators to them directly so I expect something like this: or(and(test1(), test2()), test3()) If you want to use binary operators then we have to use operator overloading what is a bit more work and I am not sure if it worth it but it is still possible.

I'm not opposed to it in principle, but I think we should optimize the design for conciseness at the point where you decorate a class or function. If it makes the point of decoration more verbose or harder to read, then I would probably be against it. I guess I'd need to see a proposed syntax for what you want it to look like when you decorate a function.

This revision was automatically updated to reflect the committed changes.
zturner added a comment.EditedFeb 8 2016, 5:24 PM

I'm not opposed to it in principle, but I think we should optimize the design for conciseness at the point where you decorate a class or function. If it makes the point of decoration more verbose or harder to read, then I would probably be against it. I guess I'd need to see a proposed syntax for what you want it to look like when you decorate a function.

I did think of one possible way to make this elegant (at least in my opinion). I like the keyword argument syntax because it makes it clear at a glance what you are testing, and it's also very concise while still allowing complex conditionals to be built up.

But one area that is lacking is the ability for it to specify OR conditions. Basically if you say @skipIf(A=a, B=b, C=c) then it only skips if EVERY condition is true. You hinted at this above, but it would be nice if OR were somehow supported. So one possibility is something like this:

# Skips if all conditions are true.
@skipIf(all(A=a, B=b, C=c))

# Skips if any condition is true.
@skipIf(any(A=a, B=b, C=c))

# Skips if none of the conditions are true.
@skipIf(none(A=a, B=b, C=c))

Of course, the values here could be no_match objects as well, just as now, so you could write:

# skips if ~a || b || c
@skipIf(any(A=no_match(a), B=b, C=c))

but even better, they could be written to take *args and **kwargs as well, so that non-keyword arguments are treated as compound objects. This is easier to illustrate with an example:

# skips if ~a || b || c || ~(aa && bb)
@skipIf(any(A=no_match(a), B=b, C=c, none(A=aa, B=bb)))

This should support arbitrarily complex conditionals, but is still pretty concise IMO.

Thoughts?

labath added a subscriber: labath.Feb 9 2016, 1:14 AM

I think this is getting way too complicated. I haven't seen any test
which needs such complicated combinations of skip conditions (and I
hope I never see one).

My main concern is that we have a lot of named argument for skipIf/expectFailure and I expect it to increase even further as we support more and more configuration. In my opinion we can have some functional style solution as readable as the current solution with greater flexibility.

What do you think about having the skipIf decorator taking a list of functions as argument with *argc and it's meaning is that the test is skipped if all function returns true (skipIfAll)? To make it readable we can then write simple helper functions such as: archMatches, compilerMatches, hostMatches, etc....

In this case we can have decorators like this: @skipIf(archMatches("arm"), compilerMatches("clang", "3.7"))

If we need some more complicated condition then we can either introduce logical operators what can be used to negate functions, or/xor functions or do even more complicated things based on the existing basic functions or we can introduce new free standing functions what can be passed in to the decorator (they can be specified either locally or globally)

A few complicated function I think we will need: androidDeviceMatches, linuxDistributionMatches, deviceSupportsHardwareBreakpoints, etc...

I think this solution keeps the common decorators very readable while making the system easily extensible for extreme situations.

I think this is getting way too complicated. I haven't seen any test
which needs such complicated combinations of skip conditions (and I
hope I never see one).

I agree that you don't need arbitrary complexity, but I think there are some things we would greatly benefit from. For example, have you ever seen this?

@expectedFailureWindows
@expectedFailureGcc
@expectedFailureHostLinux

That's 3 decorators, when you could write it as 1:

@expectedFailure(or(oslist="windows", compiler="gcc", hostoslist="linux"))

The rest of the functionality I mentioned you practically get for free if you support this.

My main concern is that we have a lot of named argument for skipIf/expectFailure and I expect it to increase even further as we support more and more configuration. In my opinion we can have some functional style solution as readable as the current solution with greater flexibility.

What do you think about having the skipIf decorator taking a list of functions as argument with *argc and it's meaning is that the test is skipped if all function returns true (skipIfAll)? To make it readable we can then write simple helper functions such as: archMatches, compilerMatches, hostMatches, etc....

In this case we can have decorators like this: @skipIf(archMatches("arm"), compilerMatches("clang", "3.7"))

If we need some more complicated condition then we can either introduce logical operators what can be used to negate functions, or/xor functions or do even more complicated things based on the existing basic functions or we can introduce new free standing functions what can be passed in to the decorator (they can be specified either locally or globally)

A few complicated function I think we will need: androidDeviceMatches, linuxDistributionMatches, deviceSupportsHardwareBreakpoints, etc...

I think this solution keeps the common decorators very readable while making the system easily extensible for extreme situations.

Something like that might be ok. I do agree that we can't support every use case through keyword arguments, that would be a little crazy. So we have to decide whether we want to use functional style for everything, or keep keyword style for some very common use cases and functional style for everything else. Maybe functional style for everything is better, I'll have to think about it some though.

True about the bugnumbers. Not always diferent bug numbers, but certainly
sometimes.

The reason I don't like writing them as separate decorators is because we
have tons of cases where the condition of multiple decorators pass, and the
best we can do is report one bugnumber / reason in the output because we
don't even evaluate the rest after that. If the entire conditional is
specified on one decorator, you can support that.

With functional style you could still support multiple bugnumbers on an or
conditional like this:

@xfail(or(compiler("gcc", [">=", 4.7], bugnumber=12345), hostos("windows",
bugnumber=23456)))

which is equivalent to:
@xfail(compiler="gcc", compiler_version=[">=", 4.7], bugnumber=12345)
@xfail(hostoslist="windows", bugnumber=23456)

I think also having many layers of decorator nesting provides an
implementation challenge. Ultimately we would like to re-use the skipif
functionality of unittest / unittest2 (especially since we want to port to
unittest). So it would be nice if we could write our decorator like this:

def skipIf(...):

def fn(...):
    ...
unittest2.skipIf(fn)

Right now we can't do this because the nature of the nested decorators
requires us to hack together our own implementation of skipif so that we
can manually call into the next decorator under the right conditions. If
we have just one decorator, it's much easier to write it in terms of
unittest.skipIf. Just construct the function that evaluates the condition,
and pass it in to unittest.

(For what it's worth, I'm not planning to do any of this extra stuff right
now, or even soon, so this is all theoretical at this point. Right now I
just want to remove low hanging fruit and reduce the number of decorators,
but not get it down all the way to 2).