Now that the manual fixup in the upstream (https://github.com/w3c/web-platform-tests/pull/6822) has been merged, the importer should not try to revert the changes anymore. Downgrading this as P1.

It is observed as expected in the most recent automatic import: https://build.chromium.org/p/chromium.infra.cron/builders/wpt-importer/builds/454 (CL: https://chromium-review.googlesource.com/c/612144).

I'm investigating the root cause and already have a rough idea. I'll follow up soon.

This seems to be a new problem related to  issue 734121  (trying to apply chromium commits on WPT head).

Here's the timeline of events prior to the incident:

1. PR was created for https://chromium-review.googlesource.com/c/602381 (noted as patch A from here on).
2. Patch A was merged into Chromium, but PR for A was stuck because of flakiness.
3. A fix for patch A was created and merged (https://chromium-review.googlesource.com/c/608629 noted as patch B), but PR was never for B as B cannot be applied to wpt without A.

At this point, chromium had both A and B, but wpt had none.

Then the importer ran. https://build.chromium.org/p/chromium.infra.cron/builders/wpt-importer/builds/453

During this run, the importer tried to re-apply exportable commits as it is supposed to. However, patch B was not considered as exportable since it could not be applied *independently* without patch A. Therefore, only A was re-applied, and B was clobbered.

We call "common.exportable_commits_over_last_n_commits" to find exportable commits (https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py?l=244&rcl=6f34424ff93b31f73b481b7e04ebba0e675983a3), which tests the commits individually. This is the desired behavior when we are looking for commits to export, but not so when we are trying to re-apply commits.

cc @qyearsley

Arr, very confusing.

I expect this kind of thing to not be very common. Aborting would also have been a reasonable behavior in this case... I'm not sure what the best solution is though.

What about we do the following in the importer:

* Test whether a commit is exportable by testing all the conditions in https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py?l=87&rcl=ef6e100e869af31681d62bfdb1b886e5cc18a963 *except* testing the patch, i.e. consider the "otherwise exportable" commits also exportable in this phase.
* For all exportable commits, try to reapply them cumulatively. If any commit fails, fail loud and abort. Only continue to import when all of them can be cleanly applied.


P.S. I had a thought when I was modifying common.py that the wording could use some improvements as the definition of "exportable" isn't clear. Now I believe it is not just the wording, but in fact we should really define an enum of status instead of a boolean of "exportable", e.g. ignored/exported/should_be_exported/exportable (just a random example, the choice of words is terrible).

That sounds like a good proposal.

In general we're expecting the common case to be that there are usually no exportable-but-not-exported commits, and if there are and this causes import to fail, this should generally be temporary.

Re-organizing the "exportability-related" functions that are currently in common.py sounds good as well. We could move that code to a separate file first ( bug 734799   #4 ).

The proposal in #4 makes sense to me. With the caveat that checking PR status ought to be replaced by checking the history of wpt from the commit we will try to export. A consequence of that would be that closing a PR wouldn't be enough to cause a Chromium change to eventually be reverted by import.

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

commit ea15079934e270dec29c94c00c6506202a43b370
Author: Robert Ma <robertma@chromium.org>
Date: Wed Aug 23 20:31:22 2017

Refactor exportable_commits_* out of common.py

This change is meant to be a refactor only. No behaviours or external
interfaces should change.

The refactor intends to de-clutter common.py (in preparation for
 bug 734799 ). It also clarifies the definition of "exportable": now all
commits with changes in wpt that are not ignored and not exported are
considered "exportable", regardless of whether they can apply. We
further define "exportable and clean", which is the previous definition
of "exportable". And, instead of a boolean, an enum is now used to
represent different "exportability" states of a commit. It is intended
to prepare for  bug 754613  where we will need the new definition of
"exportable".

Bug:  734799 ,  754613 
Change-Id: I237689476625d9b8b76457269269030c4a4f4646
Reviewed-on: https://chromium-review.googlesource.com/621747
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496792}
[add] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits.py
[add] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits_unittest.py
[modify] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py
[modify] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py
[modify] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
[modify] https://crrev.com/ea15079934e270dec29c94c00c6506202a43b370/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py

Just want to document another instance happened last week:

File:
https://chromium.googlesource.com/chromium/src/+log/master/third_party/WebKit/LayoutTests/external/wpt/credential-management/credentialscontainer-create-basics.https.html

The test was modified in 6be8c5b, but was later reverted in 08d2c5c. The importer clobbered the revert (i.e. resurrected 6be8c5b) in ab429eb (https://chromium-review.googlesource.com/c/chromium/src/+/614986), because the revert cannot be applied cleanly by itself. Eventually, everything was fixed in a later import https://chromium-review.googlesource.com/c/chromium/src/+/619152

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

commit 6e440eae8c3e6a9d9d1242fd984da9828a6e2f91
Author: Robert Ma <robertma@chromium.org>
Date: Fri Aug 25 21:14:14 2017

Importer reapplies all exportable commits

Previously importer only reapplied exportable commits that can be cleanly
applied to wpt independently. When there are two exportable commits and
the later one depends on the earlier one, only the earlier one would be
considered cleanly exportable and reapplied, which would cause the later
commit to be clobbered.

This CL also improves some documentation and adds a new test.

Bug:  754613 
Change-Id: I38752f0ef4aa83c6bc422264e2a3f1a0f9bca480
Reviewed-on: https://chromium-review.googlesource.com/634154
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497535}
[modify] https://crrev.com/6e440eae8c3e6a9d9d1242fd984da9828a6e2f91/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits.py
[modify] https://crrev.com/6e440eae8c3e6a9d9d1242fd984da9828a6e2f91/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits_unittest.py
[modify] https://crrev.com/6e440eae8c3e6a9d9d1242fd984da9828a6e2f91/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/6e440eae8c3e6a9d9d1242fd984da9828a6e2f91/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py
[modify] https://crrev.com/6e440eae8c3e6a9d9d1242fd984da9828a6e2f91/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer_unittest.py

I think this is causing the recent wpt-importer failures, which are basically all imports done after the latest CL landed:
* https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/583
* https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/584
* https://luci-milo.appspot.com/buildbot/chromium.infra.cron/wpt-importer/585

Oops... Well, the change itself is WAI. However, it reveals  issue 756202 .


Allow Date.now() rounding errors in timeOrigin test (https://chromium.googlesource.com/chromium/src/+/ed96173eaf) has already been exported, but was squashed in one PR with another commit, which cannot be recognized correctly. So the importer still thinks it is exportable and tries to reapply the patch. (Previously this change was not tried because it could not be applied cleanly, which is the wrong reason for not to reapply it.)