Skip to content

Conversation

@jjotero
Copy link
Contributor

@jjotero jjotero commented Jul 7, 2021

One now can do

class MyTest(rfm.RegressionTest):
    @final
    def foo(self):
        ...

instead of having to use the @rfm.final syntax.

Other changes:

  • Renamed _rfm_special_test to _rfm_override_final. This flag is only used in the metaclass to allow the override of final methods, so this name is more descriptive. Also, the "special" naming of it should just be down to the RegressionTest class.
  • The unit test of this functionality is now in unit tests/tests_meta.py instead of the file with the loader tests.
  • The run stage of the RunOnlyRegressionTest was calling super().run.__wrapped__(self). I've replaced this line for super.run() since now the final decorator does not create a functors.wrap wrapper.

Closes #2001

@jjotero jjotero added this to the ReFrame Sprint 21.06.2 milestone Jul 7, 2021
@jjotero jjotero requested review from ekouts and vkarak July 7, 2021 12:21
@jjotero jjotero self-assigned this Jul 7, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 7, 2021

Codecov Report

Merging #2058 (044c6be) into master (34ee3d0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2058      +/-   ##
==========================================
- Coverage   86.75%   86.75%   -0.01%     
==========================================
  Files          52       52              
  Lines        9235     9240       +5     
==========================================
+ Hits         8012     8016       +4     
- Misses       1223     1224       +1     
Impacted Files Coverage Δ
reframe/core/meta.py 98.98% <100.00%> (+0.02%) ⬆️
reframe/core/pipeline.py 91.51% <100.00%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34ee3d0...044c6be. Read the comment docs.

@jjotero
Copy link
Contributor Author

jjotero commented Jul 7, 2021

@jenkins-cscs retry daint

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm. I have just a minor comment.

@victorusu
Copy link
Contributor

@jjotero and @vkarak, I cannot understand why we have a way to override the @final.
If it is final, then it should be final, no?
I would vote for not having @final at all, if one can bypass it in a way that is supported by the framework...
What do you think?
Am I being too radical?

@jjotero
Copy link
Contributor Author

jjotero commented Jul 12, 2021

@victorusu This is needed by the RunOnlyRegressionTest and CompileOnlyRegressionTest classes. The pipeline methods in the base RegressionTest are marked as final to ensure that users do not accidentally override the methods and break the pipeline, but a "master override" method is needed to specialise these two derived classes.

Copy link
Contributor

@victorusu victorusu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing we don't check in the unit tests is that a deprecation warning is thrown in case of the old syntax. I think that's why "coverage" is not very happy.

@vkarak
Copy link
Contributor

vkarak commented Jul 13, 2021

@jenkins-cscs retry daint

@vkarak vkarak merged commit 6c5f3fc into reframe-hpc:master Jul 13, 2021
@jjotero jjotero deleted the feat/final-decorator branch July 15, 2021 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose the final decorator through the metaclass

5 participants