-
Notifications
You must be signed in to change notification settings - Fork 8
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: add TestResultsFlow for measuring time to notification #439
base: main
Are you sure you want to change the base?
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: tasks/upload.py
Did you find this useful? React with a 👍 or 👎 |
3d33289
to
7638bed
Compare
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 399 399
Lines 33607 33630 +23
=======================================
+ Hits 32715 32738 +23
Misses 892 892
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 399 399
Lines 33607 33630 +23
=======================================
+ Hits 32715 32738 +23
Misses 892 892
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #439 +/- ##
=======================================
Coverage 97.34% 97.34%
=======================================
Files 399 399
Lines 33607 33630 +23
=======================================
+ Hits 32715 32738 +23
Misses 892 892
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
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 #439 +/- ##
=======================================
Coverage 97.37% 97.37%
=======================================
Files 430 430
Lines 34297 34320 +23
=======================================
+ Hits 33396 33419 +23
Misses 901 901
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes This change has been scanned for critical changes. Learn more |
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.
It might be interesting to add another checkpoint in the beginning of TestResultsFinisherTask.process_impl_within_lock
like TEST_RESULTS_PROCESSING_BEGIN
.
THe subflow TEST_RESULTS_BEGIN --> TEST_RESULTS_FINISHER_BEGIN
would give us a rough estimate of processing time. (I think that the finisher task only runs after all the processor tasks finish, and that we don't current have a metric for that)
What do you think?
But in general lgtm 👍
Very exciting to see more Flows
helpers/checkpoint_logger/flows.py
Outdated
@success_events("TEST_RESULTS_BEGIN") | ||
@subflows( | ||
("notification_latency", "TEST_RESULTS_BEGIN", "TEST_RESULTS_NOTIFY"), | ||
("notification_latency", "TEST_RESULTS_BEGIN", "FLAKE_DETECTION_NOTIFY"), |
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.
should the subflows have the same name?
Signed-off-by: joseph-sentry <[email protected]>
7638bed
to
20ec9ae
Compare
No description provided.