New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 698294 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Port._wpt_manifest incorrectly assumes that MANIFEST.json always exists.

Project Member Reported by qyears...@chromium.org, Mar 3 2017

Issue description

After https://chromium-review.googlesource.com/c/447959/, MANIFEST.json may not necessarily exist, but Port._wpt_manifest still always assumes that it does, which causes an error in some cases, e.g. when I run `webkit-patch rebaseline-cl`.

Note, in https://chromium-review.googlesource.com/c/447959/ we originally decided not to generate the manifest in Port._wpt_manifest because of the fact that that that code *could* be called in multiple subprocesses in parallel when running the tests. Instead we decided to make it so that Port._wpt_manifest assumes that the manifest exists, and layout_tests/controllers/manager.py is responsible for making sure that it gets generated if necessary.

One possible solution:
 - Move ensure_manifest method to WPTManifest.
 - Call ensure_manifest in more places?
 
> Move ensure_manifest method to WPTManifest

I think we should do this but I don't think it would solve the problem - if Port._wpt_manifest calls it from within one of the subprocesses, it would still run into the same resource contention problems we saw.

Do we have any way of acquiring a lock between all the subprocesses? I'll look into that.

As a quick fix could `webkit-patch rebaseline-cl` also call ensure_manifest?
Yeah, as a quick fix doing that sounds good; I'm still not 100% sure that that's correct in all cases, since I think there might be other places outside of running the tests or running rebaseline-cl where tests are listed, which would require the manifest to exist.

Will update https://codereview.chromium.org/2722243005.
Project Member

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

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

commit db5746f9a7e4afd9a91fd8fafc275f74035e6614
Author: qyearsley <qyearsley@chromium.org>
Date: Fri Mar 03 22:53:44 2017

Ensure manifest exists in Port._wpt_manifest.

In this CL:
 - Move Manager._ensure_manifest to WPTManifest.ensure_manifest
 - Call it in Port._wpt_manifest

The purpose of this CL now is mainly to make rebaseline-cl etc. work again.

I'm not entirely sure that this is the absolute best solution; it might be theoretically possible that multiple Port objects could be created in separate processes, and that _wpt_manifest could be called on two at the same time, and both could start to try to copy the base manifest file at the same time. I don't *think* that this would happen now, since when actually running the tests, the manifest is always generated before gathering the list of tests (in Manager.run).

BUG= 698294 

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

[modify] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py
[modify] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager_unittest.py
[modify] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py
[modify] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl.py
[modify] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_cl_unittest.py
[modify] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_manifest.py
[add] https://crrev.com/db5746f9a7e4afd9a91fd8fafc275f74035e6614/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_manifest_unittest.py

Status: Fixed (was: Started)
Marking fixed for now, should re-open if there are more cases where manifest is needed but may not exist.
Status: Assigned (was: Fixed)
I'm getting the message: 

"""
Manifest not found at /usr/local/google/home/crouleau/chromium3/src/third_party/WebKit/LayoutTests/external/wpt/MANIFEST.json. See  http://crbug.com/698294 
"""

I get this message when I run the layout tests manually on my linux box. It's unclear from the message whether this is bad or not. Perhaps the log message simply needs to be removed?
Labels: -Pri-1 Pri-3
@crouleau what command are you using to run the tests? If you rm external/wpt/MANIFEST.json does the error persist?
Components: Blink>Infra
I was able to reproduce this with these steps:

 1. Remove MANIFEST.json.
 2. Run a subset of the layout tests that doesn't include web-platform-tests.

The message (which is at https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py?l=760) isn't really helpful, and could be removed.
Ah got it. Just to clarify, as I understand it's just an extraneous warning log. This isn't preventing anyone from running the tests, right?

In layout_tests/controllers/manager.py we only call WPTManifest.ensure_manifest if 'external' is in any of the paths or if there are no test paths given. To fix this problem, we could add the same condition to the WPT test collecting methods in tests() in layout_tests/port/base.py.
Cc: crouleau@chromium.org
Sorry for the late response. Yes, comment #7 is correct.

Yes, it's just an extraneous warning log. It didn't prevent me from running the tests; it just made me confused.
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 7 2017

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

commit 72179b10a1692277f414a2ed1882e406d4d620a7
Author: Jeff Carpenter <jeffcarp@chromium.org>
Date: Mon Aug 07 20:59:35 2017

Prevent MANIFEST.json warning log by only adding WPT if WPT path specified

Bug:  698294 
Change-Id: I89ba0f102d25ef6b8b1e8d5bd5018b2dd6b73c8e
Reviewed-on: https://chromium-review.googlesource.com/602740
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492403}
[modify] https://crrev.com/72179b10a1692277f414a2ed1882e406d4d620a7/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/base.py

Status: Fixed (was: Assigned)
Thanks crouleau and Jeff :-)

Sign in to add a comment