Repository: reframe-hpc/reframe

Issue:
Some options in a mode do not expand environment variables past in
Given the following mode:
```
        {
            'name': 'jenkins-prebuilt',
            'options': [
                '--run',
                '--save-log-files',
                '--performance-report',
                '--purge-env',
                '--prefix ${AT_OUTPUT_DIR}',
                '--stage ${AT_OUTPUT_DIR}/stage/${JOB_BASE_NAME}/${BUILD_NUMBER}',
                '--output ${AT_OUTPUT_DIR}/output/${JOB_BASE_NAME}/${BUILD_NUMBER}',
                '--perflogdir ${AT_OUTPUT_DIR}/perflogs',
                '--report-file ${AT_OUTPUT_DIR}/report/report_{sessionid}.json',
                '--job-option v=JENKINS_BUILD=${JOB_BASE_NAME}-${BUILD_NUMBER}',
                '--setvar project=${AT_ACCOUNT}',
                '--setvar use_prebuilt=True',
                '--setvar queue=${QUEUE}',
                '--setvar num_nodes=${NODES}',
            ]
        },
```

I would expect the environment variables for each option to expand to what they're set to. For some (`--stage`, `--prefix`, `--output`, `--perflogdir`, and `--report-file`) they do expand, but some don't expand (`--job-option`, `--setvar`).

If environment variables are set prior to executing `reframe`:

```
export AT_OUTPUT_DIR=/flare/Aurora_AT/leggett/reframe/data
export NODES=1
...
```

Some are obviously set because paths are correct:
```
$ reframe -v -C /flare/Aurora_AT/leggett/alcf_reframe_testsuite/config/alcf.py --mode jenkins-prebuilt -c /flare/Aurora_AT/leggett/alcf_reframe_testsuite/tests/sow/FFT/fft_sow.py
[ReFrame Setup]
  version:           4.5.1
  command:           '/opt/aurora/24.180.1/spack/unified/0.8.0/install/linux-sles15-x86_64/gcc-12.2.0/reframe-4.5.1-exvucbt/bin/reframe -v -C /flare/Aurora_AT/leggett/alcf_reframe_testsuite/config/alcf.py --mode jenkins-prebuilt -c /flare/Aurora_AT/leggett/alcf_reframe_testsuite/tests/sow/FFT/fft_sow.py'
  launched by:       auroraat@aurora-uan-0014
  working directory: '/lus/flare/projects/Aurora_AT/leggett/reframe/data/tmp/sow/FFT/1/13'
  settings files:    '<builtin>', '/flare/Aurora_AT/leggett/alcf_reframe_testsuite/config/alcf.py'
  check search path: (R) '/lus/flare/projects/Aurora_AT/leggett/alcf_reframe_testsuite/tests/sow/FFT/fft_sow.py'
  stage directory:   '/flare/Aurora_AT/leggett/reframe/data/stage/FFT_TEST/13'
  output directory:  '/flare/Aurora_AT/leggett/reframe/data/output/FFT_TEST/13'
  log files:         '/lus/flare/projects/Aurora_AT/leggett/reframe/data/tmp/sow/FFT/1/13/reframe.log', '/lus/flare/projects/Aurora_AT/leggett/reframe/data/tmp/sow/FFT/1/13/reframe.out'
```

But the test is erroring out because `${NODES}` doesn't seem to resolve but is instead being passed as literal text:
```
WARNING: skipping test 'FFTTest': value error: invalid literal for int() with base 10: '${NODES}' (rerun with '-v' for more information)
```

Discussion:
@vkarak: That's indeed inconsistent and we should treat more uniformly the environment variable expansion.@vkarak: This happens because we don't expand the environment variables in the mode's option. Any expansion happens at a later point and it is usually done only for options that are associated with a configuration parameter.

We should instead expand all environment variables in the mode options before parsing them.

Solution patch:
diff --git a/reframe/frontend/cli.py b/reframe/frontend/cli.py
index 2337546bd..7551134e2 100644
--- a/reframe/frontend/cli.py
+++ b/reframe/frontend/cli.py
@@ -941,9 +941,12 @@ def restrict_logging():
                 # We lexically split the mode options, because otherwise spaces
                 # will be treated as part of the option argument;
                 # see GH bug #1554
-                mode_args = list(
-                    itertools.chain.from_iterable(shlex.split(m)
-                                                  for m in mode_args))
+                mode_args = list(itertools.chain.from_iterable(
+                    shlex.split(osext.expandvars(arg)) for arg in mode_args)
+                )
+                printer.debug(f'Expanding execution mode {options.mode!r}: '
+                              f'{" ".join(mode_args)}')
+
                 # Parse the mode's options and reparse the command-line
                 options = argparser.parse_args(mode_args,
                                                suppress_required=True)


Test patch:
diff --git a/unittests/resources/config/settings.py b/unittests/resources/config/settings.py
index fb207d64c..70571e229 100644
--- a/unittests/resources/config/settings.py
+++ b/unittests/resources/config/settings.py
@@ -275,6 +275,11 @@ def hostname():
                 '-p builtin',
                 '-S local=1'
             ]
+        },
+        {
+            'name': 'env_vars',
+            'options': ['-n', '${TEST_NAME_PATTERN}',
+                        '-S', '${VAR}=${VAL}']
         }
     ],
     'logging': [
diff --git a/unittests/test_cli.py b/unittests/test_cli.py
index 799b45f9e..4e3b1c44b 100644
--- a/unittests/test_cli.py
+++ b/unittests/test_cli.py
@@ -542,6 +542,20 @@ def test_execution_modes(run_reframe, run_action):
     assert 'Ran 1/1 test case' in stdout
 
 
+def test_execution_modes_envvar_expansion(run_reframe, monkeypatch):
+    monkeypatch.setenv('TEST_NAME_PATTERN', '^HelloTest$')
+    monkeypatch.setenv('VAR', 'x')
+    monkeypatch.setenv('VAL', '1')
+    returncode, stdout, stderr = run_reframe(
+        mode='env_vars', action='list'
+    )
+    assert returncode == 0
+    assert 'Traceback' not in stdout
+    assert 'Traceback' not in stderr
+    assert "the following variables were not set: 'x'" in stdout
+    assert 'Found 1 check(s)' in stdout
+
+
 def test_invalid_mode_error(run_reframe):
     mode = 'invalid'
     returncode, stdout, stderr = run_reframe(
diff --git a/unittests/test_config.py b/unittests/test_config.py
index 160adabbb..e63093c1f 100644
--- a/unittests/test_config.py
+++ b/unittests/test_config.py
@@ -225,7 +225,7 @@ def test_select_subconfig(site_config):
     site_config.select_subconfig('testsys')
     assert len(site_config['systems']) == 1
     assert len(site_config['systems'][0]['partitions']) == 2
-    assert len(site_config['modes']) == 1
+    assert len(site_config['modes']) == 2
     assert site_config.get('systems/0/name') == 'testsys'
     assert site_config.get('systems/0/descr') == 'Fake system for unit tests'
     assert site_config.get('systems/0/hostnames') == ['testsys']