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

Issue 879128 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 30
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

[WPT Export] "Import all remaining cookie tests into WPT" change not exported, then reverted

Project Member Reported by foolip@chromium.org, Aug 30

Issue description

https://chromium-review.googlesource.com/c/chromium/src/+/1021577 landed on Aug 16, with a lot of changes in third_party/WebKit/LayoutTests/external/wpt/.

https://ci.chromium.org/buildbot/chromium.infra.cron/wpt-exporter/96310 is the last wpt-exporter build before the commit, and passed.

https://ci.chromium.org/buildbot/chromium.infra.cron/wpt-exporter/96311 is the first wpt-exporter build including the commit, and failed.

However, "cookie" doesn't appear in the log, the failures was because https://chromium.googlesource.com/chromium/src/+/c3dade0056 (the immediately previous commit) couldn't be applied.

The exporter kept failing for the same reason until https://ci.chromium.org/buildbot/chromium.infra.cron/wpt-exporter/96318, and then began passing in https://ci.chromium.org/buildbot/chromium.infra.cron/wpt-exporter/96319. "cookie" still doesn't appear at the log then.

https://chromium-review.googlesource.com/c/chromium/src/+/1180077 is the import that ended up reverting the changes.

The same fate probably awaits https://chromium-review.googlesource.com/c/chromium/src/+/1196543 given that no export PR has been created.
 
The problem is here:
https://cs.chromium.org/chromium/src/third_party/blink/tools/blinkpy/w3c/chromium_exportable_commits.py?l=103&rcl=245b8ffc7a2ff951dff3b7ef61c35936611e659c

Any commit whose message begins with "Import" isn't considered exportable. This condition dates back to https://chromium.googlesource.com/chromium/src/+/06bc68e15d0b41c94ddc85a6b5713da14561823f.

At this point we should just drop that condition, import commits have "No-Export: true".
robertma@, I have a fix in progress.
Project Member

Comment 4 by bugdroid1@chromium.org, Aug 30

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

commit 34d106c9b3aae447635559612a96d718b61d2beb
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Thu Aug 30 14:49:57 2018

[WPT Export] Allow commits with "Import" in subject to be exported

These conditions are no longer needed, as real import commits have
'No-Export: true' in their commit message.

Bug:  879128 
Change-Id: I829373c595931d8b221066b4edbb55bcb4db8cb7
Reviewed-on: https://chromium-review.googlesource.com/1196447
Reviewed-by: Robert Ma <robertma@chromium.org>
Commit-Queue: Robert Ma <robertma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587571}
[modify] https://crrev.com/34d106c9b3aae447635559612a96d718b61d2beb/third_party/blink/tools/blinkpy/w3c/chromium_exportable_commits.py
[modify] https://crrev.com/34d106c9b3aae447635559612a96d718b61d2beb/third_party/blink/tools/blinkpy/w3c/chromium_exportable_commits_unittest.py
[modify] https://crrev.com/34d106c9b3aae447635559612a96d718b61d2beb/third_party/blink/tools/blinkpy/w3c/gerrit.py
[modify] https://crrev.com/34d106c9b3aae447635559612a96d718b61d2beb/third_party/blink/tools/blinkpy/w3c/gerrit_unittest.py

https://ci.chromium.org/buildbot/chromium.infra.cron/wpt-exporter/98525 is the first export to run after these changes.
Owner: foolip@chromium.org
Status: Fixed (was: Assigned)
Alright! It worked: https://github.com/web-platform-tests/wpt/pull/12762
From mkwst@ on chat:

> https://chromium-review.googlesource.com/c/chromium/src/+/1196543 is in a weird state where it looks like the importer bot reimported the old CL (in https://chromium-review.googlesource.com/c/chromium/src/+/1196908), but didn't push it to WPT.

> So I can't land the chromium patch, because it's already landed. Which means the bot won't auto-land the patch against WPT. Which possibly means that we should just land the PR against WPT? And hope that the bot doesn't remove things in the meantime?

What's happened is that when I fixed the definition of what is exportable, that affected not just the exporter but also the importer. The importer, to avoid reverting changes which have not yet been exported, applies all such commit on top of a clean import. From the commit message:

> With Chromium commits locally applied on WPT:
> 29a89cda66 "Import all remaining cookie tests into WPT"

I didn't predict this, but it is the intended behavior of import.

I've applied https://chromium-review.googlesource.com/c/chromium/src/+/1196543 to the commit before https://chromium-review.googlesource.com/c/chromium/src/+/1021577 was landed. The only difference is that these files were added and still exist on master:
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/chromium-tests-expected.txt
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/domain-tests-expected.txt
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/mozilla-tests-expected.txt
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/name-tests-expected.txt
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/ordering-tests-expected.txt
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/path-tests-expected.txt
third_party/WebKit/LayoutTests/external/wpt/cookies/http-state/value-tests-expected.txt

Since those are the only differences, the export PRs are identical. I'll get https://github.com/web-platform-tests/wpt/pull/12762 merged.
I listed all commits containing "Import" in LayoutTests/external/wpt (going back to 2017-01-17) except for proper wpt import and with some ad-hoc changes test them for exportability using the updated definition, but ignoring the _is_commit_exported condition. These are the ones that were listed:

Import all remaining cookie tests into WPT 29a89cda6627b5e4c4b1682501b84c9022dccd1a
Import MediaStreamTrack-getSettings and add expectation 2e3125b7dff512325a9a704b8d9f36483422d59c
Import //fetch from Web Platform Tests. 09fdd7696d0f20abd9984f23e0fd1aadd5e2f93b
Import web-platform-tests/editing/. a2d8556be5121f4c6640443f5a5c549ba450c770


The first one is the CL this bug is about. The remaining three are all actually imports from wpt into Chromium that depended on beginning with "Import" to not be exported.

In other words, it appears as though "Import all remaining cookie tests into WPT" was the first and last case of data loss due to this.

The instructions at https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Enabling-import-for-a-new-directory suggest depending on auto-import for importing new directories, so I'm not too concerned that people will try to use "Import foo" commits without No-Export going forward.

Sign in to add a comment