Skip to content

Conversation

@vkarak
Copy link
Contributor

@vkarak vkarak commented Jan 31, 2022

This PR adds a convenience function, set_var_default() to check if a variable is defined. to set a variable in a test only if it is not already defined. This is particularly useful in library tests, when you would like to set a default in a pipeline hook and you should only do that if the user has not defined another value with the -S option.

@pep8speaks
Copy link

pep8speaks commented Jan 31, 2022

Hello @vkarak, 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 2022-02-01 16:14:18 UTC

@vkarak
Copy link
Contributor Author

vkarak commented Jan 31, 2022

Do you think that it would make sense to add also a set_var_default() convenience method? Users could write:

type(self).set_var_default('foo , 1)

instead of

if not type(self).is_var_defined('foo'):
    self.foo = 1

@vkarak
Copy link
Contributor Author

vkarak commented Jan 31, 2022

Perhaps, we should better expose this functionality at the test level, so that users avoid the type(self) thing.

@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2022

Codecov Report

Merging #2403 (2d2a80b) into master (5dc93df) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##           master    #2403   +/-   ##
=======================================
  Coverage   85.63%   85.64%           
=======================================
  Files          56       56           
  Lines       10465    10471    +6     
=======================================
+ Hits         8962     8968    +6     
  Misses       1503     1503           
Impacted Files Coverage Δ
reframe/core/meta.py 99.11% <ø> (ø)
reframe/core/pipeline.py 93.14% <100.00%> (+0.05%) ⬆️

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 5dc93df...ad68b98. Read the comment docs.

@vkarak vkarak changed the title [feat] Add convenience function to check if a test variables is defined [feat] Add convenience function to set a test variable only if it's undefined Feb 1, 2022
@vkarak
Copy link
Contributor Author

vkarak commented Feb 1, 2022

I have hidden the metaclass stuff and I exposed a new function at the level of RegressionTest. Now users can simply do set_var_default('foo', 10) in their test.

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

@vkarak vkarak merged commit 63bb936 into reframe-hpc:master Feb 1, 2022
@vkarak vkarak deleted the feat/is-var-defined branch February 1, 2022 16:57
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.

5 participants