Abstract os.environ so real env doesn't interfere with tests#61
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR abstracts direct os.environ accesses into injectable mappings for both the CI environment detection and API client, allowing real code to use os.environ and tests to pass controlled dictionaries.
- Introduces
RunEnvBuilderto encapsulate environment‐based CI detection and removes the olddetect_envfunction. - Adds an
APIclass wrapping the previoussubmitfunction, taking an env mapping in its constructor. - Refactors tests and the pytest plugin to use the new builder and API classes instead of patching
os.environdirectly.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/buildkite_test_collector/collector/test_run_env.py | Refactored calls to use RunEnvBuilder instead of detect_env |
| tests/buildkite_test_collector/collector/test_payload.py | Changed import to only bring in timedelta |
| tests/buildkite_test_collector/collector/test_api.py | Swapped detect_env/submit for RunEnvBuilder/API calls |
| src/buildkite_test_collector/pytest_plugin/init.py | Updated plugin to build RunEnv via builder and submit via API |
| src/buildkite_test_collector/collector/run_env.py | Added RunEnvBuilder, removed standalone detect_env |
| src/buildkite_test_collector/collector/api.py | Replaced free submit with an API class |
Comments suppressed due to low confidence (1)
tests/buildkite_test_collector/collector/test_payload.py:1
- The test still references
datetimebut it was removed from the import. Re-adddatetimealongsidetimedelta(e.g.from datetime import datetime, timedelta).
from datetime import timedelta
tommeier
approved these changes
Jul 18, 2025
Merged
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.
Two parts of this collector need to access environment variables:
RunEnvuses it to discover & constructrun_env(CI environment details e.g. build message)submit()uses it to get API key/URL.Previously we were reaching into
os.environthroughout the code, and mocking it in tests like this:Now, we wrap those environment accesses behind a
Mapping[str, Optional[str]]type; real code passes inos.environ, and tests pass in an arbitrary dict.Diff best viewed with whitespace ignored, there's quite a bit of indentation change from removing the
with mock…:blocks.Related:
RuntimeEnvironmenttoRunEnv#60