[WPT Export] Run wpt lint in CQ for exportable WPT changes |
||||||||||||||
Issue descriptionIn the web-platform-tests' continuous integration system[1], there are currently a few tests that are run: ci_lint.sh ci_built_diff.sh ci_stability.sh If a PR can't get passed these checks, then in principle we shouldn't merge it (we may have to revert the Chromium CL). So, we should run these checks in Chromium for any exportable CLs, ideally before the Chromium CL is ever landed. Doing the lint is relatively trivial - this could be done by adding a PRESUBMIT in LayoutTests/external. The built_diff check might also be a relatively quick check, in which case it could be a presubmit. Doing the "stability check" may be more complex - AFAIK, the stability check involves getting new/changed tests to run ~10 times on stable versions of each major browser, and fails if the results aren't consistent. Does this sound possible for us? (What about running the tests 10+ times on just Chrome and failing if the results aren't consistent?) [1] https://github.com/w3c/web-platform-tests/blob/master/.travis.yml 1. [x] Unskip wpt/lint.whitelist in LayoutTests/W3CImportExpectations https://crrev/c/471787/ 2. [x] Parameterize whitelist https://github.com/w3c/wpt-tools/pull/202 (will apply patch locally) 3. [x] Import lint.py into webkitpy/thirdparty/wpt https://crrev.com/c/478155/ 4. [x] Add a check to LayoutTests/external/PRESUBMIT.py that invokes lint.py
,
Jan 31 2017
I suppose that for the stability checker, we want to run the test 10 times on the content_shell binary we just build, only for other engines would we want to run the final browsers. Hmmm... this means that we could have a CL fixing the cause of instability itself also slightly tweaking the test, perhaps adding a comment. It would make it through our CQ, but probably fail the WPT stability checker. Maybe it would be rare enough that we can just merge into upstream WPT manually in those cases, but it's not stellar. Looking into what it would take to run modified tests 10 times in content_shell would certainly be a nice start here. To have other browsers running as part of our CQ is important but I guess is a bit more work, and might require some approvals to run those binaries?
,
Feb 3 2017
ci_built_diff has the slight awkwardness of needing Cairo and I think its headers installed. The lint we should absolutely run and add ASAP; it depends only on Python 2.
,
Feb 3 2017
In terms of the edge cases where we could make it through our CQ then fail the upstream CI, I think there was some agreement that once we're checking essentially the same things in our CQ as the upstream CI, we should just stop running the upstream CI entirely (it's redundant except for edge cases which probably aren't worth worrying about unless we have real evidence they're a problem).
,
Feb 3 2017
There was still the concern about actually having the test results in the export PR for later validation. To bypass the upstream checks would be a bit of an optimization, so I don't think we should look into that until we see a reason to, like if perhaps Travis is just flaky and we can't figure out how to fix it.
,
Feb 3 2017
I've pushed for it to be disabled in the Mozilla case with bulk uploads because it consumes Travis for several hours typically with all the builds it causes; if we're doing one CL at a time then we don't have that problem and it probably makes sense to run the upstream CI.
,
Feb 7 2017
Note: Just started trying to add the lint script to .../webkitpy/thirdparty/wpt/, but hit a little issue running it from there: the lint script needs to be able to find lint.whitelist, which needs to be able to checked in as well (and updated during wpt auto-import). https://github.com/w3c/wpt-tools/blob/master/lint/lint.py may need to be changed.
,
Apr 3 2017
Is there a plan of action/next steps here to be able to run lint on WPT CLs in the Chromium codebase? I'm upstreaming some new tests and would much rather lint them locally than risk an upstream rejection :)
,
Apr 3 2017
Agreed, upstream rejection is a huge pain for all parties involved. My next step is to create WPT PRs once Chromium CLs are uploaded to Gerrit in order to kick off Travis CI. If Travis fails, comment back on the CL that it won't be upstreamed unless the failures are resolved. ( bug 700092 )
,
Apr 3 2017
,
Apr 3 2017
,
Apr 6 2017
I think that what's required here is: 1. Unskip wpt/lint.whitelist in LayoutTests/W3CImportExpectations 2. Make a change in lint.py upstream in wpt-tools (https://github.com/w3c/wpt-tools/blob/master/lint/lint.py) to take an optional path to the whitelist file. 3. Get a copy of lint.py in webkitpy/thirdparty/wpt by editing WPTWhitelist and running thirdparty/wpt/checkout.sh. 4. Finally, add a check in LayoutTests/PRESUBMIT.py or LayoutTests/external/PRESUBMIT.py which invokes lint.py.
,
Apr 7 2017
,
Apr 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9309a096d9fff76eceefaf529cf0a3df85617c13 commit 9309a096d9fff76eceefaf529cf0a3df85617c13 Author: Jeff Carpenter <jeffcarp@chromium.org> Date: Fri Apr 07 21:20:55 2017 [WPT Sync] Unskip wpt/lint.whitelist in W3CImportExpectations This step is in preparation for running the WPT linter as part of Chromium PRESUBMIT. Lint errors make up the majority of cases when and upstream WPT PR fails Travis CI, so running the same lint check in PRESUBMIT will help curb a large number of stuck PRs. Bug: 686927 Change-Id: Ic54d73368db2bcc0a5ea09411c674681bbaa6972 Reviewed-on: https://chromium-review.googlesource.com/471787 Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#463007} [modify] https://crrev.com/9309a096d9fff76eceefaf529cf0a3df85617c13/third_party/WebKit/LayoutTests/W3CImportExpectations
,
Apr 11 2017
PR for step #2 is up in wpt-tools: https://github.com/w3c/wpt-tools/pull/202 cc/geoffers@gmail.com
,
Apr 17 2017
,
Apr 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/93b7082279a607e490ebb091d62445f3e78a61dc commit 93b7082279a607e490ebb091d62445f3e78a61dc Author: Jeff Carpenter <jeffcarp@chromium.org> Date: Mon Apr 17 21:15:35 2017 Import WPT lint tool into webkitpy/thirdparty/wpt We're going to use the lint tool to run WPT lints during Chromium PRESUBMIT to catch lint errors that would have otherwise surfaced during the WPT export process. Bug: 686927 Change-Id: Ia4c873fdabc0930ccd9e3af744422dc78939f09a Reviewed-on: https://chromium-review.googlesource.com/478155 Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#465019} [modify] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/README.chromium [modify] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/WPTHeads [modify] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/WPTWhiteList [add] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/lint [add] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/lint/__init__.py [add] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/lint/fnmatch.py [add] https://crrev.com/93b7082279a607e490ebb091d62445f3e78a61dc/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/lint/lint.py
,
Apr 18 2017
,
Apr 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce95e5d83f3538e5797b85f986e52afda8a7a40a commit ce95e5d83f3538e5797b85f986e52afda8a7a40a Author: Jeff Carpenter <jeffcarp@chromium.org> Date: Tue Apr 18 21:51:36 2017 [WPT Export] Apply whitelist parameterization patch locally This patch is from https://github.com/w3c/wpt-tools/pull/202, however since wpt-tools is being merged into WPT, it makes more sense just to apply the relevant parts of the patch locally for now. Bug: 686927 Change-Id: Ie18709097dbc3619d1c2a75e80bcf90261dfc014 Reviewed-on: https://chromium-review.googlesource.com/479800 Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#465389} [modify] https://crrev.com/ce95e5d83f3538e5797b85f986e52afda8a7a40a/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/lint/lint.py
,
May 30 2017
Running lint would be really nice to avoid issues once the change is already in a GitHub PR.
,
Jun 1 2017
,
Jun 6 2017
Modifying title to refer specifically to lint. We can make another bug if we decide running stability checks is feasible as a presubmit. One problem I'm running into is that the WPT lint script consistently takes 140-150 seconds to run. This is kind of a pain and I'm sure the script can be sped up. Is that level of latency acceptable for landing in Chromium right now, or should we investigate speeding up the script before landing?
,
Jun 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67 commit 0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67 Author: Jeff Carpenter <jeffcarp@chromium.org> Date: Tue Jun 13 01:11:46 2017 Run WPT lint as a PRESUBMIT for WPT changes This CL: - Adds a new PRESUBMIT for WPT that calls WPT lint on affected files - Adds a --repo-root parameter to WPT lint Bug: 686927 Change-Id: I5d736bec4628e82e2d801051320b18c14f4a4c38 Reviewed-on: https://chromium-review.googlesource.com/526398 Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org> Reviewed-by: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#478853} [add] https://crrev.com/0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67/third_party/WebKit/LayoutTests/external/PRESUBMIT.py [modify] https://crrev.com/0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/README.chromium [modify] https://crrev.com/0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/lint [modify] https://crrev.com/0c0029fc1d20f7c0b3da2df268f7e8e1c09c7b67/third_party/WebKit/Tools/Scripts/webkitpy/thirdparty/wpt/wpt/tools/lint/lint.py
,
Jun 13 2017
Is it live now? This is great!
,
Jun 14 2017
Just tested this out to verify it, and found a bug; uploaded https://chromium-review.googlesource.com/c/536234/ to fix that.
,
Jun 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/295132a293eadbdee96c0a728c811b4d0c2a481b commit 295132a293eadbdee96c0a728c811b4d0c2a481b Author: Quinten Yearsley <qyearsley@google.com> Date: Thu Jun 15 18:02:26 2017 Fix wpt lint presubmit script. Specifically, this changes the the --ignore-glob flag in the command. This CL also makes a couple minor style/comment changes and makes the presubmit skip the linting if no files in wpt were changed. Bug: 686927 Change-Id: I17f55fdaa30b3ecdf82cae1884b332a0ccff8afe Reviewed-on: https://chromium-review.googlesource.com/536234 Reviewed-by: Jeff Carpenter <jeffcarp@chromium.org> Commit-Queue: Quinten Yearsley <qyearsley@chromium.org> Cr-Commit-Position: refs/heads/master@{#479755} [modify] https://crrev.com/295132a293eadbdee96c0a728c811b4d0c2a481b/third_party/WebKit/LayoutTests/external/PRESUBMIT.py
,
Jun 15 2017
,
Jun 16 2017
Now that lint has landed, what are people's thoughts on running further stability checks? My opinion is that it wouldn't be worth the effort to set up stability checking in our CQ. Since our CQ's environment is different from that of Travis CI, running the checks in our CQ by no means guarantees unstable tests won't make it upstream. Furthermore, the cause of stuck PRs is not commonly flakiness on Chrome, so we'd need to run other browsers in our CQ to even attempt to replicate the Travis CI checks that run. I think a better hands-off long-term solution is having the exporter add a Code Review -1 to the Gerrit CL if Travis CI fails (as qyearsley suggested) to prevent an exportable CLs that would otherwise get stuck in limbo from landing.
,
Jun 22 2017
Yeah, that sounds very reasonable, but would a -1 block landing? Occasionally we'll want to merge things against Travis's recommendations, and then we have to land the Chromium CL first.
,
Jun 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e8961f25e7bdf5bb821821b227924818756c7373 commit e8961f25e7bdf5bb821821b227924818756c7373 Author: Quinten Yearsley <qyearsley@google.com> Date: Thu Jun 22 15:12:27 2017 Fix spelling in css-writing-modes-3 and test linter Bug: 686927 Change-Id: I428280c1742dde5dc76cb7f0f3c518d66dcb6ef8 Reviewed-on: https://chromium-review.googlesource.com/535085 Commit-Queue: Koji Ishii <kojii@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Cr-Commit-Position: refs/heads/master@{#481535} [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/contiguous-floated-table-vlr-007.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/contiguous-floated-table-vlr-009.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/contiguous-floated-table-vrl-006.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/contiguous-floated-table-vrl-008.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-003.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-005.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-007.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-009.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-011.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-013.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-021.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vlr-023.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vrl-002.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vrl-004.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vrl-006.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vrl-008.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vrl-010.xht [modify] https://crrev.com/e8961f25e7bdf5bb821821b227924818756c7373/third_party/WebKit/LayoutTests/external/wpt/css/css-writing-modes-3/line-box-height-vrl-012.xht
,
Jun 22 2017
@foolip that's a good point, we'll need some sort of escape hatch. After talking with jparent and agable about this, it sounds like issuing a Code-Review -1 is not idiomatic for a bot - setting up a Trybot to check Travis CI status would be better. I'm going to set up bugs & a sync meeting for that soon. Since the main work of this bug has been completed, I'm going to close it.
,
Jul 3 2017
,
Jul 3 2017
|
||||||||||||||
►
Sign in to add a comment |
||||||||||||||
Comment 1 by qyears...@chromium.org
, Jan 30 2017