Get rid of mocks for causes in the tests of registries #521
Conversation
This file contains 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
Causes need to be checked with `isinstance()` in the code base, to properly pass the type checking: `isinstance()` adjusts the inferred type for operations that follow the check. An example is handler-matching functions that check for causes to be resource-changing causes, and using `.diff` only after that (`.diff` is absent in all other causes). However, this does not work with mocks in tests: all tests do fail, because `Mock` and `MagicMock` classes do not pass the `isinstance()` criterion. Besides that, mocks are just a terrible way of testing: first of all, they leak the abstractions of the mocked objects by knowing which fields they contain, which properties they implement and how (and can mimic them wrong); second, they prevent tests to be type-checked both by type-checkers or by IDEs.
nolar
force-pushed
the
cause-factory-in-tests
branch
from
0795e49 to
91875a4
Compare
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.
What do these changes do?
Refactor tests that previously used mocks for causes — to use proper causes' classes. Mocks are bad.
Description
Causes need to be checked with
isinstance()in the code base, to properly pass the type checking:isinstance()adjusts the inferred type for operations that follow the check. An example is handler-matching functions that check for causes to be resource-changing causes, and using.diffonly after that (.diffis absent in all other causes).However, this does not work with mocks in tests: all tests do fail, because
MockandMagicMockclasses do not pass theisinstance()criterion.Besides that, mocks are just a terrible way of testing: first of all, they leak the abstractions of the mocked objects by knowing which fields they contain, which properties they implement and how (and can mimic them wrong); second, they prevent tests to be type-checked both by type-checkers or by IDEs.
This PR improves the test design for registries & causes so that the following PRs can also rely on proper typing of arguments (e.g. #518).
Issues/PRs
Type of changes
Checklist
CONTRIBUTORS.txt