New issue
Advanced search Search tips

Issue 683485 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature



Sign in to add a comment

Keep manifest generator from adding certain files which may never be tests.

Project Member Reported by jeffcarp@chromium.org, Jan 21 2017

Issue description

If MANIFEST.json already exists in LayoutTests/external/wpt, running the following command will add an entry for it:

third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/manifest --work --tests-root=third_party/WebKit/LayoutTests/external/wpt
 
Status: Available (was: Untriaged)
Does this mean that MANIFEST.json will add/change an entry for MANIFEST.json itself?

Note, MANIFEST.json also adds other entries which we don't require right now -- for example, it adds entries for all baselines.

The way that MANIFEST.json is used by the test runner now is: When deciding whether a file is a test, it checks the manifest. If there's no entry, then by default it's not a test. So if there were a way to make it so that MANIFEST.json and baseline files are not included in the list, that would make it so that there are fewer unnecessary changes to the manifest file.

Comment 2 by pwnall@chromium.org, Jan 24 2017

#1: Yes, MANIFEST.json contains an entry for itself.

Would blacklisting .txt and .json be sufficient?

Alternatively, I think it's possible to create a whitelist of suffixes used by WPT tests. It might be:
.html
.svg
.xhtml
.any.js
.worker.js

Given that adding support for a new test type is non-trivial and requires changes to the test runner, I would guess that having to maintain a whitelist isn't so bad.
Labels: -Type-Bug Type-Feature
Summary: Keep manifest generator from adding certain files which may never be tests. (was: If MANIFEST.json already exists, running wpt/manifest adds an entry for it)
I think that adding this blacklisting functionality would involve changing the manifest script upstream; it might involve adding a command-line argument to the manifest script and then using it when iterating through files in the tree, which is done in: https://github.com/w3c/wpt-tools/blob/master/manifest/vcs.py.

After making that change to the manifest script upstream, we would want to update the version of the manifest script checked into Chromium, and update the way that it's called.

Comment 4 by pwnall@chromium.org, Jan 26 2017

Cc: -cos...@gmail.com pwnall@chromium.org
If you don't consider this a bug for the automated WPT upstreaming process, the documentation should explain why MANIFEST.json diffs look odd (i.e. they might contain MANIFEST.json and expectations).

Comment 5 by geoff...@gmail.com, Feb 3 2017

It shouldn't be adding itself, ever. That seems like something has gone wrong? In principle it shouldn't be including anything in .gitignore; are we just not importing .gitignore?

Blacklisting expectation fails is something we should probably do; that probably should be done by adding some blacklist argument upstream and calling it with that by default?
geoffers@: since the script is vendored into Chromium and is being run in the context of Chromium, it uses Chromium's .gitignore, not that of WPT.

We could investigate if it would be feasible to add some entries from WPT's .gitignore to Chromium's?

Comment 7 by geoff...@gmail.com, Feb 6 2017

.gitignore is supported in subdirectories: it doesn't need to be at the top of the repo; there's nothing stopping us from having the upstream .gitignore in LayoutTests/external/wpt. It's also additive, so everything in Chromium's .gitignore will still apply.
Filed issue in wpt-tools: https://github.com/w3c/wpt-tools/issues/162. Note, this probably wouldn't need to be done if MANIFEST.json is in .gitignore.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 3 2017

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

MANIFEST.json should no longer be added to MANIFEST.json now that it exists as an entry in LayoutTests/external/wpt/.gitignore.
Components: Blink>Infra>Ecosystem
Components: -Blink>Infra>Predictability
Status: Fixed (was: Available)
I believe this is fixed now with WPT_BASE_MANIFEST.json being generated on import.

Sign in to add a comment