New issue
Advanced search Search tips

Issue 685854 link

Starred by 3 users

Issue metadata

Status: WontFix
Owner: ----
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Update testharness.js, testdriver.js, idlharness.js and webidl2.js separately (not in auto-imports)

Project Member Reported by qyears...@chromium.org, Jan 26 2017

Issue description

There have been a multiple times when updating web-platform-tests updates testharness.js (and other resources which are copied to LayoutTests/resources) and this modifies the behavior of tests outside of external/wpt.

To make auto-imports more reliable or simpler, it might be better if the import process didn't update LayoutTests/resources/, and if those were updated separately (and less often), probably manually?

This would enable the auto-importer to be sure to only affect imported tests, and the updating of expectations and baselines could then ignore any flaky failures outside of the directory being updated.

I'm not entirely sure whether this would be be the best way though. Are there any big disadvantages to having LayoutTests/resources/testharness.js and LayoutTests/external/wpt/resources/testharness.js out of sync sometimes?
 
Note, the main problems I've seen so far with new versions of testharness.js is stricter checking leading to more console error messages, e.g.  http://crbug.com/662172  and https://codereview.chromium.org/2648173006/.

Bug 679742 suggests changing testharnessreport.js so that console messages are not output; if console messages were not output for all testharness.js tests, then that would also make it simpler if any future changes to testharness.js change console message behavior without changing actual test results.
Status: WontFix (was: Untriaged)
I think that what we actually want to do is probably what is proposed in bug 679742 -- somehow make it so that console messages don't affect whether testharness.js tests pass.
Labels: -Pri-3 Pri-2
Owner: qyears...@chromium.org
Status: Assigned (was: WontFix)
Looking at this again, I think that updating testharness.js (and idlharness.js) separately would be a good choice, because it could mean that we could say with certainty that the importer should never update expectations outside of wpt.

As it is now, the importer can sometimes change unrelated expectations outside of wpt, and this sometimes happens, e.g. in https://chromium.googlesource.com/chromium/src/+/60cc55805fd10b00fcd9472b17ae9517d7f942c9.
Cc: qyears...@chromium.org foolip@chromium.org
 Issue 722550  has been merged into this issue.

Comment 5 by jsb...@chromium.org, Jun 19 2017

Updating separately sgtm
Awesome -- uploaded a CL to do this: https://chromium-review.googlesource.com/c/539818/

The main question now of course is how we should update them -- I was thinking that for now, they could be updated manually, and later there could be a separate script that:
  1. Copies the files over
  2. Runs try jobs and downloads new baselines.

This script could also be run on the wpt-importer bot. Does that sound reasonable?

Comment 7 by jsb...@chromium.org, Jun 19 2017

Also sgtm!

Comment 8 by foolip@chromium.org, Jun 20 2017

Allowing the copies to drift apart to make import more straightforward SGTM, but who will be responsible for updating testharness.js so that they get back in sync regularly?
Summary: Update the common testharness.js resource separately (not in auto-imports) (was: Updating the common testharness.js resource separately (not in auto-imports)?)
I was thinking that in the immediate future, I would update it when I notice a change in testharness.js in an import :-/ It seems like testharness.js usually changes a few times per month, but being one or two months out of sync probably won't cause a lot of trouble. File history: https://chromium.googlesource.com/chromium/src/+log/master/third_party/WebKit/LayoutTests/resources/testharness.js

As mentioned in #6, we could have another recipe, or another step in the wpt-importer recipe, which copies the files from external/wpt/resources/ to resources/, then starts try jobs and rebaselines any files anywhere in LayoutTests *except* for wpt. It seems possible that this rebaselining could occasionally introduce incorrect baselines, but at least the CL would be smaller and focused on one change.

I think we should keep this bug open until there's a process for keeping testharness (and idlharness) in sync.
Summary: Update testharness.js and idlharness.js separately (not in auto-imports) (was: Update the common testharness.js resource separately (not in auto-imports))
Keeping this open until we have a process SGTM. Adding idlharness.js to the title as well. It's not necessarily worth automating this, we could also make it part of a rotation that resolves export problems, perhaps.
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5bbc2bf8150d69cc30463a9040de0e97f5caad28

commit 5bbc2bf8150d69cc30463a9040de0e97f5caad28
Author: Quinten Yearsley <qyearsley@google.com>
Date: Wed Jun 21 20:10:11 2017

Don't copy testharness.js and idlharness.js on wpt import.

Background: In general we want to keep
LayoutTests/resources/testharness.js in sync with
LayoutTests/external/wpt/resources/testharness.js because we want
testharness tests inside of and outside of wpt to behave similarly.

However, the practice of keeping these files always in sync means that
it's possible for results of testharness tests outside of wpt to change
upon import. Currently, the wpt importer tries to download new baselines
for any tests that fail on import. This works most of the time but
sometimes means we download baselines that we didn't want to download.

Rationale for this change: To simplify the import process, I think it's
easier to make it so that imports can only affect tests in external/wpt.

I think there should be a separate process to sync the testharness.js
and idlharness.js files; this could be manual and occasional, or it
could be automatic. It could be a separate "mode" of the wpt-importer.

In this change:
 1. Simplify importer so it doesn't copy files over
 2. Simplify presubmit so it doesn't check that the files are in sync
 3. Change wpt-update-expectations so that it ignores all failures outside of external/wpt.


Bug:  685854 
Change-Id: I8f39790313cd1429f9016418a76a0eecc3183016
Reviewed-on: https://chromium-review.googlesource.com/539818
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481285}
[modify] https://crrev.com/5bbc2bf8150d69cc30463a9040de0e97f5caad28/third_party/WebKit/LayoutTests/PRESUBMIT.py
[modify] https://crrev.com/5bbc2bf8150d69cc30463a9040de0e97f5caad28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/5bbc2bf8150d69cc30463a9040de0e97f5caad28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_expectations_updater.py
[modify] https://crrev.com/5bbc2bf8150d69cc30463a9040de0e97f5caad28/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_expectations_updater_unittest.py

Components: Blink>Infra>Ecosystem
Components: -Blink>Infra>Predictability
Project Member

Comment 14 by bugdroid1@chromium.org, Jul 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/724dbb5f562ebb4c62c6c52015f7a916b73c1923

commit 724dbb5f562ebb4c62c6c52015f7a916b73c1923
Author: Quinten Yearsley <qyearsley@google.com>
Date: Wed Jul 19 01:21:06 2017

Reorganize the order of methods in wpt importer

This CL reorganizes the (confusingly-named) update and do_auto_update
methods in TestImporter.

The main purpose of this is to prepare to re-use the code related to
triggering try jobs, updating expectations and triggering in order to
automate the updating of LayoutTests/resources/testharness.js.

The other purpose is to try to reorganize the code to improve readability.

Bug:  685854 
Change-Id: I6aac4a0875763fbcc7f5fbdd16cc038798243f92
Reviewed-on: https://chromium-review.googlesource.com/576368
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487705}
[modify] https://crrev.com/724dbb5f562ebb4c62c6c52015f7a916b73c1923/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/724dbb5f562ebb4c62c6c52015f7a916b73c1923/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

Owner: ----
Status: Available (was: Assigned)
Cc: robertma@chromium.org
I believe the next step here is to make a script which does this:

 1. Copy LayoutTests/external/wpt/resources/testharness.js to LayoutTests/resources/.
 2. Upload a CL.
 3. Trigger and wait for try jobs.
 4. Downloads new baselines. (There should generally be no new test expectation lines.)
 5. Triggers CQ.

There are other conceivable ways to do it, like making this an option of wpt-import, but that option seems more complicated somehow.

This script would use the functionality of FileSystem (copying file) and GitCL (triggering try job, uploading CL, triggering CQ); the easiest way to rebaseline now is by invoking webkit-patch rebaseline-cl in a separate process just like how it's done in wpt_expectations_updater.py.
Project Member

Comment 18 by bugdroid1@chromium.org, Oct 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5c6da120dc404e10f642e4540426999875b68017

commit 5c6da120dc404e10f642e4540426999875b68017
Author: Quinten Yearsley <qyearsley@chromium.org>
Date: Tue Oct 03 08:53:22 2017

Manually update Chromium's copy of testharness.js and idlharness.js.

Bug:  685854 
Change-Id: I3eeaad133bf9a1830eb5ef8558e3f29966214872
Reviewed-on: https://chromium-review.googlesource.com/695662
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505988}
[modify] https://crrev.com/5c6da120dc404e10f642e4540426999875b68017/third_party/WebKit/LayoutTests/compositing/overflow/composited-scroll-overlap-test-expected.txt
[modify] https://crrev.com/5c6da120dc404e10f642e4540426999875b68017/third_party/WebKit/LayoutTests/fast/dom/custom/svg-use-shadow-tree-expected.txt
[modify] https://crrev.com/5c6da120dc404e10f642e4540426999875b68017/third_party/WebKit/LayoutTests/http/tests/w3c/webperf/approved/navigation-timing/html/idlharness-expected.txt
[modify] https://crrev.com/5c6da120dc404e10f642e4540426999875b68017/third_party/WebKit/LayoutTests/http/tests/webaudio/autoplay-crossorigin-expected.txt
[modify] https://crrev.com/5c6da120dc404e10f642e4540426999875b68017/third_party/WebKit/LayoutTests/resources/idlharness.js
[modify] https://crrev.com/5c6da120dc404e10f642e4540426999875b68017/third_party/WebKit/LayoutTests/resources/testharness.js

Cc: nzolghadr@chromium.org
 Issue 781322  has been merged into this issue.
Summary: Update testharness.js, testdriver.js and idlharness.js separately (not in auto-imports) (was: Update testharness.js and idlharness.js separately (not in auto-imports))
Adding testdriver.js to the title per  issue 781322 .
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c9a7b51c37e742e24f36b5d47680bce3361fb96

commit 4c9a7b51c37e742e24f36b5d47680bce3361fb96
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Fri Nov 24 15:17:17 2017

Sync LayoutTests/resources/ with LayoutTests/external/wpt/resources/

The change to assert_array_equals in testharness.js affected two tests:
LayoutTests/bluetooth/descriptor/writeValue/write-updates-value.html
LayoutTests/fast/canvas-api/canvas-lineDash-invalid.html

The bluetooth test was previously trying to compare two ArrayBuffer
instances, which would before silently fail to do something useful, as
actual/expected.length were undefined and `0 < undefined` is false.

The canvas test was just using the wrong assert method.

The change to assert_array_equals came from here: (Thanks @Ms2ger!)
https://github.com/w3c/web-platform-tests/pull/7560

Bug:  685854  (not a fix, just related)
Change-Id: I12214518a6445fa6e52fa06a0a954e8acc8f289c
Reviewed-on: https://chromium-review.googlesource.com/786016
Reviewed-by: Robert Ma <robertma@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519116}
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/bluetooth/descriptor/writeValue/write-updates-value.html
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/fast/canvas-api/canvas-lineDash-invalid.html
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/fast/dom/custom/svg-use-shadow-tree-expected.txt
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/http/tests/security/subresourceIntegrity/subresource-integrity-style-allowed-expected.txt
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/http/tests/webaudio/autoplay-crossorigin-expected.txt
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/resources/WebIDLParser.js
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/resources/idlharness.js
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/resources/testdriver.js
[modify] https://crrev.com/4c9a7b51c37e742e24f36b5d47680bce3361fb96/third_party/WebKit/LayoutTests/resources/testharness.js

Summary: Update testharness.js, testdriver.js, idlharness.js and webidl2.js separately (not in auto-imports) (was: Update testharness.js, testdriver.js and idlharness.js separately (not in auto-imports))
Changed the summary to include webidl2.js
Project Member

Comment 23 by bugdroid1@chromium.org, Nov 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0578baafc77e7360e0027975b9436a4d9385fa03

commit 0578baafc77e7360e0027975b9436a4d9385fa03
Author: Robert Ma <robertma@chromium.org>
Date: Tue Nov 28 21:08:27 2017

Create a copy of webidl2.js for non-WPT layout tests

With this change, non-WPT layout tests are completely independent of
WPT files and hence won't be affected by imports. The change also makes
the behaviour more consistent as we already have copies of
testharness.js, testdriver.js and idlharness.js, with webidl2.js being
the only left. An apache alias rule is changed accordingly.

See  crbug.com/685854  for maintaining these Chromium copies.

Besides, WebIDLParser.js (which is an old version of webidl2.js) is
removed from resources/ to avoid confusion. The only three files using
it are modified to use webidl2.js instead.

Bug:  787829 ,  685854 
Change-Id: I2d9966646cc7db557d87ec403cd0c833ee262279
Reviewed-on: https://chromium-review.googlesource.com/785992
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519834}
[modify] https://crrev.com/0578baafc77e7360e0027975b9436a4d9385fa03/third_party/WebKit/LayoutTests/installedapp/idl.html
[modify] https://crrev.com/0578baafc77e7360e0027975b9436a4d9385fa03/third_party/WebKit/LayoutTests/resources/README.txt
[rename] https://crrev.com/0578baafc77e7360e0027975b9436a4d9385fa03/third_party/WebKit/LayoutTests/resources/webidl2.js
[modify] https://crrev.com/0578baafc77e7360e0027975b9436a4d9385fa03/third_party/WebKit/LayoutTests/webaudio/audio-worklet/audio-worklet-node-idl.html
[modify] https://crrev.com/0578baafc77e7360e0027975b9436a4d9385fa03/third_party/WebKit/LayoutTests/webauth/idl.html
[modify] https://crrev.com/0578baafc77e7360e0027975b9436a4d9385fa03/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py

Comment 24 by ajuma@chromium.org, Feb 13 2018

Is the remaining work here just writing the script described in Comment 17?
And run the script (or manually update the resources) periodically.
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b98895b9d2871d0365f4c5509d8371542df5897b

commit b98895b9d2871d0365f4c5509d8371542df5897b
Author: Robert Ma <robertma@chromium.org>
Date: Thu Mar 01 22:39:41 2018

Sync LayoutTests/resources/ with LayoutTests/external/wpt/resources/

Roll in the most recent version of:
testharness.js, testdriver.js, idlharness.js and webidl2.js

Some baselines are adjusted accordingly (mostly due to
https://github.com/w3c/web-platform-tests/pull/9051).

Bug:  685854 
Change-Id: Ic74431ad0bbbc13e0c9ab2fa8bb4701f69806d32
Reviewed-on: https://chromium-review.googlesource.com/941915
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540318}
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/animations/stability/keyframe-iteration-exception-crash-expected.txt
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/fast/dom/custom/svg-use-shadow-tree-expected.txt
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/fast/js/put-forwards-expected.txt
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/http/tests/webaudio/autoplay-crossorigin-expected.txt
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/resources/idlharness.js
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/resources/testharness.js
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/resources/webidl2.js
[modify] https://crrev.com/b98895b9d2871d0365f4c5509d8371542df5897b/third_party/WebKit/LayoutTests/webaudio/audio-worklet/audio-worklet-node-idl-expected.txt

Status: WontFix (was: Available)
I think the state we are in now is the right one so closing this. The files inside LayoutTests/external/wpt should be auto-updated and are, and the copies outside are not.
Project Member

Comment 28 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e832355cf4019319cb55bebd11fa6533ec5a5143

commit e832355cf4019319cb55bebd11fa6533ec5a5143
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Fri May 18 16:48:20 2018

Sync LayoutTests/resources/ with LayoutTests/external/wpt/resources/

Roll in the most recent version of:
testharness.js, testdriver.js, idlharness.js and webidl2.js

Trailing whitespace in webidl2.js is removed to pass presubmit.
This means it is not 100% in sync with upstream.

Bug:  685854 
Change-Id: Iac8177bd995eb13d545796ef91e1075692db70f0
Reviewed-on: https://chromium-review.googlesource.com/1059152
Reviewed-by: Robert Ma <robertma@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559924}
[modify] https://crrev.com/e832355cf4019319cb55bebd11fa6533ec5a5143/third_party/WebKit/LayoutTests/fast/dom/custom/svg-use-shadow-tree-expected.txt
[modify] https://crrev.com/e832355cf4019319cb55bebd11fa6533ec5a5143/third_party/WebKit/LayoutTests/resources/idlharness.js
[modify] https://crrev.com/e832355cf4019319cb55bebd11fa6533ec5a5143/third_party/WebKit/LayoutTests/resources/testdriver.js
[modify] https://crrev.com/e832355cf4019319cb55bebd11fa6533ec5a5143/third_party/WebKit/LayoutTests/resources/testharness.js
[modify] https://crrev.com/e832355cf4019319cb55bebd11fa6533ec5a5143/third_party/WebKit/LayoutTests/resources/webidl2.js
[delete] https://crrev.com/1cfbf9f53aa4eba9ed7fbbbc58e149667a463778/third_party/WebKit/LayoutTests/webaudio/audio-worklet/audio-worklet-node-idl-expected.txt
[modify] https://crrev.com/e832355cf4019319cb55bebd11fa6533ec5a5143/third_party/WebKit/LayoutTests/webaudio/audio-worklet/audio-worklet-node-idl.html

Comment 29 by tkent@chromium.org, May 20 2018

Cc: -tkent@chromium.org

Sign in to add a comment