[ews] merge-queue fails to find reviewer on GitHub PRs with lots of review activity #6355

JonWBedard · 2022-11-10T19:24:48Z

`42fd020`

[ews] merge-queue fails to find reviewer on GitHub PRs with lots of review activity
https://bugs.webkit.org/show_bug.cgi?id=243548
rdar://98129710

Reviewed by Aakash Jain.

* Tools/CISupport/ews-build/steps.py:
(GitHubMixin):
(GitHubMixin.get_reviewers): Handle pagination when retrieving reviews. Approval
and rejection should override older approval or rejection.
* Tools/CISupport/ews-build/steps_unittest.py:
(TestGitHubMixin):
(TestGitHubMixin.Response): Mock of request's response class.
(TestGitHubMixin.test_no_reviewers):
(TestGitHubMixin.test_single_review):
(TestGitHubMixin.test_multipe_reviews):
(TestGitHubMixin.test_retracted_review):
(TestGitHubMixin.test_pagination):
(TestGitHubMixin.test_reviewers_invalid_response):
(TestGitHubMixin.test_reviewers_error):

Canonical link: https://commits.webkit.org/256752@main

84ed6b2

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🛠 mac-AS-debug~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 api-mac~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk1~~
✅ 🧪 services	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
✅ 🛠 🧪 unsafe-merge	~~🛠 watch-sim~~	~~🧪 mac-wk2-stress~~

webkit-early-warning-system · 2022-11-10T19:25:21Z

EWS run on previous version of this PR (hash ee5993a)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
✅ 🧪 services	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

aj062 · 2022-11-10T19:57:38Z

Tools/CISupport/ews-build/steps.py

Let's make this 10 as a parameter as well, so that it's explicit and easy to modify later on if we need to change it.

aj062 · 2022-11-10T20:00:15Z

Tools/CISupport/ews-build/steps.py

Can change it to something like: for page in range(1, LIMIT_NUM_PAGES + 1)

aj062 · 2022-11-10T20:00:51Z

Tools/CISupport/ews-build/steps.py

Nit: can rename to something like: LIMIT_REVIEWS_PER_PAGE

aj062 · 2022-11-10T20:01:43Z

Tools/CISupport/ews-build/steps.py

why do we need this check?

We're asking for 100 on the page. If the page has less than 100, we can safely assume this is the last page. Most of the time, we won't even make a second request.

Seems like the comparison need to be inverted than. Should be if len(response_content) < self.LIMIT:

aj062 · 2022-11-10T20:04:42Z

Tools/CISupport/ews-build/steps.py

would be good to add self._addToLog here to help in debugging such failures.

aj062 · 2022-11-10T20:08:19Z

Tools/CISupport/ews-build/steps.py

what are we doing here?

aj062 · 2022-11-10T20:08:23Z

Tools/CISupport/ews-build/steps.py

aj062 · 2022-11-10T20:10:55Z

Tools/CISupport/ews-build/steps.py

what's the logic being implemented here? what's id_val? maybe rename id_val to something more readable.

We're trying to make sure that someone can override an r+ with an r- in the future. id_val comes from GitHub, it's the review "id" (according to GitHub's API). Larger numbers are newer. We could use timestamp, expect I didn't really want to decode the GitHub timestamp.

ok.
Also, id_val is a local variable in this line, maybe we can call it _id.
Also, since this is important logic, probably having a explicit for loop with a comment explaining what we are doing is better (instead of using list comprehension).

webkit-early-warning-system · 2022-11-10T22:29:45Z

EWS run on previous version of this PR (hash 2107c05)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	~~🧪 gtk-wk2~~
	⏳ 🧪 api-ios	✅ 🧪 api-mac	~~🧪 api-gtk~~
	✅ 🛠 tv	⏳ 🧪 mac-wk1
✅ 🧪 services	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

aj062 · 2022-11-10T22:32:53Z

Tools/CISupport/ews-build/steps.py

This still seems inverse. Maybe I am missing something.

Nope, my brain just wasn't working...updating

webkit-early-warning-system · 2022-11-10T23:15:52Z

EWS run on previous version of this PR (hash 6d27bff)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	❌ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
✅ 🧪 services	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

aj062 · 2022-11-15T16:06:26Z

Tools/CISupport/ews-build/steps.py

should it be continue or break. Seems like continue will result in an infinite loop.

aj062

would be good to add some unit-tests.
Can override/mock GitHubMixin.fetch_data_from_url_with_authentication_github() in unit-tests to avoid network request.

webkit-early-warning-system · 2022-11-15T16:57:54Z

EWS run on previous version of this PR (hash ff5809f)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 🧪 win
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	✅ 🧪 mac-wk1
✅ 🧪 services	✅ 🛠 tv-sim	✅ 🧪 mac-wk2
	✅ 🛠 watch	✅ 🧪 mac-AS-debug-wk2
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

webkit-early-warning-system · 2022-11-16T19:42:24Z

EWS run on previous version of this PR (hash 8b9cc8b)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	✅ 🛠 mac-debug	✅ 🛠 gtk	✅ 🛠 wincairo
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	✅ 🛠 mac-AS-debug	✅ 🧪 gtk-wk2
	~~🧪 api-ios~~	✅ 🧪 api-mac	✅ 🧪 api-gtk
	✅ 🛠 tv	~~🧪 mac-wk1~~
✅ 🧪 services	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
	✅ 🛠 watch-sim	✅ 🧪 mac-wk2-stress

aj062 · 2022-11-16T21:23:35Z

Tools/CISupport/ews-build/steps.py

Nit: this range would be one less than self.NUM_PAGE_LIMIT

aj062 · 2022-11-16T21:25:03Z

Tools/CISupport/ews-build/steps.py

do we need to construct this variable here, it's used only to construct the url variable. We can directly construct url in one go inside the for loop.

e.g.: url = f'{api_url}/pulls/{pr_number}/reviews?per_page={self.PER_PAGE_LIMIT}&page={page}

webkit-early-warning-system · 2022-11-16T22:34:14Z

EWS run on current version of this PR (hash 84ed6b2)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	~~🛠 mac~~	✅ 🛠 wpe	~~🛠 🧪 win~~
✅ 🧪 bindings	✅ 🛠 ios-sim	~~🛠 mac-debug~~	✅ 🛠 gtk	~~🛠 wincairo~~
✅ 🧪 webkitperl	~~🧪 ios-wk2~~	~~🛠 mac-AS-debug~~	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	~~🧪 api-mac~~	~~🧪 api-gtk~~
	✅ 🛠 tv	~~🧪 mac-wk1~~
✅ 🧪 services	✅ 🛠 tv-sim	~~🧪 mac-wk2~~
	✅ 🛠 watch	~~🧪 mac-AS-debug-wk2~~
✅ 🛠 🧪 unsafe-merge	~~🛠 watch-sim~~	~~🧪 mac-wk2-stress~~

…eview activity https://bugs.webkit.org/show_bug.cgi?id=243548 rdar://98129710 Reviewed by Aakash Jain. * Tools/CISupport/ews-build/steps.py: (GitHubMixin): (GitHubMixin.get_reviewers): Handle pagination when retrieving reviews. Approval and rejection should override older approval or rejection. * Tools/CISupport/ews-build/steps_unittest.py: (TestGitHubMixin): (TestGitHubMixin.Response): Mock of request's response class. (TestGitHubMixin.test_no_reviewers): (TestGitHubMixin.test_single_review): (TestGitHubMixin.test_multipe_reviews): (TestGitHubMixin.test_retracted_review): (TestGitHubMixin.test_pagination): (TestGitHubMixin.test_reviewers_invalid_response): (TestGitHubMixin.test_reviewers_error): Canonical link: https://commits.webkit.org/256752@main

webkit-commit-queue · 2022-11-16T22:42:53Z

Committed 256752@main (42fd020): https://commits.webkit.org/256752@main

Reviewed commits have been landed. Closing PR #6355 and removing active labels.

JonWBedard requested review from aj062 and ryanhaddad as code owners November 10, 2022 19:24

JonWBedard self-assigned this Nov 10, 2022

JonWBedard added Safari 13 Tools / Tests Tools in the Tools directory, build issues, test infrastructure, and bugs in test cases labels Nov 10, 2022

aj062 reviewed Nov 10, 2022

View reviewed changes

JonWBedard force-pushed the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch from ee5993a to 2107c05 Compare November 10, 2022 22:26

aj062 reviewed Nov 10, 2022

View reviewed changes

JonWBedard force-pushed the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch from 2107c05 to 6d27bff Compare November 10, 2022 23:15

JonWBedard requested a review from aj062 November 10, 2022 23:26

webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 11, 2022

aj062 reviewed Nov 15, 2022

View reviewed changes

Tools/CISupport/ews-build/steps.py Outdated

Copy link

Member

aj062 Nov 15, 2022

Choose a reason for hiding this comment

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

should it be continue or break. Seems like continue will result in an infinite loop.

aj062 reviewed Nov 15, 2022

View reviewed changes

JonWBedard removed the merging-blocked Applied to prevent a change from being merged label Nov 15, 2022

JonWBedard force-pushed the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch from 6d27bff to ff5809f Compare November 15, 2022 16:57

JonWBedard force-pushed the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch from ff5809f to 8b9cc8b Compare November 16, 2022 19:42

aj062 approved these changes Nov 16, 2022

View reviewed changes

JonWBedard force-pushed the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch from 8b9cc8b to 84ed6b2 Compare November 16, 2022 22:34

JonWBedard added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 16, 2022

webkit-early-warning-system force-pushed the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch from 84ed6b2 to 42fd020 Compare November 16, 2022 22:42

webkit-early-warning-system merged commit 42fd020 into WebKit:main Nov 16, 2022

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 16, 2022

JonWBedard deleted the eng/ews-merge-queue-fails-to-find-reviewer-on-GitHub-PRs-with-lots-of-review-activity branch November 17, 2022 23:59

[ews] merge-queue fails to find reviewer on GitHub PRs with lots of review activity #6355

[ews] merge-queue fails to find reviewer on GitHub PRs with lots of review activity #6355

Uh oh!

Conversation

JonWBedard commented Nov 10, 2022 • edited by webkit-early-warning-system Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aj062 left a comment

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-early-warning-system commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webkit-early-warning-system commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webkit-commit-queue commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JonWBedard commented Nov 10, 2022 •

edited by webkit-early-warning-system

Loading

webkit-early-warning-system commented Nov 10, 2022 •

edited

Loading

webkit-early-warning-system commented Nov 10, 2022 •

edited

Loading

webkit-early-warning-system commented Nov 10, 2022 •

edited

Loading

webkit-early-warning-system commented Nov 15, 2022 •

edited

Loading

webkit-early-warning-system commented Nov 16, 2022 •

edited

Loading

webkit-early-warning-system commented Nov 16, 2022 •

edited

Loading