Oh, I'm sorry - this pre-dates the rename (I forgot --follow when doing the git log).  Apparently we've always had a duplicate copy, and there's a presubmit rule that verifies that they match...

That still seems silly.  It should be easy to have the test runner remap to the single location (when running wpt tests), right?  Although if we're moving to a DEPS model for WPT then we'll presumably want to remap to the actual WPT repo instead (although we might not want to break being able to open many LayoutTests directly in an unmodified browser).


Thoughts?

If only testharness.js is remapped but idlharness.js is left where it is, it seems like finding the files will be harder for people unaware of the rewriting. Just keeping them in sync with a presumbit check seems reasonable to me, and it provides an out if the upstream testharness.js should change in a way that breaks our local tests but none of the upstream tests, in which case they might need to diverge for a short amount of time.

Now for input automation script injection we have added some code to testharnessreport.js, so the copy in LayoutTests/resources is not the same as the copy in LayoutTests/imported/wpt/resources/ -- but, it looks like this may be what we want; at the top of testharnessreport.js, it says:

 * This file is intended for vendors to implement
 * code needed to integrate testharness.js tests with their own test systems.

(See  bug 626740 .)

Currently LayoutTests/PRESUBMIT.py checks for equality of testharnessreport.js in LayoutTests/resources and LayoutTests/imported/wpt/resources, but it sounds like we want to have our own copy, independent of the wpt upstream repo.

So it seems like we may want to remove the presubmit check for equality of our copy of testharnessreport.js and the imported copy. The main danger seems to be that this is confusing and it's not always obvious which copy is used if we have two copies.

Also relevant to this discussion was the issue that led to us adding the check for equality of these files in 2014:  bug 362788 

Someone changed a string the upstream copy of the tests while the copy in LayoutTests/resources was not changed, which led to mismatch failures.

If we just stop making them identical and ignore new changes to the upstream copy, this might happen again... but then again, this is unlikely to happen often, and if our tests start failing due to a change in the upstream copy, then we could change our local copy.

testharnessreport.js should be overwritten on import since (as noted) that's intended to be vendor specific, and is how our test runner identifies pass/fail. So requiring testharnessreport.js to be the same seems fine.

Whether we want the the other files to remain in sync is separate. Here's how we ended up where we are:

(1) Originally, we only wanted to ensure that our multiple non-imported copies remained the same, i.e. LayoutTests/http/tests/resources/FOO and LayoutTests/resources/FOO ( bug 362788 )

(2) Then we added the requirement that the imported copies remain the same; this ensured that we didn't get too stale, although it added the burden that an import might break non-imported tests that depended on quirks, adding to the import burden.

(3) Then we fixed the test http server so that we eliminated the local duplicate copies - LayoutTests/http/tests/resources/FOO no longer exists, we fall back automatically to LayoutTests/resources/FOO. (This is a better fix for the original problem in #1.)

(4) So now we're only asserting that imported and non-imported copies remain the same.

Therefore, if the logic in #2 no longer holds (it's a burden), then I'm fine with no longer requiring the the imported and non-imported copies to be the same. Although some process to keep the non-imported copies fresh would be good.

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

commit dfec6156c3e17e698acff95a6558dcecaddef761
Author: qyearsley <qyearsley@chromium.org>
Date: Fri Jul 15 02:25:33 2016

Access environment variables through Host object instead of directly.

This makes it easier to test interactions with os.environ without
affecting or using actual environment variables in tests.

This CL doesn't change places where os.environ is used where there
is no Host object.

BUG= 625846 

Review-Url: https://codereview.chromium.org/2143123004
Cr-Commit-Position: refs/heads/master@{#405680}

[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/common/host.py
[delete] https://crrev.com/fb90bf24d217da0937226ac058bae3f2a0d8e799/third_party/WebKit/Tools/Scripts/webkitpy/common/system/environment.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/common/system/systemhost_mock.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/breakpad/dump_reader_win.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/android.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux_unittest.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/mock_drt.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/win_unittest.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py
[modify] https://crrev.com/dfec6156c3e17e698acff95a6558dcecaddef761/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py

Oops, last comment (about commit 405680) is not related to this bug; accidentally copied the wrong bug number.

In  issue 685854  we've decided not only to live with the duplicated files, but to allow them to go out of sync. WontFixing this, although it's probably not the last to be said on the issue.