-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Improve naming scheme of tests #1848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
418fabf
98c418c
f5c7ce9
87ad77b
5854ea5
25879fe
23321f5
44bd2db
adec202
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -135,11 +135,21 @@ def assert_target_cls(self, cls): | |
| assert isinstance(getattr(cls, self.local_namespace_name), | ||
| LocalNamespace) | ||
|
|
||
| def allow_inheritance(self, cls, base): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should also go away when we drop the |
||
| '''Return true if cls is allowed to inherit the namespace of base. | ||
|
|
||
| Subclasses may override that to customize the behavior. | ||
| ''' | ||
| return True | ||
|
|
||
| def inherit(self, cls): | ||
| '''Inherit the Namespaces from the bases.''' | ||
|
|
||
| for base in filter(lambda x: hasattr(x, self.namespace_name), | ||
| cls.__bases__): | ||
| if not self.allow_inheritance(cls, base): | ||
| continue | ||
|
|
||
| assert isinstance(getattr(base, self.namespace_name), type(self)) | ||
| self.join(getattr(base, self.namespace_name), cls) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,24 +27,30 @@ class TestParam: | |||||||||||||||||||||||||||||||||||||||||||||||||||
| ''' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __init__(self, values=None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| inherit_params=False, filter_params=None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| inherit_params=False, filter_params=None, fmtval=None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if values is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| values = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # By default, filter out all the parameter values defined in the | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # base classes. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not inherit_params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # By default, filter out all the parameter values defined in the | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # base classes. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| def filter_params(x): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return () | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If inherit_params==True, inherit all the parameter values from the | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # base classes as default behaviour. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| elif filter_params is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If inherit_params==True, inherit all the parameter values from the | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # base classes as default behaviour. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| def filter_params(x): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return x | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not callable(filter_params): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise TypeError('filter_params must be a callable') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| if fmtval and not callable(fmtval): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise TypeError('fmtval must be a callable') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.values = tuple(values) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.filter_params = filter_params | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.format_value = fmtval | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __set_name__(self, owner, name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.name = name | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -90,7 +96,16 @@ def __init__(self, target_cls=None, target_namespace=None): | |||||||||||||||||||||||||||||||||||||||||||||||||||
| super().__init__(target_cls, target_namespace) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Internal parameter space usage tracker | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.__unique_iter = iter(self) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.set_iter_fn(itertools.product) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # self.__unique_iter = iter(self) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
98
to
+100
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With fixtures (and also the callbacks from below) in mind, I think it's a lot better if we enable index access to the parameter points. That would give us all the control when instantiating a class on choosing which point in the parameter space we want, rather than relying on the consumption of the parameter space. What I have in mind is something like the following: self.__rand_access_iter = list(iter(self))and then we could even override the def __getitem__(self, idx):
return self.__rand_access_iter[idx]Then we can inject the parameters from the point in the parameter space that the reframe test wants (see the |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Alternative value formatting functions | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.__format_fn = {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def allow_inheritance(self, cls, base): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Do not allow inheritance of the parameterization space in case of | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # the old @parameterized_test decorator. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return not hasattr(cls, '_rfm_compat_parameterized') | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+105
to
+108
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🪚 |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def join(self, other, cls): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| '''Join other parameter space into the current one. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -103,6 +118,7 @@ def join(self, other, cls): | |||||||||||||||||||||||||||||||||||||||||||||||||||
| :param other: instance of the ParamSpace class. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| :param cls: the target class. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ''' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| for key in other.params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # With multiple inheritance, a single parameter | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # could be doubly defined and lead to repeated | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -128,6 +144,8 @@ def extend(self, cls): | |||||||||||||||||||||||||||||||||||||||||||||||||||
| self.params[name] = ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| p.filter_params(self.params.get(name, ())) + p.values | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if p.format_value: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.__format_fn[name] = p.format_value | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # If any previously declared parameter was defined in the class body | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # by directly assigning it a value, raise an error. Parameters must be | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -158,26 +176,34 @@ def inject(self, obj, cls=None, use_params=False): | |||||||||||||||||||||||||||||||||||||||||||||||||||
| parameter values defined in the parameter space. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| ''' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now in this function, instead of having Also, since the reframe test has the index of the parameter space, the |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Set the values of the test parameters (if any) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| injected = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if use_params and self.params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Consume the parameter space iterator | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| param_values = next(self.unique_iter) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for index, key in enumerate(self.params): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| setattr(obj, key, param_values[index]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| format_fn = self.__format_fn.get(key) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| injected.append( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| (key, param_values[index], format_fn) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj.params_inserted(injected) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+181
to
+193
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| except StopIteration as no_params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise RuntimeError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| f'exhausted parameter space: all possible parameter value' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| f' combinations have been used for ' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| f'{obj.__class__.__qualname__}' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) from None | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Otherwise init the params as None | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for key in self.params: | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| setattr(obj, key, None) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| obj.params_inserted(injected) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def __iter__(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| '''Create a generator object to iterate over the parameter space | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -186,10 +212,17 @@ def __iter__(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| :return: generator object to iterate over the parameter space. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ''' | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield from itertools.product( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # yield from itertools.product( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # *(copy.deepcopy(p) for p in self.params.values()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| # ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| yield from self.__iterator( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| *(copy.deepcopy(p) for p in self.params.values()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| def set_iter_fn(self, fn): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.__iterator = fn | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.__unique_iter = iter(self) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| @property | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| def params(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return self._namespace | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just remove the parameterized test here already? I think its time has arrived 🪚