I'm upstreaming it manually in https://github.com/w3c/web-platform-tests/pull/6483.

jeffcarp@, when you've found the root cause of this, can you also go through all previous commits to see if any others may have been missed without anyone noticing?

Oops, a manual export was already in progress in https://github.com/w3c/web-platform-tests/pull/6424.

Here's the patch failure that original CL would have generated:

Failed to run "['git', 'apply', '-']" exit_code: 1 cwd: /tmp/wpt

output: error: cannot apply binary patch to 'content-security-policy/support/passs.png' without full index line
error: content-security-policy/support/passs.png: patch does not apply

Hmm, so is it a problem with binary files? At any rates, I think that in case of conflicts we would have failed at a similar point. Does that mean that if there are conflicts, a CL will also be silently skipped?

It looks like in addition to the lack of export, it's also a problem that it's not considered exportable in the first place, or import would have been blocked on this, right?

Yes, CLs that have conflicts will silently be skipped. That is (as we saw) definitely not optimal behavior.

As you stated, the importer should be blocked on patches like this - but only if they end up on master, which is a result of the exporter not seeing it as exportable in the first place due to a patch failure.

What should the exporter do when it encounters a patch failure? I'm guessing it should alert the Ecosystem oncall somehow - via email? via an audit log the oncall can monitor?

Would it be straightforward to make https://build.chromium.org/p/chromium.infra.cron/builders/wpt-exporter go red in that case, with something clearly stated in the logs? Then the rotation would pick it up, and building monitoring on top of that (covering all failure reasons) seems like a decent layering of things.

That's doable and would be straightforward for the rotation triage. I can make a separate bug for that or transform this one.

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

commit b742cce168cd70acc7f1498ba03a3c42be17fa72
Author: Jeff Carpenter <jeffcarp@chromium.org>
Date: Fri Jul 14 03:31:53 2017

[WPT Export] When scanning commits, filter out those with PRs

Also:
- Print out `git apply` error message in LocalWPT.test_patch
- Add accidentally omitted return statement in WPTGitHub.pr_for_chromium_commit

Bug:  737898 
Change-Id: Id35454a3a5a91dd4963c7e7f0f86d5a29fb79cfd
Reviewed-on: https://chromium-review.googlesource.com/571400
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Commit-Queue: Jeff Carpenter <jeffcarp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486663}
[modify] https://crrev.com/b742cce168cd70acc7f1498ba03a3c42be17fa72/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py
[modify] https://crrev.com/b742cce168cd70acc7f1498ba03a3c42be17fa72/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py
[modify] https://crrev.com/b742cce168cd70acc7f1498ba03a3c42be17fa72/third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py
[modify] https://crrev.com/b742cce168cd70acc7f1498ba03a3c42be17fa72/third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py

Created  bug 743149  for turning the Exporter red when a patch doesn't apply.

> jeffcarp@, when you've found the root cause of this, can you also go through all previous commits to see if any others may have been missed without anyone noticing?

Ok, so I inspected every exportable commit since the introduction of the exporter. There were around 20-30 commits that generated patch errors. Most of them were adding files that already exist (the same version) or deleting files that no longer exist. There were 3 patches that had legitimately unexported changes:

1. Add test names for NetInfo tests
- https://chromium.googlesource.com/chromium/src/+/ae909c654a
- Date: Wed Jun 07 14:33:31 2017
- Problem: Modifies netinfo/netinfo-basics.html but changes aren't reflected in WPT.
- Remediation: created PR https://github.com/w3c/web-platform-tests/pull/6609

2. Don't crash web payments mojo on very long ID.
- Commit: https://chromium.googlesource.com/chromium/src/+/473dbadd4f
- Date: Mon Jun 05 13:58:27 2017
- Problem: Two commits around the same time (one in Chromium, one in WPT) modify a line in payment-request/payment-request-constructor.https.html.
- Remediation: reached out to Chromium author (rouslan@).

3. [PATCH] Implement HTMLMenuItemElement.label
- Commit: https://chromium.googlesource.com/chromium/src/+/ae4d8f0007
- Date: Wed, 26 Apr 2017 15:40:44 -0700
- Problem: One new file was not exported.
- Remediation: created PR https://github.com/w3c/web-platform-tests/pull/6608

As mentioned above,  bug 743149  and our Ecosystem oncall rotation will help prevent these types of issues from cropping up again.

Robert was just looking at #3; in that case, the importer also didn't find the original exportable chromium commit and so the file added (html/semantics/interactive-elements/the-menu-element/menuitem-label.html) was later deleted on another import.

I'm not sure if there was a bug that was already fixed there..., do you know?

So regarding 3, the commit was clobbered in a manual import: https://chromium-review.googlesource.com/c/530608

Looking at webkitpy.w3c.common I have a hypothesis why it happened: there were too many commits between the original commit that added the tests and the head when manual import was run:

$ git log ae4d8f00071215f9dfe05e1d762f34833c0eec9b..91aff5625b214afb9005b0dac0d42a218359e51a --pretty=oneline|wc -l
11076

which exceeds the default 5000 history window used in exportable_commits_over_last_n_commits.


Not sure it was the root cause and/or if there were other issues. Jeff do you know why the exporter did not pick up that commit?

@robertma I haven't dug into it further than that. That sounds like a reasonable hypothesis. 5000 commits currently points to July 14th, or only 20 days back.

Regardless of whether slipping out of the window is the cause of #3, it'd be great to turn both the Chromium commit and GitHub PR hard-coded windows into date-based windows so we can say in our documentation "your CL must be merged within 3 months or the exporter will forget about it." If that sounds good I'll make a separate bug.

As for this issue, since the original cause was found and all other commits were audited, I'm closing it.

I agree that we need to do something about the history window. The knowledge that something bad (e.g. clobbered by importer) could happen if a CL is not exported (and merged) to upstream within a certain time window is too implicit.

We should document it somewhere. And, using a date-based metric would definitely be more friendly than the number of commits.