Clean up environment before each test #270
Conversation
al-rigazzi
added
area: test
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #270 +/- ##
===========================================
- Coverage 87.65% 87.53% -0.12%
===========================================
Files 60 60
Lines 3386 3386
===========================================
- Hits 2968 2964 -4
- Misses 418 422 +4 |
smartsim/settings/slurmSettings.py
Outdated
| return [f"{k}={v}" for k, v in self.env_vars.items() if "," not in str(v)] | ||
|
|
||
There was a problem hiding this comment.
I make styled now, thanks.
conftest.py
Outdated
| @@ -411,6 +411,13 @@ def db_cluster(fileutils, wlmutils, request): | |||
| exp.stop(db) | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="function", autouse=True) | |||
| def environment_cleanup(): | |||
| os.environ.pop("SSDB", "") | |||
There was a problem hiding this comment.
Could we use monkeypatch here just to make sure that the state of the env is restored after the test suite runs?
def environment_cleanup(monkeypatch):
monkeypatch.delenv("SSDB", raising=False)
# etc...Shouldn't matter unless someone decides to source pytest, but never hurts to be safe, lol
There was a problem hiding this comment.
Monkeys are everywhere.
smartsim/settings/slurmSettings.py
Outdated
| # If a variable is defined, it will take precedence over --export | ||
| # we warn the user | ||
| preexisting_var = os.environ.get(k, None) | ||
| if preexisting_var: |
There was a problem hiding this comment.
Should we make this an explicit null check just in case a user explicitly exports an empty env var?
Doing so will allow us to handle this scenario better
export EMPTY=
unset DNEby still logging the warning for EMPTY. Currently this clause will not, but I think it should.
import os
assert os.environ.get("EMPTY") == ""
assert os.environ.get("DNE") is None
There was a problem hiding this comment.
I think we'll have to revisit this when we add key-only exports. The current check would mark them as pre-existing and log a misleading warning.
There was a problem hiding this comment.
Quick clarification based on a discussion between me and @ankona, could we make this current check:
if preexisting is None:
... # issue warningto handle the empty-env-var case
There was a problem hiding this comment.
@MattToast I am almost 100% sure you meant
if preexisting is not None:
... # issue warningand I agree, I was not considering the empty environment variable.
@ankona yes, I think we'll have to revisit this for that case, but adding a more specific check now would make the code a bit confusing.
There was a problem hiding this comment.
🤦
hahahaha, it only took 3 devs, but I do think we ended up at the right clause for this if statement!
tests/test_slurm_settings.py
Outdated
| assert record.message == msg | ||
|
|
||
| os.environ.pop("SMARTSIM_TEST_VAR", None) | ||
| os.environ.pop("SMARTSIM_TEST_CSVAR", None) |
There was a problem hiding this comment.
Incredibly minor refactor request, don't feel the need to make this change if you don't want:
If you'd like, monkeypatch.setenv will automatically handle the resetting of the envvars back to their original values a the end of a test
(Shouldn't matter unless someone sources pytest AND has SMARTSIM_TEST_VAR or SMARTSIM_TEST_CSVAR set in their env, so the chances of this mattering are minuscule)
There was a problem hiding this comment.
Thanks for introducing me to the joys of monkeypatch.
This PR adds an environment cleanup fixture which is run before every test. Environment variables such as
SSDB,SSKEYIN, andSSKEYOUTare removed if they are found to be set. This avoids issues with testing on Slurm.To make users aware of Slurm's behavior, a warning is issued if the user tries to export a variable that is already defined. This warning is not needed when we set a variable to a comma-separated value, as in that case, we are explicitly overwriting the environment value (by setting the value before
srun).