Today, nzolghadr@ brought up the fact that currently we don't support adding new tests to the Chromium copy of wpt and running the tests -- the test won't be found until the manifest script is re-run.

In the meeting today, we discussed how perhaps in Port.wpt_manifest()[1], the test runner could check if there are any local changes that add new tests, and re-run the script[2] if that's the case.

[1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py#742
[2] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Tools/Scripts/webkitpy/w3c/deps_updater.py#165

Note however that in order for this to work for everyone, all of the dependencies of the manifest script have to be available ( bug 681736 ).

FWIW, I imagine you'll need something like this in a presubmit check too, so you can make sure we didn't forget to update the manifest before uploading the CL.

I think that sounds reasonable.

Although, I think if there is a new test added and someone commits without updating the manifest file, and someone else syncs and runs the test, then their manifest file will be updated locally too when they run the test, so the set of tests run should still be correct as long as the manifest is updated before running.

CL for this issue: https://codereview.chromium.org/2644783003

#7: So... after running layout tests, your git checkout would become dirty?

If going that route, I think it'd be cleaner to remove MANIFEST.json from the repository and .gitignore it.

That would be cleaner. Since it's in there already, I'm guessing there's a good reason MANIFEST.json is checked into the repo (but I don't have context on why it was added originally).

Yep, after running layout tests, there would be uncommitted changes to MANIFEST.json in your git checkout, which might be mysterious or unwanted.

The advantage of checking in MANIFEST.json, and not regenerating it every time, is to save time -- it takes more than 10 seconds to fully regenerate MANIFEST.json (11.5 seconds on my workstation).

But, to make a minor update to MANIFEST.json takes about half a second. So keeping a mostly-updated copy checked in and updating it on test run doesn't add a large amount of overhead on test run.


To avoid the issue of leaving behind uncommitted changes to MANIFEST.json after running the tests, I still think it may be a good idea to just have the importer update the file, and make the test runner temporarily update the file but then restore it. jeffcarp@, pwnall@, does this sound OK?

#11: To clarify, I think the dirty git tree issue is only relevant if we allow the repository to get in a state where MANIFEST.json doesn't match the tests that are checked in.

I still think we should add a presubmit check, so the repository never gets into such a situation. I think it's dangerous to have a checked in manifest that doesn't match the state of the world, because it's tempting to write tools that consume it, and such tools can potentially lead to bad decisions. 

Also, if we go the presubmit check route, having MANIFEST.json updated after running tests becomes a desirable side-effect.

Quinten and I chatted this morning and I think we came to the same conclusion. So just to summarize: we should go ahead and modify MANFIEST.json during test runs and also have a presubmit hook that does the same.

I can create a separate issue for the presubmit, if that sounds good.

That sounds great! Thank you very much!

Created a bug for presubmit: https://bugs.chromium.org/p/chromium/issues/detail?id=684230

After having gotten more experience with this process, I'm starting to think that MANIFEST.json should be .gitignore'd.

Earlier, I thought that there'd be productivity benefits gained by using a cached manifest. Then I started having to deal with conflicts:

https://codereview.chromium.org/2653923007/#ps1
https://codereview.chromium.org/2664623002/#ps20001
https://codereview.chromium.org/2659513002/#ps80001

When I rebase, the conflicts are spread out enough that I end up removing and re-generating MANIFEST.json. To avoid future conflicts, I use "git add -i" and go through the MANIFEST.json changes hunk-by-hunk.

At this point, I've re-generated the manifest, thus lost the cache benefits, and I've paid the penalty of an interactive git add, and of an extra CQ run. 

Based on this experience, I now think that having MANIFEST.json in .gitignore would be a superior approach. If the performance hit of re-generating MANIFEST.json when running layout tests is really undesirable, how about having "gclient sync" download prebuilt versions, like we do for clang?

Yeah, that does sound annoying and it really does seem easier for developers to not have to deal with MANIFEST.json. The contents of the tests themselves on the filesystem are the real "source of truth" about the tests; the manifest file is just an optimization to make it easier to get facts about the tests, and the manifest script provides the logic for parsing the tests to get metadata.

Re-generating from scratch would take many seconds (maybe around 10 seconds?)
Having gclient sync download prebuilt versions sounds like it could work...

The gclient option sounds interesting. So that would mean you'd never have to regenerate MANIFEST.json manually unless you're adding a new test? (and even if you regenerated manually you'd never commit those changes)

How would we keep the gclient dependency up to sync? I'd imagine we'd have a job running that regenerates MANIFEST.json after every commit and uploads it to wherever gclient sync gets its dependencies from?

I think it is quite common for people to switch between commits that aren't far apart (no rolls) without running gclient sync, I certainly do.

Re #21: I think that this should be OK, since as long as MANIFEST.json is *mostly* generated, then updating it is fast (<1s), even if it hasn't been updated for the past month.

How I imagine the gclient option would work would be something like this:

 1. There's an entry in the hooks section of the DEPS file (https://cs.chromium.org/chromium/src/DEPS?l=553) which downloads from a particular GS location and puts it in wpt/MANIFEST.json, which is not tracked by git.

 2. Every time an w3c-test-autoroller job is run, it should also generate the manifest and upload to that location.

 3. Every time the tests are run, the manifest script is run so that the manifest is updated -- but since it's .gitignore'd, this is not tracked by git.

Some questions are:
 - What GS bucket would we use? Can we use an existing one?
 - What are the disadvantages of this approach?

Since the manifest tool got updated, it shouldn't be anywhere near so expensive to update from a slightly-wrong MANIFEST.json compared with doing it from scratch (and from scratch is going to get even more expensive when csswg-test gets merged due to the number of tests involved).

As such, using gclient to update it to something updated from time-to-time (whether that's daily or weekly I don't think matters) and then regenerating it when running tests is probably a decent compromise.

With the recent changes to the manifest format, all files are included together file a hash of their content, and this has made changes to MANIFEST.json a lot more noisy. MANIFEST.json also includes -expected.txt files and MANIFEST.json itself, so regenerating over and over it never actually stabilizes.

The hash also means that even after fixing a typo in a test, one would have to regenerate the manifest. Even if that were enforced by a presubmit check and in CQ, it would be a pretty serious nuisance. Raising the P1 since one Blink developer emailed me to say the manifest stuff is more hassle than it's worth.

Getting MANIFEST.json out of git entirely and seeding it with gclient sounds like a decent plan.

On my laptop, deleting MANIFEST.json and regenerating takes 22 seconds. That's too long, but I think I'd rather wait 22 seconds the first time running tests in external/wpt than update the manifest.

Concretely: First remove MANIFEST.json from git and generate it somewhere around the place that wptserve is started. Then work on getting it seeded via gclient as an optimization.

I think this would amount to a slight simplification of https://codereview.chromium.org/2644783003.

BTW, if it's possible to put a generated-and-not-checked-in MANIFEST.json in the out/ directory instead of in the tree, that'd be better I think.

> Concretely: First remove MANIFEST.json from git and generate it somewhere around the place that wptserve is started.

We ran into concurrency problems when generating MANIFEST.json after run-webkit-tests broke out into multiple processes. Right now the CL I have generates it at the very beginning while run-webkit-tests is still 1 process (only if any WPT tests are detected).

Also, is it alright that we're adding 22 seconds of time to all run-webkit-tests invocations that include WPT (including the CQ, I presume) until we make use of gclient sync?

So to summarize: 
- Add MANIFEST.json to .gitignore
- When run-webkit-tests is run, if there are any WPT tests detected, then (re-)generate MANIFEST.json and put it in out/. 

I can update my CL if that sounds good.

Adding 22 seconds is probably not going to be very welcome if we're unsure how long it'll take to fix again. If we can keep MANIFEST.json checked in somewhere and use that as the basis for generating a separate MANIFEST.json, that sounds like an expedient way forward. If we call it MANIFEST.json.blablabla and move it outside of external/wpt, then perhaps instead of seeding it with gclient, we could have the import script update it periodically?

That sounds like a great approach, I can update my CL accordingly.

Yep, that does sound easier :-) There's an occasional problem associated with the fact that Rietveld doesn't handle large files really well, and so in the last couple (large) imports, MANIFEST.json wasn't updated by auto-import. But again, this is another thing that could perhaps resolve itself after moving to Gerrit.

Question: Should the boilerplate MANIFEST.json script be 1. copied into LayoutTests/external/wpt every time the tests are run, or 2. once someone generates it, should we just update their local copy?

Downside to approach 1: a higher average cost of running the tests
Downside to approach 2: if someone generated MANIFEST.json a long time ago, they'd incur a large one-time cost to update it

Also I am OOO this week but I uploaded an in-progress version of what we talked about here: https://codereview.chromium.org/2644783003

Approach 2 as in your CL seems fine to me. One risk is that the manifest format changes drastically, and the scripts to generate change enough, an existing very old MANIFEST.json could cause things to break. It would be good to check what happens if the existing MANIFEST.json is full of garbage or is a JSON file of some different shape entirely.

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

commit eb1ff6b20500834aa8283148b8a7b78f2938ca27
Author: Jeff Carpenter <jeffcarp@chromium.org>
Date: Fri Mar 03 00:48:43 2017

Add MANIFEST.json to .gitignore, regenerate it from template in test runner

This solves 2 problems:

1. Regenerating MANIFEST.json is failing on ToT ( bug 697207 )
- Now that MANIFEST.json is in .gitignore, regeneration works
2. Regenerating adds MANIFEST.json to MANIFEST.json ( bug 683485 )
- MANIFEST.json no longer shows up in MANIFEST.json

BUG= 697207 , 666957 , 683485 

Change-Id: I09987b66027e1f94055888c8ead58e7e40896d62
Reviewed-on: https://chromium-review.googlesource.com/447959
Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#454454}
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/docs/testing/web_platform_tests.md
[rename] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/LayoutTests/external/WPT_BASE_MANIFEST.json
[add] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/LayoutTests/external/wpt/.gitignore
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/Tools/Scripts/webkitpy/common/host_mock.py
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py
[modify] https://crrev.com/eb1ff6b20500834aa8283148b8a7b78f2938ca27/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_manifest.py

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

commit e09b87d2ba3bc1a8c616d836fe10b8cb604237cc
Author: Quinten Yearsley <qyearsley@google.com>
Date: Fri Mar 03 18:39:13 2017

Update WPT_BASE_MANIFEST.json on import.

BUG= 666957 

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

MANIFEST.json is now updated behind the scenes during layout test runs! You shouldn't have to manually fiddle with that file again.