-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clear env before test cases requiring empty env #12561
Clear env before test cases requiring empty env #12561
Conversation
Thank you @tkonopka for your quick action! There seem to be other occurrences which I believe need to be updated in the tests. Could you also update the following lines? $ grep -rnI "os.environ, {}" tests
tests/loggers/test_tensorboard.py:126:@mock.patch.dict(os.environ, {})
tests/callbacks/test_gpu_stats_monitor.py:145:@mock.patch.dict(os.environ, {})
tests/plugins/environments/test_lsf_environment.py:104: with mock.patch.dict(os.environ, {}):
tests/plugins/environments/test_lightning_environment.py:22:@mock.patch.dict(os.environ, {})
tests/plugins/environments/test_lightning_environment.py:68:@mock.patch.dict(os.environ, {})
tests/plugins/environments/test_slurm_environment.py:23:@mock.patch.dict(os.environ, {})
tests/plugins/environments/test_slurm_environment.py:95: with mock.patch.dict(os.environ, {}):
tests/plugins/environments/test_torchelastic_environment.py:24:@mock.patch.dict(os.environ, {})
tests/plugins/environments/test_torchelastic_environment.py:77: with mock.patch.dict(os.environ, {}):
tests/plugins/environments/test_torchelastic_environment.py:95: with mock.patch.dict(os.environ, {}):
tests/plugins/environments/test_kubeflow_environment.py:23:@mock.patch.dict(os.environ, {}) |
test_slurm_environments.py
for compatibility with 'make test'
Done the additional changes. This will help the suite run in environments that pre-set variables. Thanks. Earlier, some of the automatic checks seemed stuck (4 expected, and 1 pending checks). I was waiting on those before marking the PR as 'ready', but happy to follow your lead @akihironitta. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tkonopka Thank you for applying the changes! LGTM!
* clearing env vars in a test to allow compatibility with "make test" * added clear=True to more mock environments in testcases Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>
* clearing env vars in a test to allow compatibility with "make test" * added clear=True to more mock environments in testcases Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>
What does this PR do?
Fixes #12551
An existing test case currently runs well with
python -m pytest tests/
but fails withmake test
because the Makefile changes an env variable. The fix updates the test case so that it sees the same env state under the two scenarios.Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃