test: avoid changing os.environ#562
Merged
Merged
Conversation
|
The created documentation from the pull request is available at: docu-html |
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
MaximilianSoerenPollak
approved these changes
May 28, 2026
Contributor
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Ye that looks good to me.
Makes sense ot use the monkeypatch for the thing it actually was meant to be used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Tests that directly assign to
os.environand clean up manually are fragile: if a test raises before the cleanup code runs, the environment variable leaks into every subsequent test in the same process. Because several of these fixtures were scoped tosession, a leakedBUILD_WORKSPACE_DIRECTORYorKNOWN_GOOD_JSONcould silently influence unrelated tests, making failures order-dependent and hard to reproduce. The manual try/finally pattern for saving and restoring the old value is also noisy boilerplate that obscures the test's actual intent.A subtler problem with
session-scoped fixtures is thatmonkeypatchonly supportsfunctionandmodulescope — you can't use it withsession. The fix must therefore reduce the fixture scope tomodulebefore switching tomonkeypatch.What
Replace all direct
os.environmutations in tests withmonkeypatch.setenv. This is pytest's standard mechanism for environment isolation: it automatically reverts every change after the test (or module) finishes, even if the test raises. No manual save/restore oros.environ.popcalls are needed.Fixtures in
test_repo_source_link_integration.pyandtest_source_code_link_integration.pythat were previouslysession-scoped are downgraded tomodule-scoped so thatmonkeypatchcan be used safely. Thegit_repo_setupfixtures no longer setBUILD_WORKSPACE_DIRECTORYthemselves; each test that needs it sets it viamonkeypatch.setenvinstead, keeping environment side-effects visible at the call site.In
test_helper_lib.py, theos.chdircall intest_runfiles_dir_foundis also removed — it was unnecessary becauseget_runfiles_dir()reads the env var directly and never consults the working directory.Changes
test_repo_source_link_integration.py— downgradesphinx_base_dir,git_repo_setup,create_demo_filesfromsessiontomodulescope; removeos.environassignment fromgit_repo_setup; replace six directos.environ["BUILD_WORKSPACE_DIRECTORY"] = …assignments withmonkeypatch.setenvtest_source_code_link_integration.py— same scope downgrade; removeos.environassignment fromgit_repo_setup; replace two direct assignments withmonkeypatch.setenvtest_xml_parser.py— replace two manual try/finally save-restore patterns aroundKNOWN_GOOD_JSONwithmonkeypatch.setenvtest_helper_lib.py— replace manualos.environmutation andos.environ.popcleanup withmonkeypatch.setenv; remove the unnecessaryos.chdircall