Skip to content
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: test results processor return type #954

Closed
wants to merge 1 commit into from

Conversation

joseph-sentry
Copy link
Contributor

there's a sentry bug currently happening because the finisher is not handling one of the possible return values of the processor

so i'm generally changing this to make it clearer:

  • a processor either succeeds or fails, and this is represented by the return value being a boolean
    • a success means that the processor successfully parsed at least one file in any of the raw uploads it was processing
    • else, fail
  • since the finisher is called in a chord with the processor it will receive a list of such booleans, if there is any success in this list of results then there is some valid test result data
  • otherwise, we can just fail the finisher and make an error comment and try to notify coverage since there may be some valid coverage data

there's a sentry bug currently happening because the finisher is not
handling one of the possible return values of the processor

so i'm generally changing this to make it clearer:
- a processor either succeeds or fails, and this is represented by the
  return value being a boolean
  - a success means that the processor successfully parsed at least
    one file in any of the raw uploads it was processing
  - else, fail
- since the finisher is called in a chord with the processor it will
  receive a list of such booleans, if there is any success in this list
  of results then there is some valid test result data
- otherwise, we can just fail the finisher and make an error comment
  and try to notify coverage since there may be some valid coverage data
@joseph-sentry joseph-sentry requested a review from a team December 10, 2024 16:30
Copy link

sentry-io bot commented Dec 10, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: tasks/test_results_finisher.py

Function Unhandled Issue
process_impl_within_lock TypeError: list indices must be integers or slices, not str app.tasks.test_results.TestResultsF...
Event Count: 510
process_impl_within_lock TypeError: 'NoneType' object is not iterable app....
Event Count: 290
process_impl_within_lock AttributeError: 'NoneType' object has no attribute 'pullid' app.tasks.test_results.TestResultsF...
Event Count: 8
📄 File: tasks/test_results_processor.py (Click to Expand)
Function Unhandled Issue
run_impl TypeError: unsupported operand type(s) for *: 'NoneType' and 'int' app.tasks.test_results.Te...
Event Count: 54
run_impl FileNotInStorageError: File test_results/v1/raw/github/ghostwriter::::compliance/2c6b21e5a6c90b7c45a8b6f492711883ac28aa2... ...
Event Count: 12
process_individual_upload FileNotInStorageError: File test_results/v1/raw/github/getsentry::::sentry-python/e89490471c3ecf286f273cb87e71ce435d6053... ...
Event Count: 6
run_impl UnicodeDecodeError: 'utf-8' codec can't decode byte 0x98 in position 4: invalid start byte a...
Event Count: 6
---

Did you find this useful? React with a 👍 or 👎

@codecov-notifications
Copy link

codecov-notifications bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
tasks/test_results_processor.py 85.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.99%. Comparing base (e22c04e) to head (94e650f).
Report is 5 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
tasks/test_results_processor.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #954      +/-   ##
==========================================
- Coverage   97.99%   97.99%   -0.01%     
==========================================
  Files         443      443              
  Lines       35721    35712       -9     
==========================================
- Hits        35005    34996       -9     
  Misses        716      716              
Flag Coverage Δ
integration 42.11% <95.00%> (-0.02%) ⬇️
unit 90.65% <5.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

✅ All tests successful. No failed tests were found.

📣 Thoughts on this report? Let Codecov know! | Powered by Codecov

@joseph-sentry joseph-sentry marked this pull request as ready for review December 10, 2024 21:22
Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, though this should probably be split into 3 different PRs/deploys:

  • make the finisher forward compatible with the new signature of the processor
  • change the processor to emit the new result type
  • remove all the compatibility code

@@ -149,7 +149,7 @@ def process_impl_within_lock(
db_session.add(totals)
db_session.flush()

if self.check_if_no_success(previous_result):
if not self.check_if_any_success(previous_result):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not self.check_if_any_success(previous_result):
if not any(previous_result):

The fn is now so simple you don’t need it anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants