-
Notifications
You must be signed in to change notification settings - Fork 28
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
Use shared
report_service and test factories
#849
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #849 +/- ##
==========================================
- Coverage 96.26% 96.25% -0.02%
==========================================
Files 823 823
Lines 19235 19049 -186
==========================================
- Hits 18516 18335 -181
+ Misses 719 714 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
❌ 3 Tests Failed:
View the top 3 failed tests by shortest run time
To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard |
Test Failures Detected: Due to failing tests, we cannot provide coverage reports at this time. ❌ Failed Test Results:Completed 2712 tests with View the full list of failed testspytest
|
This PR includes changes to |
12d6b59
to
a57b56c
Compare
a57b56c
to
1eb29d3
Compare
This PR includes changes to |
report_builder
featureshared
report_service and test factories
895de8d
to
3c5e3b1
Compare
This PR includes changes to |
3c5e3b1
to
076573a
Compare
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.
love this cleanup, only question is about OwnerFactory
- I see some spots where you've swapped it to the shared
factory, and others where you've left it referring to the api
factory, any reason other than this pr getting too big?
This might just be an oversight. |
076573a
to
c5e6616
Compare
This PR includes changes to |
de1361f
to
0510f64
Compare
d3ee9b5
to
640f16e
Compare
I rebased the PR, and fixed all of the remaining tests, so things are green 🎉 |
640f16e
to
f9a15d8
Compare
This PR includes changes to |
267ac89
to
e4f56a9
Compare
This switches most of the code using the `report_builder` service to use that same code imported from `shared` (except some functions specific to `api`). Similarly, the `core.tests.factories` were also duplicated in shared, so this switches over to those factories, deleting them from `api`.
e4f56a9
to
f26a4e4
Compare
Purpose/Motivation
This switches most of the code using the
report_builder
service to use that same code imported fromshared
(except some functions specific toapi
).Similarly, the
core.tests.factories
were also duplicated in shared, so this switches over to those factories, deleting them fromapi
.The primary reason here is that the
shared
code has been changed to remove the obsoletereport_builder
feature.This feature was recently removed from
worker
and its configuration from thek8s
config.It is currently not enabled in production.
I believe this feature does not pull its weight, as querying multiple tables and fetching the
files_array
file from storage is most likely slower than just fetching thereport_json
from storage instead.Links to relevant tickets
codecov/engineering-team#2554