Page MenuHomePhabricator

Make test categories composable
ClosedPublic

Authored by labath on Dec 11 2015, 7:35 AM.

Details

Summary

Previously the add_test_categories would simply overwrite the current set of categories for a
method. This change makes the decorator truly "add" categories, by extending the current set of
categories instead of replacing it.

To do this, I have:

  • replaced the getCategories() property on a method (which was itself a method), with a simple list property "categories". This makes add_test_categories easier to implement, and test categories isn't something which should change between calls anyway.
  • rewritten the getCategoriesForTest function to merge method categories with the categories of the test case. Previously, it would just use the method categories if they were present. I have also greatly simplified this method. Originally, it would use a lot of introspection to enable it being called on various types of objects. Based on my tests, it was only ever being called on a test case. The new function uses much less introspection then the preivous one, so we should easily catch any stray uses, if there are any, as they will generate exceptions now.

Diff Detail

Repository
rL LLVM

Event Timeline

labath updated this revision to Diff 42521.Dec 11 2015, 7:35 AM
labath retitled this revision from to Make test categories composable.
labath updated this object.
labath added reviewers: zturner, tfiala, tberghammer.
labath added a subscriber: lldb-commits.
labath updated this revision to Diff 42522.Dec 11 2015, 7:41 AM

I've changed my mind and I'm moving back the getCategoriesForTest function into test_result.py.
The function only works when the test is currently executing (because of the magic
_testMethodName hack), so it's usability as a general utility function is very limited. Let's keep it here until we have a reason to do otherwise.

tberghammer accepted this revision.Dec 11 2015, 8:38 AM
tberghammer edited edge metadata.

LGTM

packages/Python/lldbsuite/test/lldbtest.py
512 ↗(On Diff #42522)

Can you be more specific about where can we add this decorator? "item" doesn't really tell anything to me

This revision is now accepted and ready to land.Dec 11 2015, 8:38 AM
tfiala accepted this revision.Dec 11 2015, 10:24 AM
tfiala edited edge metadata.

LGTM. I added a few optional comments on possible improvements, but I think they're orthogonal to what you're doing here.

packages/Python/lldbsuite/test/lldbtest.py
512 ↗(On Diff #42522)

I think we're talking TestCase test method, right?

518 ↗(On Diff #42522)

This code could potentially accept a single category and drop the need for the list. It could check the type of the input (i.e. the categories input) and, if a single element rather than a list, it could just cat.append() it.

That would enable a single argument passed to @add_test_categories()

Totally minor and you can skip this, it just makes for a nice usability enhancement provided the decorator is documented as such.

tberghammer added inline comments.Dec 14 2015, 3:12 AM
packages/Python/lldbsuite/test/lldbtest.py
518 ↗(On Diff #42522)

+1: I think we will (accidentally) call the function by a single string in some case so accepting it would be a good idea.

labath added inline comments.Dec 14 2015, 4:32 AM
packages/Python/lldbsuite/test/lldbtest.py
518 ↗(On Diff #42522)

The test_categories.validate will catch the case where you forget to put the braces and throw errors at you, so it's not possible to get unexpected behavior here.

I may be old-fashioned, but I like when things have a single type, which is obvious from the code. Look at all the cases where we do callable(bugnumber) and tell me if it's obvious why a "bug number" should be "callable". Plus, none of our other decorators taking lists have this kind of magic in them (imagine how would this complicate the logic of expectedFailureAll).

If we want to provide a simpler syntax for adding a single category, i would go for adding a separate @add_test_category decorator for that.

This revision was automatically updated to reflect the committed changes.
tberghammer added inline comments.Dec 14 2015, 5:39 AM
packages/Python/lldbsuite/test/lldbtest.py
518 ↗(On Diff #42522)

I think the pythonic way is to accept (almost) any type of argument including a single element string and any iterable (list, set, generator expression, etc...). In long term I would like to see all of our decorator taking single and multi argument at the same time but we can make that change later with refactoring all decorator to support it (it can be handled easily by a utility function).