[WPT Sync] Same CL exported twice, leading to incorrect exportable-but-not-exported result. |
|||||||
Issue descriptionToday, the exporter seems to have exported the same CL twice: https://crrev.com/472192 https://github.com/w3c/web-platform-tests/pull/5948 https://github.com/w3c/web-platform-tests/pull/5964 So we have this sequence of recent exports: For example, we have this sequence of exports: https://github.com/w3c/web-platform-tests/pull/5948 https://crrev.com/472192 https://github.com/w3c/web-platform-tests/pull/5949 https://crrev.com/472215 https://github.com/w3c/web-platform-tests/pull/5953 https://crrev.com/472332 https://github.com/w3c/web-platform-tests/pull/5961 https://crrev.com/472402 (revert of https://crrev.com/472192) https://github.com/w3c/web-platform-tests/pull/5962 https://crrev.com/472409 https://github.com/w3c/web-platform-tests/pull/5964 https://crrev.com/472192 (again) Now, as a result, the latest export commit in WPT has the commit position 472192. So, LocalWPT.most_recent_chromium_commit is returning ChromiumCommit(472192). Then, when exportable commits after that commit are listed, it's listing r472215, r472332, r472402, and r472409. The revert commit actually applies cleanly (because the original commit was relanded), so it is considered an exportable but not yet exported commit. But, the exporter is not exporting the revert commit again, because the pull request is not open. Recent wpt-exporter output: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwpt-exporter%2F19863%2F%2B%2Frecipes%2Fsteps%2Fcreate_PR_or_merge_in-flight_PR%2F0%2Fstdout The main thing I don't understand right now is: Why did the exporter create https://github.com/w3c/web-platform-tests/pull/5964?
,
May 17 2017
Thank you for investigating this. That's a good questoin, I can't find the build where the exporter actually created the PR. I'll keep looking into it. One other idea I had is since the exporter is currently only pulling 100 PRs from the GH API, maybe the order got shuffled and we didn't pull down the correct PRs. I'm pretty sure the PRs should be ordered latest first, but I'll check.
,
May 17 2017
It looks like the PR GH API response in reverse chronological order, so I don't think that's the problem: https://api.github.com/search/issues?q=repo:w3c/web-platform-tests%20type:pr%20label:chromium-export&page=1&per_page=100
,
May 22 2017
,
Jul 3 2017
,
Jul 3 2017
,
Jul 5 2017
Already had jeffcarp@ as the owner so setting to assigned, but is this still a problem? If not then it can be closed.
,
Jul 5 2017
I haven't seen this problem pop up recently. I still don't understand this problem completely, but we no longer rely on LocalWPT.most_recent_chromium_commit.
,
Jul 6 2017
Lowering priority to 3, if this comes up in triage as having no activity for a long time then I think we can close it.
,
Sep 27 2017
I think it's safe to say this is no longer an issue. Various possible causes have been fixed in the past two months, and we have not seen any new case for a long time. Closing. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by qyears...@chromium.org
, May 17 2017The general overall export process is now: (a) First: Look at open Gerrit CLs, and create or update corresponding PRs. (b) Then: Look over the last 5000 Chromium commits; for each one: (c) Is it exportable? (Can a patch be created that applies cleanly?) If it's not exportable, ignore it and move on. (d) If yes, it's exportable. Does it have a corresponding PR? (e) If yes, is that PR open? (f) If it's open, try to merge it. (g) If that PR is already closed, ignore it and move on. (h) If it's exportable and there's no corresponding PR, try to make one. In this case, *after* the revert, the original CL was considered exportable again (it contained wpt changes and the patch would apply cleanly again). What *should* have happened, I think, is the exporter should have seen that PR was already created for the original patch and closed, so the commit is no longer eligible for creating another PR. So I think something might have gone wrong in part (d). That is, I think that TestExporter.corresponding_pull_request_for_commit might have returned None when it should have returned the PR https://github.com/w3c/web-platform-tests/pull/5948.