Skip to content

Conversation

@jjotero
Copy link
Contributor

@jjotero jjotero commented Aug 11, 2021

The test registry is now a class that the loader can interact with. This TestRegistry class has a public member function instantiate_all which is equivalent to _rfm_gettests() in the previous implementation. This should give the loader far more control over the test instantiation and simplify the implementation of #2117.

However, the parameterized_test decorator deals with the construction arguments in a different way and it's incompatible with this new registry. In any case, since this decorator is deprecated, we can keep the current test registry as a "legacy_registry" which is only used by tests decorated as parameterized_test.

Similarly, the deprecated require_version decorator also complicates things slightly, forcing the TestRegistry class to account for the tests to be skipped. All this extra logic can be trimmed out once the require_version decorator is dropped.

@pep8speaks
Copy link

pep8speaks commented Aug 11, 2021

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-08-12 17:58:24 UTC

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

Codecov Report

Merging #2119 (4e155ce) into master (4c4de13) will decrease coverage by 0.30%.
The diff coverage is 80.76%.

❗ Current head 4e155ce differs from pull request most recent head a867698. Consider uploading reports for the commit a867698 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2119      +/-   ##
==========================================
- Coverage   86.26%   85.96%   -0.31%     
==========================================
  Files          53       53              
  Lines        9313     9355      +42     
==========================================
+ Hits         8034     8042       +8     
- Misses       1279     1313      +34     
Impacted Files Coverage Δ
reframe/core/decorators.py 50.33% <78.26%> (-14.16%) ⬇️
reframe/frontend/loader.py 93.70% <100.00%> (+1.57%) ⬆️

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 4c4de13...a867698. Read the comment docs.

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. Just a couple of minor comments.

@vkarak vkarak merged commit 4b0cb19 into reframe-hpc:master Aug 13, 2021
@jjotero jjotero deleted the feat/test-registry branch August 13, 2021 10:27
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.

4 participants