feat(test results): error message PR comment if no successful parse #358
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: tasks/test_results_finisher.py
Did you find this useful? React with a 👍 or 👎 |
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #358 +/- ##
=======================================
Coverage 97.44% 97.44%
=======================================
Files 395 395
Lines 33422 33514 +92
=======================================
+ Hits 32568 32658 +90
- Misses 854 856 +2
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. Additional details and impacted files@@ Coverage Diff @@
## main #358 +/- ##
==========================================
- Coverage 97.44% 97.44% -0.01%
==========================================
Files 395 395
Lines 33430 33514 +84
==========================================
+ Hits 32577 32658 +81
- Misses 853 856 +3
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 #358 +/- ##
==========================================
- Coverage 97.44% 97.44% -0.01%
==========================================
Files 395 395
Lines 33430 33514 +84
==========================================
+ Hits 32577 32658 +81
- Misses 853 856 +3
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 #358 +/- ##
==========================================
- Coverage 97.47% 97.47% -0.01%
==========================================
Files 426 426
Lines 34121 34205 +84
==========================================
+ Hits 33259 33340 +81
- Misses 862 865 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 17 files with indirect coverage changes
|
Closed
3 tasks
joseph-sentry
force-pushed
the
joseph/test-error-comment
branch
2 times, most recently
from
c2afcbb
to
51268e3
Compare
giovanni-guidini
previously approved these changes
Apr 9, 2024
joseph-sentry
force-pushed
the
joseph/test-error-comment
branch
3 times, most recently
from
dd7aa1b
to
aab542a
Compare
joseph-sentry
force-pushed
the
joseph/test-error-comment
branch
2 times, most recently
from
8dd6d4f
to
7bf5756
Compare
services/comparison/__init__.py
Outdated
| @@ -29,6 +29,7 @@ class NotificationContext(object): | |||
| """Extra information not necessarily related to coverage that may affect notifications""" | |||
|
|
|||
| all_tests_passed: bool | |||
| test_results_error: TestResultsProcessingError | None = False | |||
There was a problem hiding this comment.
Shouldn't the default value be None? To follow the type annotation.
| yield ( | ||
| ":white_check_mark: All tests successful. No failed tests found :relaxed:" | ||
| ) | ||
| if comparison.test_results_error(): |
There was a problem hiding this comment.
So... if there are no tests then we assume all tests passed? Crazy... Can we fix that? Maybe by checking that there's at least 1 test in the "all tests passed".
I didn't realize this was a possibility before 😅
joseph-sentry
force-pushed
the
joseph/test-error-comment
branch
from
7bf5756
to
1fc9b1b
Compare
giovanni-guidini
previously approved these changes
Apr 25, 2024
| @@ -47,10 +47,16 @@ def header_lines(self, comparison: ComparisonProxy, diff, settings) -> List[str] | |||
| hide_project_coverage = settings.get("hide_project_coverage", False) | |||
| if hide_project_coverage: | |||
| if comparison.all_tests_passed(): | |||
There was a problem hiding this comment.
Shouldn't this bit follow the same logic as above? For consistency
if comparison.test_results_error():
...
elif comparison.all_tests_passed():
...
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
This commit: - adds the TestResultsProcessingError enum - adds the enum as a nullable field in the test results total - in the test results finisher, when there are no succesful parses, it writes to the error field in the totals - it immediately writes the error message to the comment, then queues up a notify - in the notify step, it will read from the totals to check if there was a test result processing error, if there was, let the user know by displaying it in the coverage comment, similary to how we show succesful parsing of test results
The logic for doing this is as follows: If there are no successful test results processing tasks this chord then we check if there were previously test instances that were processed for this commit. If there are previous test instances on this commit, there is no error, and we just exit this finisher without doing anything. If there are no previous test instances on this commit, then we should notify the user that there is an error. So we will begin by doing a test results error comment which contains no info about coverage, then we will queue up the notify task so it can create a coverage comment that still contains this error's information. If there are successful test results processing tasks, then we should get rid of the error on the test results totals for this commit. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
If there has been any success in processing for a given commit, it's associated test result totals should have the error set to None, otherwise it should be NO_SUCCESS. The condition for all_tests_passed is: if the error is None and failed = 0 in the totals then all tests passed. In the comment, if there's an error, show the error comment, else if all tests passed show the all tests passed message. If neither of these are true and the user HAS uploaded test results, then it means that there must have been failures, or they somehow skipped all their tests. Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
Signed-off-by: joseph-sentry <joseph.sawaya@sentry.io>
joseph-sentry
force-pushed
the
joseph/test-error-comment
branch
from
0b895c4
to
e8b369a
Compare
nora-codecov
approved these changes
May 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: codecov/test-results-action#15