Add call to avoid status + checks at the same time #756
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #756 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 438 438
Lines 36386 36420 +34
==========================================
+ Hits 35669 35696 +27
- Misses 717 724 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #756 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 438 438
Lines 36386 36420 +34
==========================================
+ Hits 35669 35696 +27
- Misses 717 724 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #756 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 438 438
Lines 36386 36420 +34
==========================================
+ Hits 35669 35696 +27
- Misses 717 724 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #756 +/- ##
==========================================
- Coverage 98.02% 98.01% -0.02%
==========================================
Files 438 438
Lines 36386 36420 +34
==========================================
+ Hits 35669 35696 +27
- Misses 717 724 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -461,6 +462,7 @@ def test_notify_individual_notifier_timeout(self, mocker, sample_comparison): | |||
| def test_notify_individual_checks_notifier( | |||
| self, mocker, sample_comparison, mock_repo_provider, mock_configuration | |||
| ): | |||
| mock_repo_provider.get_commit_statuses.return_value = Status([]) | |||
There was a problem hiding this comment.
instead of defining this in every test thats using the function, how about adding this as a default to the mock_repo_provider
There was a problem hiding this comment.
Not a bad thought, but the mock_repo_provider is used in other files/tests so it may lead to unnecessary calls/undesired side effects. I also like it this way cause it makes it clearer to me what each test does, I find it better for readability :). I'll merge this tmrw since it's kinda late now
We have two ways of doing PR checks: via the commit statuses API or the check runs API. They both serve as a PR check, which tells the customer their status relevant to patch, change and project. We've had customers complain of seeing the same check with two different numbers/duplicates, and that's when you have a PR check using both APIs. You only want to use one of them and not the other, so this is a check to help favoring the commit status over a PR check if there's one existing.
This ^ happens when you try to do a check, you fail for some reason, fallback to doing a commit status, and eventually do a check for the same commit, ending up with duplicate checks. Ideally we'd like to favor having PR checks, but github offers no way to delete commit statuses (although they reset every time you push a new commit). Therefore, this seemed like the way to go. The main caveat is that, for every "legit" check, we're also calling an extra time to determine if there's a status for this commit already, which isn't ideal. But this will prevent sending a check and a status and only a status when there's already one posted.
More details here codecov/engineering-team#1967
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.