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

Replace ===/!== in assert_true/false #25086

Merged
merged 7 commits into from
Sep 22, 2020

Conversation

gsnedders
Copy link
Member

This is relatively common and this does some of the simple cases that I can easily replace with regex.

@reillyeon
Copy link
Contributor

This change is too large. I have no idea what part of it @wpt-pr-bot wants me to look at.

@stephenmcgruer
Copy link
Contributor

This change is too large. I have no idea what part of it @wpt-pr-bot wants me to look at.

wpt-pr-bot uses 'reviewers' as a cc list (because GitHub has basically no support for cc... there is a very stalled RFC to change how wpt-pr-bot works here), so you are not necessarily expected to review this. GitHub uses 'assignees' for what Chromium calls 'reviewers'.

That said, it looks like you are listed as an owner for 5 directories:

$ grep -r reillyeon `find . -name META.yml`
./bluetooth/META.yml:  - reillyeon
./webusb/META.yml:  - reillyeon
./mediacapture-image/META.yml:  - reillyeon
./orientation-event/META.yml:  - reillyeon
./screen-wake-lock/META.yml:  - reillyeon

So for this PR, mediacapture-image/getPhotoCapabilities.html seems to be the only file that you might want to look at.

(I do not disagree that this is a large and wide PR, and wpt-pr-bot does a bad job for large, wide PRs.)


Overall, for this PR I am most interested in whether @jan-ivar is happy with the new version (and thanks to @gsnedders for taking on this refactor!)

@wpt-pr-bot wpt-pr-bot requested review from zqzhang and removed request for yuki3 September 18, 2020 18:44
@stephenmcgruer stephenmcgruer requested review from jan-ivar and removed request for tkent-google September 21, 2020 14:18
@stephenmcgruer
Copy link
Contributor

LGTM again. I understand there is some risk of changed semantics here, but with the removal of most of the controversial cases I think this is a net positive change for the test suite.

I took a look at the Chrome, Firefox, and Safari results. Chrome and Firefox had no different results before/after at all. Safari had a few:

css/css-fonts/variations/at-font-face-font-matching.html - results change seems suspicious at first glance, but looking at wpt.fyi these tests are pretty flaky on Safari and I accept that this is likely flake rather than a behavioral change.

resource-timing/resource_dedicated_worker.html - flaky on Safari

user-timing/measure_associated_with_navigation_timing.html - flaky on Safari

Please give 24 hours for someone to raise a new objection, otherwise LGTM to merge.

@gsnedders gsnedders merged commit 3cabe51 into web-platform-tests:master Sep 22, 2020
@gsnedders gsnedders deleted the assert_equals branch September 22, 2020 14:56
watilde added a commit to watilde/node that referenced this pull request Sep 24, 2020
watilde added a commit to nodejs/node that referenced this pull request Sep 27, 2020
Refs: web-platform-tests/wpt#25086

PR-URL: #35329
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: web-platform-tests/wpt#25086

PR-URL: nodejs#35329
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet