-
Notifications
You must be signed in to change notification settings - Fork 117
[bugfix] Fix hook override with multiple inheritance #2081
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
Codecov Report
@@ Coverage Diff @@
## master #2081 +/- ##
=======================================
Coverage 86.74% 86.75%
=======================================
Files 52 52
Lines 9267 9272 +5
=======================================
+ Hits 8039 8044 +5
Misses 1228 1228
Continue to review full report at Codecov.
|
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.
Lgtm except a couple of minor comments + the following one. Since now the hooks are checked upon class instantiation, if somebody tries to attach a hook to an inexistent state, they get:
./bin/reframe: skipping test 'HelloTest' due to errors: use `-v' for more information
FILE: n/a:n/a
The first part of the message, I think should be addressed by #1899, but the second one might be related to this PR. If not, we address that in #1899 as well.
|
I check the error message in master: |
|
You can ignore the comments about the error message here. They will be fixed by #2087. |
|
Hello @jjotero, Thank you for updating! Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide Comment last updated at 2021-07-23 13:42:46 UTC |
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.
Just a final minor comment.
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.
lgtm
This PR rearranges the instantiation/construction process of classes using the
RegressionTestMetametaclass. TheRegressionTestMetais now agnostic of pipeline-specific concepts, which facilitates its use in other classes which could also benefit from using all the machinery implemented in this metaclass (such as theBuildSystem- see #2030):__call__method in the metaclass, instead of the__new__method from the class. This means that now any class that uses thisRegressionTestMetacan use the variables and parameters "for free" without implementing anything else.__new__method of theRegressionTestclass. This moves all pipeline-specific concepts into the pipeline itself. This rearrangement paves the way for Dynamic pipeline stages #1936, and for the same money, it closes Incomplete hook override with multiple inheritance #2055.