-
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
fix: Fix evironment metadata N+1 for environments list #2947
fix: Fix evironment metadata N+1 for environments list #2947
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uffizzi Preview |
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.
Think we should try to add new tests under the tests
directory only. The tests here should live under /tests/unit/environment/test_views
, right @matthewelwell ?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2947 +/- ##
==========================================
+ Coverage 95.64% 95.68% +0.04%
==========================================
Files 1012 1011 -1
Lines 29173 29197 +24
==========================================
+ Hits 27902 27938 +36
+ Misses 1271 1259 -12
☔ View full report in Codecov by Sentry. |
Sounds good. I've moved all the tests into the target file, including the one I wrote. |
Thanks for submitting a PR! Please check the boxes below:
pre-commit
to check lintingChanges
Isolated the cause of an N+1 query and implemented a fix without forcing it on every caller. We may want to consider reviewing the callers and seeing which of them load metadata as well, since the bulk of the performance enhancement is already complete.
How did you test this code?
One new test. During the testing process I also manually inspecting the types of queries that were generated to ensure that they matched those reported by Sentry.