-
Notifications
You must be signed in to change notification settings - Fork 157
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make BaseCache an ABC #683
Merged
Dreamsorcerer
merged 13 commits into
aio-libs:master
from
padraic-shafer:abstract-base-cache
Mar 5, 2023
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0193759
Make BaseCache an abstract base class
padraic-shafer 8e04127
Add BaseCache subclasses for unit tests
padraic-shafer 9336bb3
Update fixtures for BaseCache tests
padraic-shafer e403214
Update unit tests for abstract BaseCache
padraic-shafer 6ecbfda
Remove unused changes
padraic-shafer 886415a
Update the changelog
padraic-shafer 135dfcd
Update decorator unit tests for abstract BaseCache
padraic-shafer a9bc7de
Define concrete SimpleMemoryBackend._close()
padraic-shafer 604b00d
Update backend unit tests for abstract BaseCache
padraic-shafer 8caa687
BaseCache._close() raises NotImplementedError
padraic-shafer 85c2ec5
Make backend base classes concrete
padraic-shafer 0fb9255
Revert BaseCache._close() to concrete definition: pass
padraic-shafer db33461
Update changelog for brevity
padraic-shafer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like these 2 classes are rather awkward, but I'll merge it for now.
I don't think there's any value in testing the logic of abstract methods, and I'm wondering if the other tests would still be testing the same thing if we just use the memory cache instead.. If so, we could then drop both of these classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also introduces a new mypy error:
https://github.com/aio-libs/aiocache/actions/runs/4333749520/jobs/7567135931
Curiously, only on the first method, though it looks like it should happen on all of them.
Maybe because it doesn't have an await...Because it's the only annotated method.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this class exists purely for tests, to allow mocks without throwing ABC
TypeError
s, I think adding# type: ignore[safe-super]
would be appropriate here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise that, it's just a side note to the first comment about whether we can just remove them if they are not testing anything useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's a bit awkward because the tests aren't being performed directly on
BaseCache
, and because of how many abstract methods need to be defined on the derived class. However the definitions ofAbstractBaseCache
andConcreteBaseCache
are trivial and transparent, so at least no new complexity is added.On the contrary, I really like how the logic of what's defined in
BaseCache
gets tested independently with the (abstract) base class.The derived classes used for the backend implementations use more resources and add enough "side effects" that proper testing of the underlying logic would require repeating these tests with all 3 backends. And even then, it would perhaps be an open question of whether the
BaseCache
functionality extends to new backends.Keeping these tests of
BaseCache
separate from tests of the backends seems much cleaner and more robust. Previously this was a bit simpler becauseBaseCache
was not formally abstract--it had unimplemented methods intended to be override, but instantiatingBaseCache
did not raise an exception.Well, in any case, I'm open to finding a cleaner test solution, but I favor keeping the
BaseCache
tests separate from the backend tests of the same methods.