-
Notifications
You must be signed in to change notification settings - Fork 117
[test] Update NAMD test #574
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
|
@jenkins-cscs retry all |
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.
This should not be merged. There are three issues related to NAMD, that I'd say should be addressed by a single PR. This PR must incorporate the change of #549 and invalidate that PR, and it must also address issue https://github.com/eth-cscs/reframe-tests/issues/33. I have also comments about the test itself, but I'll come back to them later.
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.
The test must be further refactored. Currently, they are a mixture of old and new syntax. This is how it should be done:
class NamdBaseCheck(rfm.RunOnlyRegressionTest):
def __init__(self, version, variant):
super().__init__()
self.name = 'namd_%s_%s_check' % (version, variant)
self.descr = 'NAMD check (%s, %s) % (version, variant)
...
@rfm.parameterized_test(['maint'], ['prod'])
class NamdGPUCheck(NamdBaseCheck):
def __init__(self, variant):
super().__init__('gpu', variant)
...
@rfm.parameterized_test(['maint'], ['prod'])
class NamdCPUCheck(NamdBaseCheck):
def __init__(self, variant):
super().__init__('cpu', variant)
...You could also do everything in a single class that you instantiate four times. This is up to you.
|
@jenkins-cscs retry daint |
Closes https://github.com/eth-cscs/reframe-tests/issues/35
Closes https://github.com/eth-cscs/reframe-tests/issues/33
Closes #540.