Port._wpt_manifest incorrectly assumes that MANIFEST.json always exists. |
|||||||
Issue descriptionAfter 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?
,
Mar 3 2017
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.
,
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
,
Mar 5 2017
Marking fixed for now, should re-open if there are more cases where manifest is needed but may not exist.
,
Aug 3 2017
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?
,
Aug 3 2017
@crouleau what command are you using to run the tests? If you rm external/wpt/MANIFEST.json does the error persist?
,
Aug 4 2017
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.
,
Aug 4 2017
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.
,
Aug 4 2017
CL for that approach: https://chromium-review.googlesource.com/c/602740
,
Aug 4 2017
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.
,
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
,
Aug 7 2017
Thanks crouleau and Jeff :-) |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by jeffcarp@chromium.org
, Mar 3 2017