-
Notifications
You must be signed in to change notification settings - Fork 415
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
feat(pg-usage-data): Add cache to batch tracking data #4308
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
|
|
|
|
|
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/Flagsmith/flagsmith/pull/4308 ⚙️ Updating now by workflow run 9958175504. What is Uffizzi? Learn more! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4308 +/- ##
==========================================
+ Coverage 96.60% 96.78% +0.17%
==========================================
Files 1195 1162 -33
Lines 39099 38154 -945
==========================================
- Hits 37773 36927 -846
+ Misses 1326 1227 -99 ☔ View full report in Codecov by Sentry. |
Docker builds report
|
frozen_time.tick(CACHE_FLUSH_INTERVAL + 1) | ||
|
||
# Next, let's call flush | ||
cache.flush() |
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 think we have this pattern elsewhere so we're probably just following that, but it's odd to me that we're calling flush
directly here since I don't think any external calls are made to it. I think we should probably mark the flush
method as private, and this test should only use the track_request
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.
Although It's not being used, but I feel it makes the interface a bit more clean? Does that make sense?
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.
Hmm... maybe it does, yes, but the test feels a bit too 'internal' to me. It's not really testing the actual behaviour.
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, that's a fair point! I have made it private
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.
Overall looks good to me, just a bunch of typing suggestions.
path: str, | ||
enum_resource_value: int, | ||
settings: SettingsWrapper, | ||
): |
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.
nit: Missing typing for return value
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.
Done
rf: RequestFactory, mocker: MockerFixture, settings: SettingsWrapper | ||
): |
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.
Nit: Missing typing for return value
from pytest_mock import MockerFixture | ||
|
||
|
||
def test_api_usage_cache(mocker: MockerFixture): |
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.
Nit: Missing typing for return value
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingdocs/
if required so people know about the feature!Changes
Add cache to batch usage tracking
fixes: #4241
How did you test this code?
Adds unit tests