Make BaseCache an ABC #683
Conversation
Codecov Report
@@ Coverage Diff @@
## master #683 +/- ##
==========================================
- Coverage 99.67% 99.63% -0.05%
==========================================
Files 35 35
Lines 3748 3793 +45
==========================================
+ Hits 3736 3779 +43
- Misses 12 14 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
padraic-shafer
force-pushed
the
abstract-base-cache
branch
from
e25690a
to
8caa687
Compare
|
|
||
|
|
||
| class AbstractBaseCache(BaseCache[str]): | ||
| """BaseCache that can be mocked for NotImplementedError tests""" |
There was a problem hiding this comment.
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.
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.
Also introduces a new mypy error: https://github.com/aio-libs/aiocache/actions/runs/4333749520/jobs/7567135931
Because this class exists purely for tests, to allow mocks without throwing ABC TypeErrors, I think adding # type: ignore[safe-super] would be appropriate here.
There was a problem hiding this comment.
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.
I feel like these 2 classes are rather awkward, but I'll merge it for now.
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 of AbstractBaseCache and ConcreteBaseCache are trivial and transparent, so at least no new complexity is added.
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.
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 because BaseCache was not formally abstract--it had unimplemented methods intended to be override, but instantiating BaseCache 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.
What do these changes do?
BaseCacheis now an abstract base class. Attempting to instantiate aBaseCacheobject raises aTypeError.Are there changes in behavior for the user?
Generally not, because users have always been expected to create cache objects using a subclass of
BaseCache.Now a
TypeErroris raised at the point of attempted cache creation, if the user attempts to instantiate aBaseCacheobject. Previously aNotImplementedErrorwas raised at the point where an abstract method was called.Related issue number
#676
Checklist
CHANGESfolder<issue_id>.<type>(e.g.588.bugfix)issue_idchange it to the pr id after creating the PR.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.