variable assingment order inconsistency when using file| in set_env

[piotr-kubiak]

When not using file| in set_env, the variables are assigned in order (with the last value replacing any previous one, as in FOO being overwriten with QUX in my example1).

However, this behaviour is not consistent when using file|, when assingment of variables from file always take precedence (see example2, where FOO gets the value BAR from .env, and is not overwriten by subsequent assignment of QUX).

Expected:

• consistent beavhiour, so that the last assingment of variable takes precedence, regardles if the previous one was set directly or using file|
• or at least document the priority of variables when using file|.

tox.ini:

[tox]
env_list = example1,example2

[testenv]
allowlist_externals = bash
commands =
    bash -c 'env | egrep "FOO"'

[testenv:example1]
set_env = 
    FOO=BAZ
    FOO=QUX

[testenv:example2]
set_env = 
    FOO=BAZ
    file|.env
    FOO=QUX

.env:

FOO=BAR

Output:

$ tox -v
4.18.0

$ tox -e example1
FOO=QUX

$ tox -e example2
FOO=BAR

[pachewise]

The regular assignments are processed in SetEnv.__init__, while the file assignments happen in use_replacer, which is called during by build(). Would we be OK with loading these at the same time, either in the __init__ or somewhere further down the line? To me, that seems like the easiest way to make sure that order of the assignments is taken into account, and I'm leaning toward putting them all in the __init__, but doing so feels like we'd be making the constructor "bulky".

I also just started looking at this code, so I am not sure if there was a reason that the env files are loaded after. Original PR

[kosiecg]

The problem is particularly troublesome because the current behavior is not documented anywhere, as mentioned in the issue description.

@pachewise - do you have any insights on when this issue might be resolved?

[pachewise]

@hrehork unfortunately, no (and fyi I am not a tox maintainer, just someone who stumbled into the ticket). I could open a PR with my proposed fix but I wanted to get more direction/context from the tox dev team first.

I also agree that the minimum fix here should be additional documentation clarifying the behavior (either the difference in behavior as it stands now, or a contract that the behavior should be the same and any other caveats once we fix the issue).

[gaborbernat]

I don't have any good feedback to give here. Any pull request with that adds documentation and enriches the current functionality and has a test would be welcome and appreciated.