Script for exporting WPT

third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_wpt.py

+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import os
+
+CR_WPT_DIR = 'third_party/WebKit/LayoutTests/imported/wpt/'
+
+
+class ChromiumWPT(object):

[foolip, 2016/11/01 22:07:44]

A sentence or two of documentation describing the responsibilities of this class
would be great. And the "(object)" probably isn't needed?

[qyearsley, 2016/11/01 22:57:45]

It'd work without (object), but explicitly making classes inherit from object is
considered good practice I think:
https://wiki.python.org/moin/NewClassVsClassicClass

[jeffcarp, 2016/11/01 23:21:18]

The linter yells at me if classes don't inherit from object.

[foolip, 2016/11/02 13:15:32]

OK, I had no idea there was a new style in town :)

+
+    def __init__(self, host):
+        self.host = host
+        self.absolute_chromium_wpt_dir = os.path.abspath(os.path.join('..', '..', 'LayoutTests', 'imported', 'wpt'))

[foolip, 2016/11/01 22:07:44]

Does this assume that the script is being run from inside Tools/Scripts? Some
other scripts seem to use os.path.abspath(__file__), maybe some variation of
that could be used to make this current-working-dir-independent? (If you check
and no other scripts seem to actually work when run from elsewhere, ignore
this.)

[qyearsley, 2016/11/01 22:57:45]

In general code in this codebase avoids using os directly, and instead accesses
the filesystem through host.filesystem, which makes it more easily testable.
Also, you could avoid adding a member attribute here by making this a method.

The way this might be done in this codebase is:

from webkitpy.common.memoized import memoized
from webkitpy.common.webkit_finder import WebKitFinder

...

    @memoized
    def absolute_chromium_wpt_dir(self):
        finder = WebKitFinder(self.host.filesystem)
        return finder.path_from_webkit_base('LayoutTests', 'imported', 'wpt')

+
+    def exportable_commits_since(self, sha):
+        cr_commits = self.cr_commits_since(sha)
+        return filter(self.has_changes_in_wpt, cr_commits)
+
+    def has_changes_in_wpt(self, sha):
+        """Returns if a Chromium sha has changed files in
+        LayoutTests/imported/wpt unless they're expectations

[qyearsley, 2016/11/01 22:57:45]

This docstring could be clarified. A good docstring usually has a really short
one-line summary (e.g. Checks whether there are new exportable commits in
Chromium) followed by a blank line and then more details.

+        """
+        assert sha
+
+        diff_files = self.host.executive.run_command([
+            'git', 'diff-tree', '--no-commit-id',
+            '--name-only', '-r', '{}'.format(sha)

[foolip, 2016/11/01 22:07:44]

If sha is already a string, which I think it is, then just sha, otherwise
str(sha) would be a more explicit stringifier method.

[jeffcarp, 2016/11/01 23:21:18]

I don't think sha could ever be anything other than a string, so I think it's ok
here?

+        ])
+
+        files = [f.strip() for f in filter(bool, diff_files.split('\n'))]

[foolip, 2016/11/01 22:07:44]

Using splitlines() also handled \r\n if it happens to be in there. With that,
could you drop the strip bit? Using splitlines() throughout probably makes sense
if so.

After testing manually I understand that filter(bool, x) is to strip out empty
string, but that also seems to be solved by using splitlines().

[jeffcarp, 2016/11/01 23:21:18]

Ah thank you, that's much cleaner.

+        return any([f.startswith(CR_WPT_DIR) and '-expected' not in f for f in files])

[foolip, 2016/11/01 22:07:44]

In deps_updater.py there's an is_baseline. Using a shared definition would be
great.

[foolip, 2016/11/02 13:15:32]

Add a TODO for this?

+
+    def cr_commits_since(self, sha):
+        commits = self.host.executive.run_command([
+            'git', 'rev-list', '--reverse', '{}..HEAD'.format(sha)
+        ])
+        return filter(bool, commits.split('\n'))

[foolip, 2016/11/01 22:07:44]

Hopefully just return self....().splitlines() will work here.

+
+    def subject(self, sha):
+        return self.host.executive.run_command([
+            'git', 'show', sha, '--format=%s', '--no-patch'

[foolip, 2016/11/01 22:07:44]

Nit: put the sha last consistently, assuming that in fact works. If guess the
returned string also includes a newline. If it doesn't matter, fine, otherwise
rstrip()?

+        ])
+
+    def message(self, sha):

[foolip, 2016/11/01 22:07:44]

Document that this actually includes both subject and body? I only knew when I
looked up the documentation for git show.

+        return self.host.executive.run_command([
+            'git', 'show', sha, '--format=%B', '--no-patch'
+        ])
+
+    def commit_position_str(self, sha):

[foolip, 2016/11/01 22:07:44]

This is the same as the above, intentional?

+        return self.host.executive.run_command([
+            'git', 'show', sha, '--format=%B', '--no-patch'
+        ])
+
+    def wpt_diff_patch(self, sha):

[foolip, 2016/11/01 22:07:44]

When I saw it at the call site I wasn't sure what it was, maybe format_patch,
export_patch, or similar?

+        """Get patch but only for files in LayoutTests/imported/wpt"""
+        return self.host.executive.run_command([
+            'git', 'format-patch', '-1', '--stdout',
+            '{}'.format(sha), self.absolute_chromium_wpt_dir

[foolip, 2016/11/01 22:07:44]

I think this will actually need to use some of the code in has_changes_in_wpt to
enumerate affected files. Once there's an exportable commit, using that for a
unit test would be great too. A TODO to prepend "Chromium: " if you think that's
a reasonable format too.

[jeffcarp, 2016/11/01 23:21:18]

Passing self.absolute_chromium_wpt_dir limits the format-patch output to only
files in imported/wpt (if I understand you correctly).

Re: prepending Chromium: I had assumed we'd just re-use the same footer
(Cr-Commit-Position: refs/heads/master@{#428681}) in the WPT commits. Does that
work?

[foolip, 2016/11/02 13:15:32]

It'll still include changes to expectations files though.
Sure, let's start without the "Chromium: " prefix. The purpose was to make
nonsensical commit messages like "Fix crash in HTMLElement::internalThingy"
slightly less nonsensical with additional context. But Gecko already exports
commits without any prefix.

+        ])