New issue
Advanced search Search tips

Issue 734799 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[WPT Sync] Extract is_synced/is_baseline logic into a common location

Project Member Reported by qyears...@chromium.org, Jun 19 2017

Issue description

There's a little piece of knowledge -- that testharness baselines in wpt end with -expected.txt, and they're not synced -- which is now duplicated in at least a couple of places:

  w3c/test_importer.py:286
  w3c/chromium_commit.py:124
  w3c/gerrit.py (see https://chromium-review.googlesource.com/c/540146/)

It seems like we might want to put this logic in w3c/common.py.

I'm thinking something like this:

    def is_synced(basename):
        return basename in ('OWNERS', 'testharnessreport.js', 'MANIFEST.json' or is_testharness_baseline(basename)

    def is_testharness_baseline(basename):
        return basename.endswith('-expected.txt')


And then this would be used to decide:
 - which files are exportable, either from Gerrit patches or commits
 - which files should not be deleted when doing an import
 
I tried to deduplicate is_baseline but a problem I encountered is that doing so would create a cyclical dependency between common.py and chromium_commit.py (common.py currently imports chromium_commit.py, and moving ChromiumCommit.is_baseline to chromium_commit.py would require having chromium_commit.py importing common.py). I uploaded the CL as-is to demonstrate:
https://chromium-review.googlesource.com/540835
Components: Blink>Infra>Ecosystem
Cc: qyears...@chromium.org
Components: -Blink>Infra
Summary: [WPT Sync] Extract is_synced/is_baseline logic into a common location (was: Extract is_synced/is_baseline logic into a common location in webkitpy/w3c.)
Cc: robertma@chromium.org
Stumbled on the related TODOs when browsing the code. Just wanted to provide another perspective from a newcomer to this code base: we might move exportable_commits_over_last_n_commits (along with its accompanied private function, and probably is_exportable) to a new place, something like chromium_exportable_commits.py

The rationales are two-fold:
1. Initially I was confused by the functions for a second -- which repo do they work in? What commits (Gerrit or GitHub) are they looking at? Especially when they are imported in importer/exporter, the path webkitpy.w3c.common has very little information.
2. common.py would become a place for smaller utility functions like is_baseline, is_ignored, read_credentials, which would also solve the circular dependency issue if we are to deduplicate is_baseline.

What do you think? If it sounds like a good idea, I can send out a quick CL.
Extracting exportable_commits_over_last_n_commits and _exportable_commits_since to chromium_exportable_commits.py makes sense, sounds good to me.
Cc: -robertma@chromium.org
Owner: robertma@chromium.org
Status: Assigned (was: Available)
Status: Started (was: Assigned)
Starting the refactoring as mentioned in #4, as it would help fixing  issue 754613 .
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 23 2017

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

Project Member

Comment 9 by bugdroid1@chromium.org, Aug 31 2017

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

commit 0ca53e37a8293d6575444a72f477c1643c9c24ea
Author: Robert Ma <robertma@chromium.org>
Date: Thu Aug 31 20:27:09 2017

Extract all filename testing/filtering routines into common.py for reuse

There were similar routines for checking whether a file is exportable based on
its name (or path) all over the codebase. Some of them were slightly different,
which caused intricate issues like 759181. This change extracts all of them
into one central place (w3c/common.py) so that they can be reused.


Bug:  734799 ,  759181 
Change-Id: Ie54e2f455c046383f1aaab2670596ffdf24128b7
Reviewed-on: https://chromium-review.googlesource.com/643760
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498977}
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_exportable_commits_unittest.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/common_unittest.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_copier.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
[modify] https://crrev.com/0ca53e37a8293d6575444a72f477c1643c9c24ea/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_importer.py

Status: Fixed (was: Started)

Sign in to add a comment