Repository: LUMC/pytest-workflow (language: python)

Issue:
Use of environment variables should be clarified in the documentation
I am trying to run a test where I have an environment variable in the command but it seems that it is not expanded or something. 

The command (which runs as expected outside pytest-workflow) is:
```
nextflow run ./tests/test -entry test -profile $NF_PROFILE
```

And the error is a nextflow one: `Unknown configuration profile: '$NF_PROFILE'`
It appears to be passing `$NF_PROFILE` literally? Is this the expected behaviour? 


Discussion / hints:
> It appears to be passing $NF_PROFILE literally? Is this the expected behaviour?

Yes, this uses Python's subprocess which executes the command without a shell. So shell functionality such as variables is not necessarily present.

Can you try `bash -c "nextflow run ./tests/test -entry test -profile $NF_PROFILE"` ? This will execute the statement in bash. 
Ah that makes sense. `bash -c` works. Out of interest, is it apytest-workflowa doing the `subprocess` call or `pytest` itself? I am wondering if there is maybe an option to set `shell=True` somehow instead to keep the command cleaner (or a future PR to make it possible)
The call is made here in pytest-workflow:

https://github.com/LUMC/pytest-workflow/blob/33eb78a2e4b61a9b259a83febf62c0ef058c59b6/src/pytest_workflow/workflow.py#L73-L95

> I am wondering if there is maybe an option to set shell=True somehow instead to keep the command cleaner (or a future PR to make it possible)

I agree that would make the command cleaner. However this might behave differently depending on the system's shell. To keep behavior consistent I think it is best to not enable `shell` and let the user explicitly declare which shell is used. This way the behavior is always consistent between runs on different system. Which for a workflow tester I think is very important.

Also it disables some 'dangerous' shell behaviour by default. In the above case someone could set `$NF_PROFILE` to `MYPROFILE && echo "$PG_PASSWORD" && rm -rf --no-preserve-root /` for example. That was another reason why explicit shell enabling was chosen.

Maybe this should be highlighted a bit more in the documentation.


Is there really any difference in security between `bash -c` and using `shell=True`? 
What I was thinking of was more of an option - something like:

```yaml
- name: test
  command: mytest command --option 1
  shell: true
```
Which could then be passed via the workflow object to `subprocess`. 

If that's not of interest then more explicit documentation of the issue/solution (e.g. using `bash -c` ) would be great! 
This is an example of it being explicit in the command - but if I have it in my `nextflow.config` for example:
```
params.data_dir = $DATA_DIR
```

then it is less obvious you might need to use `bash -c`
> Is there really any difference in security between bash -c and using shell=True?

Yes. With `bash -c`  you decide that injection attacks are no problem, with `shell=True` the developer will have decided for you. As a developer, I cannot make that decision for everyone who uses this project.

> What I was thinking of was more of an option - something like:

This is a good suggestion. However `shell` is not as clear as `bash -c`. `shell=True` refers to the default shell. This might be different between systems. To prevent workflow tests to crash on different systems it is always better to explicitly declare which shell needs to be used. For example `/bin/sh` on Debian=based systems will use dash, which might lead to unexpected crashes when bash is expected. Some users might have set fish as their default shell, etc.

So in conclusion, pytest-workflow leaves the decision of running in a shell to the user, and the user must be explicit about the shell used. I think this approach has the least disadvantages.

> If that's not of interest then more explicit documentation of the issue/solution (e.g. using bash -c ) would be great!

Yes, that is indeed a good thing to do. I will leave this issue open until I have updated the documentation.



Fix patch:
diff --git a/HISTORY.rst b/HISTORY.rst
index 2bfdfa0f..0d72de4b 100644
--- a/HISTORY.rst
+++ b/HISTORY.rst
@@ -9,6 +9,7 @@ Changelog
 
 version 1.7.0-dev
 ---------------------------
++ Document the use of environment variables with pytest-workflow.
 + A minimum of pytest 7.0.0 is now a requirement for pytest-workflow.
   This fixes the deprecation warnings that started on the release of pytest
   7.0.0.
diff --git a/docs/conf.py b/docs/conf.py
index 873b5186..41038374 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -64,7 +64,7 @@
 #
 # This is also used if you do content translation via gettext catalogs.
 # Usually you set "language" from the command line for these cases.
-language = None
+language = "en"
 
 # List of patterns, relative to source directory, that match files and
 # directories to ignore when looking for source files.


Test patch:
diff --git a/docs/writing_tests.rst b/docs/writing_tests.rst
index f9b32b87..62086d68 100644
--- a/docs/writing_tests.rst
+++ b/docs/writing_tests.rst
@@ -90,6 +90,33 @@ sequences.
     Workflow names must be unique. Pytest workflow will crash when multiple
     workflows have the same name, even if they are in different files.
 
+Environment variables
+----------------------
+Pytest-workflow runs tests in the same environment as in which the pytest
+executable was started. This means programs started in tests can use
+environnment variables. However, environment variables inside the command
+section itself are quoted by pytest-workflow using
+`shlex.quote <https://docs.python.org/3/library/shlex.html#shlex.quote>`_.
+See the examples below:
+
+.. code-block:: YAML
+
+    - name: Try to use an environment variable
+      command: echo $MY_VAR
+      # Output will be literally "$MY_VAR"
+
+    - name: Circumenvent shlex quoting by explicitly starting the command in a shell.
+      command: bash -c 'echo $MY_VAR'
+      # Output will be the content of $MY_VAR
+
+    - name: Use a program that checks an environment variable
+      command: singularity run my_container.sif
+      # Correctly uses "SINGULARITY_" prefixed variables
+
+If you want to use shell scripting features such as environment
+variables inside ``command``, you need to explicitly set the shell as shown
+above.
+
 Writing custom tests
 --------------------