Make `ComparisonProxy` sync #730

Swatinem · 2024-09-20T13:16:54Z

As calling async_to_sync within an async context, this necessitated also turning all the various notification related functionality sync.

codecov-staging · 2024-09-20T13:21:03Z

Codecov Report

Attention: Patch coverage is 99.09400% with 8 lines in your changes missing coverage. Please review.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
services/comparison/__init__.py	95.40%	4 Missing ⚠️
services/notification/notifiers/checks/base.py	90.90%	1 Missing ⚠️
...ervices/notification/notifiers/comment/__init__.py	96.15%	1 Missing ⚠️
services/notification/notifiers/status/base.py	96.29%	1 Missing ⚠️
tasks/notify.py	85.71%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
- Coverage   98.07%   98.05%   -0.02%     
==========================================
  Files         434      434              
  Lines       36800    36466     -334     
==========================================
- Hits        36091    35758     -333     
+ Misses        709      708       -1

Flag	Coverage Δ
integration	`98.05% <99.09%> (-0.02%)`	⬇️
latest-uploader-overall	`98.05% <99.09%> (-0.02%)`	⬇️
unit	`98.05% <99.09%> (-0.02%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.95% <97.67%> (-0.02%)`	⬇️
OutsideTasks	`98.07% <99.18%> (-0.03%)`	⬇️

Files with missing lines	Coverage Δ
helpers/notifier.py	`98.18% <100.00%> (+0.03%)`	⬆️
services/bundle_analysis/notify/__init__.py	`100.00% <100.00%> (ø)`
...rvices/bundle_analysis/notify/messages/__init__.py	`80.00% <100.00%> (ø)`
...ervices/bundle_analysis/notify/messages/comment.py	`100.00% <100.00%> (ø)`
...s/bundle_analysis/notify/messages/commit_status.py	`100.00% <100.00%> (ø)`
...dle_analysis/notify/messages/tests/test_comment.py	`100.00% <100.00%> (ø)`
...alysis/notify/messages/tests/test_commit_status.py	`100.00% <100.00%> (ø)`
...undle_analysis/notify/tests/test_notify_service.py	`100.00% <100.00%> (ø)`
services/comparison/overlays/critical_path.py	`100.00% <100.00%> (ø)`
...omparison/tests/unit/overlay/test_critical_path.py	`100.00% <100.00%> (ø)`
... and 45 more

codecov · 2024-09-20T13:21:11Z

Codecov Report

Attention: Patch coverage is 99.09400% with 8 lines in your changes missing coverage. Please review.

Project coverage is 98.10%. Comparing base (a7cb347) to head (b07ef5c).
Report is 2 commits behind head on main.

Changes have been made to critical files, which contain lines commonly executed in production. Learn more

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
services/comparison/__init__.py	95.40%	4 Missing ⚠️
services/notification/notifiers/checks/base.py	90.90%	1 Missing ⚠️
...ervices/notification/notifiers/comment/__init__.py	96.15%	1 Missing ⚠️
services/notification/notifiers/status/base.py	96.29%	1 Missing ⚠️
tasks/notify.py Critical	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
- Coverage   98.12%   98.10%   -0.02%     
==========================================
  Files         475      475              
  Lines       38155    37820     -335     
==========================================
- Hits        37438    37105     -333     
+ Misses        717      715       -2

Flag	Coverage Δ
integration	`98.05% <99.09%> (-0.02%)`	⬇️
latest-uploader-overall	`98.05% <99.09%> (-0.02%)`	⬇️
unit	`98.05% <99.09%> (-0.02%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`96.07% <97.67%> (-0.01%)`	⬇️
OutsideTasks	`98.07% <99.18%> (-0.03%)`	⬇️

Files with missing lines	Coverage Δ
helpers/notifier.py	`98.18% <100.00%> (+0.03%)`	⬆️
services/bundle_analysis/notify/__init__.py	`100.00% <100.00%> (ø)`
...rvices/bundle_analysis/notify/messages/__init__.py	`80.00% <100.00%> (ø)`
...ervices/bundle_analysis/notify/messages/comment.py	`100.00% <100.00%> (ø)`
...s/bundle_analysis/notify/messages/commit_status.py	`100.00% <100.00%> (ø)`
...dle_analysis/notify/messages/tests/test_comment.py	`100.00% <100.00%> (ø)`
...alysis/notify/messages/tests/test_commit_status.py	`100.00% <100.00%> (ø)`
...undle_analysis/notify/tests/test_notify_service.py	`100.00% <100.00%> (ø)`
services/comparison/overlays/critical_path.py	`100.00% <100.00%> (ø)`
...omparison/tests/unit/overlay/test_critical_path.py	`100.00% <100.00%> (ø)`
... and 45 more

Related Entrypoints
run/app.tasks.status.SetError
run/app.tasks.notify.Notify
run/app.tasks.compute_comparison.ComputeComparison
run/app.tasks.test_results.TestResultsFinisherTask

codecov-qa · 2024-09-20T13:21:14Z

Codecov Report

Attention: Patch coverage is 99.09400% with 8 lines in your changes missing coverage. Please review.

Project coverage is 98.05%. Comparing base (a7cb347) to head (b07ef5c).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
services/comparison/__init__.py	95.40%	4 Missing ⚠️
services/notification/notifiers/checks/base.py	90.90%	1 Missing ⚠️
...ervices/notification/notifiers/comment/__init__.py	96.15%	1 Missing ⚠️
services/notification/notifiers/status/base.py	96.29%	1 Missing ⚠️
tasks/notify.py	85.71%	1 Missing ⚠️

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
- Coverage   98.07%   98.05%   -0.02%     
==========================================
  Files         434      434              
  Lines       36800    36466     -334     
==========================================
- Hits        36091    35758     -333     
+ Misses        709      708       -1

Flag	Coverage Δ
integration	`98.05% <99.09%> (-0.02%)`	⬇️
latest-uploader-overall	`98.05% <99.09%> (-0.02%)`	⬇️
unit	`98.05% <99.09%> (-0.02%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.95% <97.67%> (-0.02%)`	⬇️
OutsideTasks	`98.07% <99.18%> (-0.03%)`	⬇️

Files with missing lines	Coverage Δ
helpers/notifier.py	`98.18% <100.00%> (+0.03%)`	⬆️
services/bundle_analysis/notify/__init__.py	`100.00% <100.00%> (ø)`
...rvices/bundle_analysis/notify/messages/__init__.py	`80.00% <100.00%> (ø)`
...ervices/bundle_analysis/notify/messages/comment.py	`100.00% <100.00%> (ø)`
...s/bundle_analysis/notify/messages/commit_status.py	`100.00% <100.00%> (ø)`
...dle_analysis/notify/messages/tests/test_comment.py	`100.00% <100.00%> (ø)`
...alysis/notify/messages/tests/test_commit_status.py	`100.00% <100.00%> (ø)`
...undle_analysis/notify/tests/test_notify_service.py	`100.00% <100.00%> (ø)`
services/comparison/overlays/critical_path.py	`100.00% <100.00%> (ø)`
...omparison/tests/unit/overlay/test_critical_path.py	`100.00% <100.00%> (ø)`
... and 45 more

codecov-public-qa · 2024-09-20T13:21:40Z

Codecov Report

Attention: Patch coverage is 99.09400% with 8 lines in your changes missing coverage. Please review.

Project coverage is 98.05%. Comparing base (a7cb347) to head (b07ef5c).

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
- Coverage   98.07%   98.05%   -0.02%     
==========================================
  Files         434      434              
  Lines       36800    36466     -334     
==========================================
- Hits        36091    35758     -333     
+ Misses        709      708       -1

Flag	Coverage Δ
integration	`98.05% <99.09%> (-0.02%)`	⬇️
latest-uploader-overall	`98.05% <99.09%> (-0.02%)`	⬇️
unit	`98.05% <99.09%> (-0.02%)`	⬇️

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

Components	Coverage Δ
NonTestCode	`95.95% <97.67%> (-0.02%)`	⬇️
OutsideTasks	`98.07% <99.18%> (-0.03%)`	⬇️

Files	Coverage Δ
helpers/notifier.py	`98.18% <100.00%> (+0.03%)`	⬆️
services/bundle_analysis/notify/__init__.py	`100.00% <100.00%> (ø)`
...rvices/bundle_analysis/notify/messages/__init__.py	`80.00% <100.00%> (ø)`
...ervices/bundle_analysis/notify/messages/comment.py	`100.00% <100.00%> (ø)`
...s/bundle_analysis/notify/messages/commit_status.py	`100.00% <100.00%> (ø)`
...dle_analysis/notify/messages/tests/test_comment.py	`100.00% <100.00%> (ø)`
...alysis/notify/messages/tests/test_commit_status.py	`100.00% <100.00%> (ø)`
...undle_analysis/notify/tests/test_notify_service.py	`100.00% <100.00%> (ø)`
services/comparison/overlays/critical_path.py	`100.00% <100.00%> (ø)`
...omparison/tests/unit/overlay/test_critical_path.py	`100.00% <100.00%> (ø)`
... and 45 more

As calling `async_to_sync` within an async context, this necessitated also turning all the various `notification` related functionality sync.

giovanni-guidini

🎉 LGTM, very nice

giovanni-guidini · 2024-09-23T11:28:16Z

tasks/status_set_error.py

@@ -59,18 +59,9 @@ def run_impl(self, db_session, repoid, commitid, *, message=None, **kwargs):
                            message or "Coverage not measured fully because CI failed"
                        )
                        if context in statuses:
-                            # async_to_sync has its own "context" argument so we


Is this comment wrong? Or simply by passing the argument as arg (instead of kwarg) we sidestep async_to_sync having a context?

simply by passing the argument as arg (instead of kwarg) we sidestep async_to_sync having a context?

yes, exactly that. the problem is that it internally spreads the kwargs into a call that already has a context parameter.

I stumbled across this bug in async_to_sync myself in a different part of this PR, and fixed that the same way.
It took me a while to figure that out btw :-(

sentry-io · 2024-09-24T15:29:02Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ KeyError: 'layout' app.tasks.notify.Notify View Issue
‼️ ConnectionError: ('Connection aborted.', RemoteDisconnected('Remote end closed connection without response')) app.tasks.compute_comparison.ComputeComparison View Issue
‼️ AttributeError: 'NoneType' object has no attribute 'repoid' app.tasks.notify.Notify View Issue
‼️ IndexError: list index out of range app.tasks.notify.Notify View Issue
‼️ KeyError: 'segments' app.tasks.notify.Notify View Issue

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

Swatinem requested review from giovanni-guidini and a team September 20, 2024 13:16

Swatinem self-assigned this Sep 20, 2024

Swatinem force-pushed the swatinem/sync-comparison branch 2 times, most recently from 1444f9c to 12ee48c Compare September 23, 2024 10:21

Swatinem changed the title ~~WIP: Make ComparisonProxy sync~~ Make ComparisonProxy sync Sep 23, 2024

Swatinem marked this pull request as ready for review September 23, 2024 10:22

Make ComparisonProxy sync

b07ef5c

As calling `async_to_sync` within an async context, this necessitated also turning all the various `notification` related functionality sync.

Swatinem force-pushed the swatinem/sync-comparison branch from 12ee48c to b07ef5c Compare September 23, 2024 10:27

giovanni-guidini reviewed Sep 23, 2024

View reviewed changes

giovanni-guidini approved these changes Sep 23, 2024

View reviewed changes

Swatinem temporarily deployed to staging September 23, 2024 11:55 — with GitHub Actions Inactive

Swatinem added this pull request to the merge queue Sep 24, 2024

Merged via the queue into main with commit 4d4e8fb Sep 24, 2024
28 of 40 checks passed

Swatinem deleted the swatinem/sync-comparison branch September 24, 2024 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `ComparisonProxy` sync #730

Make `ComparisonProxy` sync #730

Swatinem commented Sep 20, 2024 •

edited

Loading

codecov-staging bot commented Sep 20, 2024 •

edited by codecov-notifications bot

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading

codecov-qa bot commented Sep 20, 2024 •

edited

Loading

codecov-public-qa bot commented Sep 20, 2024 •

edited

Loading

giovanni-guidini left a comment

giovanni-guidini Sep 23, 2024

Swatinem Sep 23, 2024

sentry-io bot commented Sep 24, 2024 •

edited

Loading

Make ComparisonProxy sync #730

Make ComparisonProxy sync #730

Conversation

Swatinem commented Sep 20, 2024 • edited Loading

codecov-staging bot commented Sep 20, 2024 • edited by codecov-notifications bot Loading

Codecov Report

codecov bot commented Sep 20, 2024 • edited Loading

Codecov Report

codecov-qa bot commented Sep 20, 2024 • edited Loading

Codecov Report

codecov-public-qa bot commented Sep 20, 2024 • edited Loading

Codecov Report

giovanni-guidini left a comment

Choose a reason for hiding this comment

giovanni-guidini Sep 23, 2024

Choose a reason for hiding this comment

Swatinem Sep 23, 2024

Choose a reason for hiding this comment

sentry-io bot commented Sep 24, 2024 • edited Loading

Suspect Issues

Make `ComparisonProxy` sync #730

Make `ComparisonProxy` sync #730

Swatinem commented Sep 20, 2024 •

edited

Loading

codecov-staging bot commented Sep 20, 2024 •

edited by codecov-notifications bot

Loading

codecov bot commented Sep 20, 2024 •

edited

Loading

codecov-qa bot commented Sep 20, 2024 •

edited

Loading

codecov-public-qa bot commented Sep 20, 2024 •

edited

Loading

sentry-io bot commented Sep 24, 2024 •

edited

Loading