-
Notifications
You must be signed in to change notification settings - Fork 117
Update gromacs values #290
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
Conversation
vkarak
left a comment
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.
It's a good opportunity to switch this test to the new syntax as well.
|
@jenkins-cscs retry all |
| super().__init__(name, os.path.dirname(__file__), **kwargs) | ||
| class GromacsBaseCheck(rfm.RunOnlyRegressionTest): | ||
| def __init__(self, name, output_file): | ||
| super().__init__(name, os.path.dirname(__file__)) |
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.
Passing arguments to the base constructor is not so new-stylish, and I don't know if it's gonna be supported in the future. The arguments are there just for backward compatibility. The correct way to do it is to set the name and the description afterwards, like that:
class GromacsBaseCheck(rfm.RunOnlyRegressionTest):
def __init__(self, name, output_file):
super().__init__()
self.name = name
self.descr = 'my gromacs test'If you set the test's name this way, it is important to set the description also, cos it will hold the automatically generated name otherwise.
| } | ||
|
|
||
|
|
||
| @rfm.parameterized_test([(1,), (2,), (4,), (6,), (8,)]) |
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.
Instead of using tuples, it's perhaps more compact to use one-element lists:
@rfm.parameterized_test([[1], [2], [4], [6], [8]])Perhaps, I could also update the documentation to show the example with lists instead of tuples.
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.
In #294, I have changed the decorator to accept multiple arguments, so that you can write this nicer as follows:
@rfm.parameterized_test([1], [2], [4], [6], [8])You can even use a range as follows:
@rfm.parameterized_test([1], *([x] for x in range(2,9,2)))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.
#294 is merged, so you can now use the syntax without the extra brackets.
| def __init__(self, num_nodes, **kwargs): | ||
| nodes_label = 'node' if num_nodes == 1 else 'nodes' | ||
| def __init__(self, variant): | ||
| nodes_label = 'node' if variant == 1 else 'nodes' |
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.
You could simply write "node(s)" and avoid this extra complication here.
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.
Also, I find the original argument name (num_nodes) more accurate than the variant.
Fixes https://github.com/eth-cscs/reframe-tests/issues/24