DM-31141: Add test decorators for cartesian product #96
Conversation
PaulPrice
force-pushed
the
tickets/DM-31141
branch
from
b1c569d
to
12288bd
Compare
python/lsst/utils/tests.py
Outdated
| @@ -842,6 +843,112 @@ def wrapper(self, *args, **kwargs): | |||
| return decorator | |||
|
|
|||
|
|
|||
| def cartesianProduct(settings): | |||
There was a problem hiding this comment.
I assume this is a private function that should start with an underscore?
There was a problem hiding this comment.
I wouldn't have called it private: it doesn't have any hidden implementation details. It's just a helper function. It doesn't get exported in the __all__ only because I didn't think it would be useful to anyone, but does that make it "private"?
There was a problem hiding this comment.
Okay. I tend to put an _ in front of functions as an additional "don't rely on this existing in the future" hint since people can pull functions out of packages even if they aren't in __all__. Up to you.
| """ | ||
| product = {kk: [] for kk in settings} | ||
| for values in itertools.product(*settings.values()): | ||
| for kk, vv in zip(settings.keys(), values): |
There was a problem hiding this comment.
To clarify my comment, I don't think .keys() is needed because zip will automatically get the keys from the dict.
>>> a = {1:2, 3:4}
>>> b = {"a":"b", "c":"d"}
>>> print(list(zip(a, b)))
[(1, 'a'), (3, 'c')]
There was a problem hiding this comment.
Absolutely. I updated the code already.
There was a problem hiding this comment.
👍 I realized my comment was ambiguous such that the thumbs up from you was ambiguous to me 😄
tests/test_decorators.py
Outdated
| def testMethodDecorator(self, xx, yy): | ||
| self.combinations.add((xx, yy)) | ||
|
|
||
| def teardown_method(self, method): |
There was a problem hiding this comment.
Ah, it seems it is -- it doesn't fire when run with python tests/test_decorators.py. I copied the earlier implementation, which also uses it.
Do I need to find an alternative implementation?
There was a problem hiding this comment.
It's somewhat ironic that the test of this new decorator that implements functionality already present in pytest, requires pytest to be tested. The test passes when run as python tests/... or it fails?
There was a problem hiding this comment.
It passes. It just doesn't hit this additional check, and so isn't exercising the full test.
I've got an alternative implementation in mind. I'll give it a go.
There was a problem hiding this comment.
OK, lemme know what you think of this.
There was a problem hiding this comment.
Using a test flag and tearDown is fine with me.
The classParameters and methodParameters decorators are useful, but sometimes you want to iterate over all combinations of some parameters, but don't want to go to the trouble of listing all combinations. This commit adds classParametersProduct and methodParmaetersProduct that generates the cartesian product of the provided parameters, and iterates over them.
The teardown_method method doesn't fire when run directly, but only under pytest, which means that a developer running the test directly isn't testing the functionality. Adopted an alternative implementation that doesn't rely on pytest. It's heavier than I would like (relies on the method being tested on teardown flipping a switch), but it gets the job done.
PaulPrice
force-pushed
the
tickets/DM-31141
branch
from
12288bd
to
92e40cf
Compare
|
OK, I think everyone is happy. Merge is imminent. |
No description provided.