Re: [Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
zturner added a comment. In http://reviews.llvm.org/D14406#283034, @labath wrote: > (The upstream unittest does not seem to have the bugnumber feature. I am > assuming the intention here is to make this upstream compatible, in hope of > moving over there at some point.) I can leave the bugnumber thing in there for now, just to reduce the impact of this change and the risk of messing something up. http://reviews.llvm.org/D14406 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
zturner added inline comments. Comment at: packages/Python/lldbsuite/test/lldbtest.py:605 @@ -611,2 +604,3 @@ if expected_fn(self): -raise case._UnexpectedSuccess(sys.exc_info(), bugnumber) +xfail_func = unittest2.expectedFailure(func) +xfail_func(*args, **kwargs) tberghammer wrote: > You are swallowing the bug number here > > Based on the implementation of unittest2.expectedFailure I think you should > write the following to preserve it (I haven't tested it): > > ``` > unittest2.expectedFailure(bugnumber)(func) > ``` Actually I think it already has the same sematnics. When I use your version it doesn't work. I think it works with my version because of the `if six.callable(bugnumber)` check on line 610 (which is cut out in the context here, but you can check it). I'll put it in like this and see what happens http://reviews.llvm.org/D14406 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
tberghammer added a comment. If the purpose of the change to get closer to upstream then I am fine with removing the bug number here. In general I don't feel it is a that high risk change, but I might miss something. http://reviews.llvm.org/D14406 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
zturner created this revision. zturner added reviewers: tfiala, tberghammer, labath. zturner added a subscriber: lldb-commits. The specific exception types that are thrown internally by unittest2 are considered implementation details and even documented as such in the source code of the library. This patch attempts to make this correct by removing reliance on these implementation details, and deferring to the library implementation. I'm not sure I have a good grasp of how all these decorators are supposed to work, but I think I did this right. Please verify carefully that I didn't mess anything up. The goal of this patch is to make a conditionalExpectedFailure decorator that has the following behavior: 1. If the "condition" is true (i.e. this is an expected failure), reuse the library's `expectedFailure` decorator. 2. If the condition is false (i.e. this is not an expected failure), just call the method. http://reviews.llvm.org/D14406 Files: packages/Python/lldbsuite/test/lldbtest.py Index: packages/Python/lldbsuite/test/lldbtest.py === --- packages/Python/lldbsuite/test/lldbtest.py +++ packages/Python/lldbsuite/test/lldbtest.py @@ -601,15 +601,11 @@ def wrapper(*args, **kwargs): from unittest2 import case self = args[0] -try: -func(*args, **kwargs) -except Exception: -if expected_fn(self): -raise case._ExpectedFailure(sys.exc_info(), bugnumber) -else: -raise if expected_fn(self): -raise case._UnexpectedSuccess(sys.exc_info(), bugnumber) +xfail_func = unittest2.expectedFailure(func) +xfail_func(*args, **kwargs) +else: +func(*args, **kwargs) return wrapper # if bugnumber is not-callable(incluing None), that means decorator function is called with optional arguments # return decorator in this case, so it will be used to decorating original method Index: packages/Python/lldbsuite/test/lldbtest.py === --- packages/Python/lldbsuite/test/lldbtest.py +++ packages/Python/lldbsuite/test/lldbtest.py @@ -601,15 +601,11 @@ def wrapper(*args, **kwargs): from unittest2 import case self = args[0] -try: -func(*args, **kwargs) -except Exception: -if expected_fn(self): -raise case._ExpectedFailure(sys.exc_info(), bugnumber) -else: -raise if expected_fn(self): -raise case._UnexpectedSuccess(sys.exc_info(), bugnumber) +xfail_func = unittest2.expectedFailure(func) +xfail_func(*args, **kwargs) +else: +func(*args, **kwargs) return wrapper # if bugnumber is not-callable(incluing None), that means decorator function is called with optional arguments # return decorator in this case, so it will be used to decorating original method ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. From what I can tell, it should just work. I assume you've done the obvious checks, like deliberately failing a test and seeing it registers as failed, etc. I think we should just commit it, and carefully observe the buildbots. http://reviews.llvm.org/D14406 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
labath added a comment. (The upstream unittest does not seem to have the bugnumber feature. I am assuming the intention here is to make this upstream compatible, in hope of moving over there at some point.) http://reviews.llvm.org/D14406 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D14406: Don't depend on implementation details of unittest2 for our custom decorators
tberghammer added inline comments. Comment at: packages/Python/lldbsuite/test/lldbtest.py:605 @@ -611,2 +604,3 @@ if expected_fn(self): -raise case._UnexpectedSuccess(sys.exc_info(), bugnumber) +xfail_func = unittest2.expectedFailure(func) +xfail_func(*args, **kwargs) You are swallowing the bug number here Based on the implementation of unittest2.expectedFailure I think you should write the following to preserve it (I haven't tested it): ``` unittest2.expectedFailure(bugnumber)(func) ``` http://reviews.llvm.org/D14406 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
