Skip to content
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 SimpleMemoryBackend store state in instance instead of class #562

Merged

Conversation

Fogapod
Copy link
Contributor

@Fogapod Fogapod commented Mar 18, 2022

What do these changes do?

Make SimpleMemoryBackend store state in instance instead of class.
While current behaviour matches other caches (same connection args == same cache; zero connection args in memory case), current design seems bad since it does not allow extending and customizing memory cache in future.

Tests adjusted accordingly.

Are there changes in behavior for the user?

Yes, if user for some reason expected different SimpleMemoryBackend objects to have shared storage. This is probably a breaking change for some weird setups.

Related issue number

Resolves: #531

Resolves: #479
There is a PR aimed to fix it: #523
But using namespace seem to not be sufficient. Instance local cache must be used for consistent behaviour.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .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.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

Copy link
Member

@Dreamsorcerer Dreamsorcerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, sorry for the delay.

Could you update to latest master?

aiocache/backends/memory.py Outdated Show resolved Hide resolved
@Fogapod Fogapod force-pushed the memorybackend-instance-local-cache branch from d20f4f7 to f5d2e4a Compare December 31, 2022 11:32
@Fogapod
Copy link
Contributor Author

Fogapod commented Dec 31, 2022

Resolved conflicts

@Dreamsorcerer
Copy link
Member

Should probably have a regression test too, but I'll merge this in for now, as it's pretty straightforward.

@Dreamsorcerer
Copy link
Member

Ah, seems you missed updating some tests. 28 failing tests currently.

@Fogapod
Copy link
Contributor Author

Fogapod commented Dec 31, 2022

I'll fix those and test locally a bit later

@Fogapod
Copy link
Contributor Author

Fogapod commented Jan 1, 2023

These weren't tests, there was one more line using class var added after this PR, now fixed

@Dreamsorcerer
Copy link
Member

Ah, I clearly didn't look closely enough.

@Fogapod
Copy link
Contributor Author

Fogapod commented Jan 1, 2023

I followed CONTRIBUTING.rst and ran make test after installing dev dependencies. All my local tests except memcached completed successfully (i don't have it installed).
Is there something I'm doing wrong

@Dreamsorcerer
Copy link
Member

I've not looked at those instructions, so they could be outdated. Ultimately, whatever the CI does, is the correct way to run it: https://github.com/aio-libs/aiocache/blob/master/.github/workflows/ci.yml#L103-L106

@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Jan 1, 2023

It's just 1 test class failing now, so should be able to run it with pytest 'tests/acceptance/test_plugins.py::TestHitMissRatioPlugin'

@Dreamsorcerer
Copy link
Member

Looks like it just needs this setup line needs to be updated: SimpleMemoryBackend._cache = data in both tests.

@Fogapod
Copy link
Contributor Author

Fogapod commented Jan 2, 2023

tests/acceptance/test_plugins.py::TestHitMissRatioPlugin now passes locally and there are no more uses of SimpleMemoryBackend in code

@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #562 (87fe720) into master (781af66) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
+ Coverage   99.60%   99.66%   +0.05%     
==========================================
  Files          36       36              
  Lines        3560     3557       -3     
  Branches      447        0     -447     
==========================================
- Hits         3546     3545       -1     
  Misses         12       12              
+ Partials        2        0       -2     
Flag Coverage Δ
unit 99.66% <100.00%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiocache/backends/memory.py 100.00% <100.00%> (ø)
tests/acceptance/test_plugins.py 100.00% <100.00%> (ø)
tests/ut/backends/test_memory.py 100.00% <100.00%> (ø)
aiocache/backends/redis.py 99.16% <0.00%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 781af66...87fe720. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants