[Lldb-commits] [PATCH] D138181: [test] Allow skipTestIfFn to apply to entire classes for skipIfNoSBHeaders

[Jordan Rupprecht via Phabricator via lldb-commits]

rupprecht added a comment.

Thanks!



================
Comment at: lldb/packages/Python/lldbsuite/test/decorators.py:146
+            # reason is used for both args.
+            return unittest2.skipIf(reason, reason)(func)
 
----------------
DavidSpickett wrote:
> This would make more immediate sense if you added the function prototype as well:
> ```
> # skipIf(condition, reason)
> ```
> 
> Or if you can figure a way to do the C++ style `skipIf(/*condition=*/reason, reason)`.
> 
We can use named args. Both the standardized `skipIf` [1] and LLDB's fork [2] use the same param names so we should be fine if we ever want to drop our fork and use standard python (I wonder if we should do that?)

I still left the comment because I feel like "condition=reason" is still atypical, but using named args makes it less load-bearing of a comment.

[1] https://docs.python.org/3/library/unittest.html?highlight=skipif#unittest.skipIf
[2] https://github.com/llvm/llvm-project/blob/0dff945bbca9e1c00ac76eafd1af61235272b7dd/lldb/third_party/Python/module/unittest2/unittest2/case.py#L86


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138181