test(tools): auto-patch release CLI helpers in conftest.py#3939
test(tools): auto-patch release CLI helpers in conftest.py#3939rickeylev wants to merge 1 commit into
Conversation
Add an autouse fixture in conftest.py for tests/tools/private/release that automatically patches run_cmd, Git._run_git, and GitHub._run_gh using pytest-mock. This prevents tests from executing command line tools that could have side-effects. TAG=agy CONV=b4972743-3d1e-4e9a-9623-cf1f25cfdd83
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an autouse fixture auto_patch_cmd_helpers in a new conftest.py file to automatically mock command-line helpers (run_cmd, Git._run_git, and GitHub._run_gh) during testing, along with a corresponding test in gh_test.py to verify the mocks. The review feedback suggests simplifying this setup by only mocking run_cmd, as mocking the internal class methods is redundant and makes writing realistic unit tests more difficult. Code suggestions are provided to simplify both the fixture and the test assertions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@aignas any thoughts on conftest.py integration? In this PR, a conftest.py is added to a py_library that defines a fixture that should be automatically used, and things just magically work. Which is kind of neat. Originally, though, I was thinking that pytest_test would glob conftest.py by default. I think that better matches pytest semantics? |
|
I agree, maybe just globbing |
dougthor42
left a comment
dougthor42
left a comment
There was a problem hiding this comment.
I was thinking that pytest_test would glob conftest.py by default
Though gazelle has some logic of including the conftest.py, so the two would be clashing
Agreed, gazelle already has python_include_ancestor_conftest so it would be a little strange for pytest_test to glob all of them.
For the record, we (Quantum) have actively been moving away from using pytest's conftest magic precisely because goes against the "explicit dependencies" Bazel idiom.
We set both directives python_include_ancestor_conftest=False and include_pytest_conftest=False because our pre-Bazel conftest files had accumulated loads of cruft, and would pollute the dependency tree: a change in file A would bust the conftest.py cache and then bust the cache of every downstream target, even if those targets didn't use the fixtures defined in that conftest.
The docs for python_include_ancestor_conftest shows an example of 4 different conftest files getting added as dependencies. This mimics what pytest does under the hood, but it also means that my_test.py's cache is much more fragile. If my_test.py doesn't use any of the fixtures defined in //:conftest (even auto-used ones), then why does it depend on //:conftest?
This PR looks reasonable to me. All comments are optional / questions.
|
|
||
|
|
||
| @pytest.fixture(name="auto_patch_cmd_helpers", autouse=True) | ||
| def fixture_auto_patch_cmd_helpers(mocker): |
There was a problem hiding this comment.
Optional Nit: More idiomatic pytest would be to have a single fixture for each item, and then a 4th fixture to collect them all.
| mocker.patch("tools.private.release.gh.run_cmd", mock_run_cmd) | ||
| mock_run_git = mocker.patch("tools.private.release.git.Git._run_git") | ||
| mock_run_gh = mocker.patch("tools.private.release.gh.GitHub._run_gh") | ||
| return { |
There was a problem hiding this comment.
I recommend returning a dataclass instead of dict - helps with type checking (do we do type checking in this project? 🤣 )
|
|
||
|
|
||
| @pytest.fixture(name="auto_patch_cmd_helpers", autouse=True) | ||
| def fixture_auto_patch_cmd_helpers(mocker): |
There was a problem hiding this comment.
Might be out of scope, but why use pytest_mock over just plain-ol unittest.mock API? Or pytest's built in monkeypatch API?
| def test_auto_patched_helpers_prevent_real_execution(auto_patch_cmd_helpers): | ||
| from tools.private.release.gh import GitHub | ||
| from tools.private.release.git import Git | ||
| from tools.private.release.shell import run_cmd |
There was a problem hiding this comment.
The gh_test target needs to be updated to include these as deps, no?
3 participants
Add an autouse fixture in conftest.py for tests/tools/private/release that automatically patches run_cmd, Git._run_git, and GitHub._run_gh using pytest-mock. This prevents tests from executing command line tools that could have side-effects.