Bug hunt/mockey autospec#965
Open
anisia19 wants to merge 4 commits into
Open
Conversation
Problem: mock.patch('module.input') and mock.patch('module.open') relied on
mock.patch's undocumented implicit create=True behaviour for names that appear
in the _builtins set. Mockey enforces that the attribute already exists on the
target before patching, so it raised AttributeError for both:
- detect_secrets.audit.io.input (used in run_logic in audit_test.py)
- detect_secrets.core.scan.open (used in test_error_reading_file)
Neither 'input' nor 'open' is imported into those modules; they are resolved
via Python's builtin lookup at call time. Without create=True the patches
were technically creating a new module attribute behind the scenes and then
deleting it on teardown — the intent was implicit.
Fix: add mockey to the test requirements and initialise it in tests/__init__.py
plus conftest.py (MockAutospecFixture). Add create=True explicitly to both
patches so the intent is visible and mockey skips the autospec check for these
builtin injections.
Problem: test_fails_if_no_line_numbers_found called
mock.patch('detect_secrets.audit.io.clear_screen') inside the test body, but
the autouse fixture prevent_clear_screen in conftest.py already holds an active
patch on that same attribute. Mockey raises InvalidSpecError when a second
patch tries to autospec an object that is already a Mock — it cannot create an
autospec of a mock.
The duplicate patch was also unnecessary: the autouse fixture guarantees
clear_screen is always mocked, so the only missing piece was a reference to
that existing mock for the assertion.
Fix: expose the mock from prevent_clear_screen by yielding it instead of plain
yield, then accept it as a fixture parameter in the test and assert on it
directly, removing the redundant inner mock.patch call.
…ospec
Problem: mockey enforces return type hints — when a patched function declares
-> bool, _lazy_autospec_method unconditionally sets mock.return_value to a new
_AutospecMagicMock(autospec=bool). This happened *after* mock.patch had
already applied the return_value kwarg from the patch call, so the caller's
value was silently discarded.
Two places depended on this pattern:
1. disable_gibberish_filter (testing/mocks.py) patched is_feature_enabled
(-> bool) with return_value=False. The override made it return a truthy
mock instead, so the gibberish filter was never actually disabled. This
caused test_supports_stdin to see 'AWSKeyDetector: False' (secret filtered
out) and test_baseline_filters_out_known_secrets to return exit code 3.
2. test_verify_invalid_secret patched verify_aws_secret_access_key (-> bool)
with return_value=False. The truthy mock caused the detector to always
return VERIFIED_TRUE regardless of the intended scenario.
Fix: separate mock creation from return_value assignment. Open the context
manager to get a reference to the mock, then set return_value explicitly inside
the with block, where it overrides whatever autospec installed as the default.
Problem: TestAWSKeyDetector and TestAnalyzeLine both defined a setup() method to initialise per-test instance state (self.example_key, self.filename, etc.). In pytest < 8.0 this worked because setup() was recognised as an alias for setup_method() on non-unittest classes. pytest 8.x removed that alias — _register_setup_method_fixture now only looks up 'setup_method' by name, so setup() was silently never called. Every test in both classes that read self.example_key or self.filename failed with AttributeError. Fix: rename setup() to setup_method() in both classes so pytest 8.x picks it up correctly.
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
1 participant
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.
Please check if the PR fulfills these requirements
Tests for the changes have been added — the fixes are all in the test layer itself; no new production code was changed
Docs have been added / updated — N/A (test-only changes)
All CI checks are green — run locally: 1033 passed, 6 xfailed
What kind of change does this PR introduce?
Bug fixes in the test suite, surfaced by adding mockey autospec enforcement.
What is the current behavior?
Four silent test bugs existed:
MockAutospecFixture is now active for every test run via tests/init.py + an autouse pytest fixture in conftest.py. All four underlying issues are fixed: create=True is explicit, the double-patch is removed, return_value is set after mock creation, and setup_method() is used throughout.
No. All changes are confined to the test suite and test helpers (testing/mocks.py). No production code was modified.
The three mockey-enforced bugs (1–3) were pre-existing silent failures: the tests appeared to pass but were not actually exercising the intended behaviour. Bug 4 was a pytest 8.x compatibility issue unrelated to mockey