Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2953993003:
Add LayoutTest for PerformanceObserver inside Worker (Closed)
|
Created:
3 years, 6 months ago by npm Modified:
3 years, 5 months ago CC:
chromium-reviews, panicker+watch_chromium.org, shimazu+worker_chromium.org, kinuko+worker_chromium.org, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd LayoutTest for PerformanceObserver inside Worker
BUG=chromium:705820
Review-Url: https://codereview.chromium.org/2953993003
Cr-Commit-Position: refs/heads/master@{#484973}
Committed: https://chromium.googlesource.com/chromium/src/+/ca205ddf1a960d002da75bbcdd8243757a77e817
Patch Set 1 #Patch Set 2 : Typo #Patch Set 3 : Fix #Patch Set 4 : DontExpose #
Total comments: 13
Patch Set 5 : Feedback #
Total comments: 2
Patch Set 6 : Nit #
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
npm@chromium.org changed reviewers: + panicker@chromium.org, tdresser@chromium.org
ptal
LGTM with nits. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js:1: try { Rename file to worker-with-performance-observer.js. It's nice to have this name be consistent between the html and the js. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js:2: var obs = new PerformanceObserver(function () { return true; }); We can use fancy new JS here, so we might as well make this a bit more modern. var -> const (or just don't bother assigning it) "function() {return true;}" -> "() => true" So overall, that would just be: new PerformanceObserver(() => true); https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js:5: catch (ex) { Put on same line as closing brace above. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/worker-with-performance-observer.html (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/worker-with-performance-observer.html:10: var worker = new Worker("resources/worker-with-perfobs.js"); var -> const https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/worker-with-performance-observer.html:12: t.step( function() { No space after ( Feel free to use arrow functions here if you want.
falken@chromium.org changed reviewers: + falken@chromium.org
drive by comment. Is there a reason to not make this a WPT test (i.e., put it in external/wpt/)? https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:2879: crbug.com/705820 fast/workers/worker-with-performance-observer.html [ Failure ] Why add a test that's failing? Instead of a Failure line here, it should have a -expected.txt file instead.
So is external/wpt/workers a better location? https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:2879: crbug.com/705820 fast/workers/worker-with-performance-observer.html [ Failure ] On 2017/07/04 14:46:35, falken wrote: > Why add a test that's failing? Instead of a Failure line here, it should have a > -expected.txt file instead. Because this is actually not working. We will enable once the bug referenced is fixed.
Yes external/wpt should be preferred by default these days, unless the test is meant to test a Chromium-specific behavior. This looks like it's testing a normal web platform exposed feature. I also don't see where that is documented but somewhere around here: https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat... https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:2879: crbug.com/705820 fast/workers/worker-with-performance-observer.html [ Failure ] On 2017/07/05 21:53:53, npm wrote: > On 2017/07/04 14:46:35, falken wrote: > > Why add a test that's failing? Instead of a Failure line here, it should have > a > > -expected.txt file instead. > > Because this is actually not working. We will enable once the bug referenced is > fixed. I can't find where this is documented now, but it's recommended practice to use an -expected.txt file instead of [ Failure ] even if the expected test output is not ultimately what's "correct". It allows us to catch unexpected regressions in test output. I.e., we basically should never have [ Failure ] lines unless the test is flakily failing in different ways.
On 2017/07/05 23:30:08, falken wrote: > Yes external/wpt should be preferred by default these days, unless the test is > meant to test a Chromium-specific behavior. This looks like it's testing a > normal web platform exposed feature. (in general) please add tests here to performance-timeline (not to worker): https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/external/... As for adding to WPT: I think the test can be submitted as usual (should pass for supported browsers) and we need to update chromium's expectations file indicating that this will fail in chromium (until our impl is fixed) > > I also don't see where that is documented but somewhere around here: > https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_plat... > > https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/TestExpectations:2879: crbug.com/705820 > fast/workers/worker-with-performance-observer.html [ Failure ] > On 2017/07/05 21:53:53, npm wrote: > > On 2017/07/04 14:46:35, falken wrote: > > > Why add a test that's failing? Instead of a Failure line here, it should > have > > a > > > -expected.txt file instead. > > > > Because this is actually not working. We will enable once the bug referenced > is > > fixed. > > I can't find where this is documented now, but it's recommended practice to use > an -expected.txt file instead of [ Failure ] even if the expected test output is > not ultimately what's "correct". It allows us to catch unexpected regressions in > test output. I.e., we basically should never have [ Failure ] lines unless the > test is flakily failing in different ways.
A line in TestExpectations shouldn't be necessary. You can add a -expected.txt file containing the current test output. See for example this CL: https://codereview.chromium.org/2907443002 It adds fetch-request-xhr-sync.https.html along with fetch-request-xhr-sync.https-expected.txt with the (expected) failing test output.
On 2017/07/06 06:08:56, falken wrote: > A line in TestExpectations shouldn't be necessary. You can add a -expected.txt > file containing the current test output. See for example this CL: > https://codereview.chromium.org/2907443002 > > It adds > fetch-request-xhr-sync.https.html along with > fetch-request-xhr-sync.https-expected.txt with the (expected) failing test > output. Ah, maybe that's what panicker@ meant by "chromium's expectations file". Sorry if my comment was redundant.
ptal https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js:1: try { On 2017/07/04 14:31:46, tdresser wrote: > Rename file to worker-with-performance-observer.js. > > It's nice to have this name be consistent between the html and the js. Done. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js:2: var obs = new PerformanceObserver(function () { return true; }); On 2017/07/04 14:31:46, tdresser wrote: > We can use fancy new JS here, so we might as well make this a bit more modern. > > var -> const (or just don't bother assigning it) > > "function() {return true;}" -> "() => true" > > So overall, that would just be: > new PerformanceObserver(() => true); Done. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/resources/worker-with-perfobs.js:5: catch (ex) { On 2017/07/04 14:31:46, tdresser wrote: > Put on same line as closing brace above. Done. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/workers/worker-with-performance-observer.html (right): https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/worker-with-performance-observer.html:10: var worker = new Worker("resources/worker-with-perfobs.js"); On 2017/07/04 14:31:46, tdresser wrote: > var -> const Done. https://codereview.chromium.org/2953993003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/workers/worker-with-performance-observer.html:12: t.step( function() { On 2017/07/04 14:31:46, tdresser wrote: > No space after ( > > Feel free to use arrow functions here if you want. Done.
tdresser@google.com changed reviewers: + tdresser@google.com
LGTM % nit https://codereview.chromium.org/2953993003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/performance-timeline/worker-with-performance-observer.html (right): https://codereview.chromium.org/2953993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/performance-timeline/worker-with-performance-observer.html:12: t.step(() => assert_equals('SUCCESS', event.data)); assert_equals takes the expected result in the second parameter. See https://darobin.github.io/test-harness-tutorial/docs/using-testharness.html.
Description was changed from ========== Add LayoutTest for PerformanceObserver inside Worker BUG=chromium:705820 ========== to ========== Add LayoutTest for PerformanceObserver inside Worker BUG=chromium:705820 ==========
tdresser@chromium.org changed reviewers: - tdresser@google.com
https://codereview.chromium.org/2953993003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/performance-timeline/worker-with-performance-observer.html (right): https://codereview.chromium.org/2953993003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/performance-timeline/worker-with-performance-observer.html:12: t.step(() => assert_equals('SUCCESS', event.data)); On 2017/07/07 15:04:54, tdresser1 wrote: > assert_equals takes the expected result in the second parameter. > > See https://darobin.github.io/test-harness-tutorial/docs/using-testharness.html. Done.
The CQ bit was checked by npm@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by npm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, tdresser@google.com Link to the patchset: https://codereview.chromium.org/2953993003/#ps100001 (title: "Nit")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1499449164025120,
"parent_rev": "299f4d2472f0bb17bb10f4bb3e0caa7947f8858a", "commit_rev":
"ca205ddf1a960d002da75bbcdd8243757a77e817"}
Message was sent while issue was closed.
Description was changed from ========== Add LayoutTest for PerformanceObserver inside Worker BUG=chromium:705820 ========== to ========== Add LayoutTest for PerformanceObserver inside Worker BUG=chromium:705820 Review-Url: https://codereview.chromium.org/2953993003 Cr-Commit-Position: refs/heads/master@{#484973} Committed: https://chromium.googlesource.com/chromium/src/+/ca205ddf1a960d002da75bbcdd82... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ca205ddf1a960d002da75bbcdd82...
Message was sent while issue was closed.
On 2017/07/07 at 17:43:56, commit-bot wrote: > Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/ca205ddf1a960d002da75bbcdd82... Hi, a pull request was made for this upstream: https://github.com/w3c/web-platform-tests/pull/6496 If it gets stuck upstream I'll let you know here. In the future for any WPT changes the workflow on Gerrit is much better - a provisional PR gets made when you upload a change instead of when you land it. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
