Skip to content

Conversation

@vkarak
Copy link
Contributor

@vkarak vkarak commented Feb 12, 2023

The documentation for this case is a bit grey area. In documentation of the modes configuration parameter it is stated:

The options of an execution mode will be passed to ReFrame as if they were specified in the command line.

And the docs for the --mode read as:

If an option is specified both in an execution mode and in the command-line, then command-line takes precedence.

These are consistent for the options that can be specified only once: every time such an option is re-passed, it overwrites the previous one. But it's unclear what happens with options that are of an append type. The first excerpt implies that the options will be simply unpacked and combined with the same options in the command line (desired behaviour that this PR provides), but the second excerpt allows the interpretation that if such an option is specified in the command line will still overwrite completely that in the execution mode (current behaviour and what was validated in the unit tests).

I believe that options that can be specified multiple options must be combined and not overwritten, because this would allow users to set test variables in an execution mode and then set additional variables or reset existing ones from the command line without the need to repass all options.

Since the current behaviour was validated in the existing unit tests, I am not classifying this as a bug fix, but as an enhancement, so it targets the develop branch.

Fixes #2785.

@vkarak vkarak added this to the ReFrame 4.1 milestone Feb 12, 2023
@vkarak vkarak requested review from ekouts and teojgo February 12, 2023 20:37
@vkarak vkarak self-assigned this Feb 12, 2023
Co-authored-by: Theofilos Manitaras <manitaras@cscs.ch>
@vkarak vkarak requested a review from teojgo February 14, 2023 16:11
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Base: 86.58% // Head: 86.60% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (990a6cc) compared to base (b6e14c6).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2788      +/-   ##
===========================================
+ Coverage    86.58%   86.60%   +0.01%     
===========================================
  Files           60       60              
  Lines        11293    11299       +6     
===========================================
+ Hits          9778     9785       +7     
+ Misses        1515     1514       -1     
Impacted Files Coverage Δ
reframe/frontend/argparse.py 92.08% <100.00%> (+0.35%) ⬆️
reframe/frontend/executors/policies.py 92.81% <0.00%> (+0.27%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@vkarak vkarak merged commit 723e8b2 into reframe-hpc:develop Feb 14, 2023
@vkarak vkarak deleted the enhancement/mode-combine-append-opts branch February 14, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

The -S option resets any test variable set in an execution mode using the -S option

3 participants