Bug 169462 - Add a script to automate W3c web-platform-tests pull request creations from WebKit commits
Summary: Add a script to automate W3c web-platform-tests pull request creations from W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks: 167911
  Show dependency treegraph
 
Reported: 2017-03-09 23:41 PST by youenn fablet
Modified: 2017-12-16 18:54 PST (History)
20 users (show)

See Also:


Attachments
Patch (11.99 KB, patch)
2017-03-09 23:55 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (24.03 KB, patch)
2017-03-21 22:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (38.15 KB, patch)
2017-05-07 23:56 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding URL to Chromium license (38.91 KB, patch)
2017-05-08 08:25 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (57.49 KB, patch)
2017-10-14 12:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (59.77 KB, patch)
2017-12-10 19:43 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (60.37 KB, patch)
2017-12-10 19:48 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing refs/heads regression (60.26 KB, patch)
2017-12-16 17:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-03-09 23:41:36 PST
Add a script to automate W3c web-platform-tests pull request creations from WebKit commits
Comment 1 youenn fablet 2017-03-09 23:55:55 PST
Created attachment 304035 [details]
Patch
Comment 2 youenn fablet 2017-03-09 23:57:16 PST
Current patch is creating the branch in the GitHub personal repo.
Adding PR creation from that should be straightforward.
Comment 3 youenn fablet 2017-03-12 22:28:24 PDT
Script might be useful for those changing imported/w3c/web-platform-tests tests.
Only working for those using git.
Comment 4 youenn fablet 2017-03-21 22:26:16 PDT
Created attachment 305076 [details]
Patch
Comment 5 youenn fablet 2017-03-21 22:32:48 PDT
wpt_github.py is a copy of the corresponding chromium file.
Comment 6 youenn fablet 2017-05-07 23:56:25 PDT
Created attachment 309348 [details]
Patch
Comment 7 Build Bot 2017-05-07 23:58:29 PDT
Attachment 309348 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:46:  expected 2 blank lines, found 1  [pep8/E302] [5]
ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:43:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:43:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44:  [MockWeb.request] Instance of 'MockWeb' has no 'responses' member  [pylint/E1101] [5]
ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:96:  [TestExporterTest.test_export] Instance of 'WPTGitHub' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]
ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:112:  [TestExporterTest.test_export] Instance of 'Bugzilla' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:176:  too many blank lines (3)  [pep8/E303] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:19:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:19:  Bad option value 'unused-argument'  [pylint/E0012] [5]
Total errors found: 9 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Alex Christensen 2017-05-08 04:35:23 PDT
Comment on attachment 309348 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309348&action=review

> Tools/Scripts/webkitpy/w3c/wpt_github.py:3
> +# 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.

We don't have this LICENSE file in our repository.  I'm not sure what to do with this license, but we can't just put this in as this patch exists now.
Comment 9 youenn fablet 2017-05-08 08:25:01 PDT
Created attachment 309359 [details]
Adding URL to Chromium license
Comment 10 youenn fablet 2017-05-08 08:25:55 PDT
(In reply to Alex Christensen from comment #8)
> Comment on attachment 309348 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309348&action=review
> 
> > Tools/Scripts/webkitpy/w3c/wpt_github.py:3
> > +# 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.
> 
> We don't have this LICENSE file in our repository.  I'm not sure what to do
> with this license, but we can't just put this in as this patch exists now.

Good point.
I updated the patch to add the chromium repository LICENSE url
Comment 11 Build Bot 2017-05-08 08:27:02 PDT
Attachment 309359 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:96:  [TestExporterTest.test_export] Instance of 'WPTGitHub' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]
ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:112:  [TestExporterTest.test_export] Instance of 'Bugzilla' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:180:  too many blank lines (3)  [pep8/E303] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:23:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:23:  Bad option value 'unused-argument'  [pylint/E0012] [5]
Total errors found: 7 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 youenn fablet 2017-05-14 22:55:54 PDT
Ping review?
Comment 13 Alex Christensen 2017-05-15 12:33:12 PDT
Comment on attachment 309359 [details]
Adding URL to Chromium license

View in context: https://bugs.webkit.org/attachment.cgi?id=309359&action=review

> Tools/Scripts/webkitpy/w3c/wpt_github.py:7
> +# The LICENSE file referenced above can be found at
> +# https://chromium.googlesource.com/chromium/src.git/+/master/LICENSE

I think we should include the LICENSE file in this repository to avoid problems if google changes how this server responds, the server location, the location on the server, etc. or if someone doesn't have access to their server for some reason.
Comment 14 youenn fablet 2017-05-15 12:48:06 PDT
(In reply to Alex Christensen from comment #13)
> Comment on attachment 309359 [details]
> Adding URL to Chromium license
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309359&action=review
> 
> > Tools/Scripts/webkitpy/w3c/wpt_github.py:7
> > +# The LICENSE file referenced above can be found at
> > +# https://chromium.googlesource.com/chromium/src.git/+/master/LICENSE
> 
> I think we should include the LICENSE file in this repository to avoid
> problems if google changes how this server responds, the server location,
> the location on the server, etc. or if someone doesn't have access to their
> server for some reason.

Talking with Geoff about it, the idea would be to inline the LICENSE in each file.
I'll do that in a follow-up or landing patch.
Comment 15 Ryosuke Niwa 2017-05-15 20:56:16 PDT
Comment on attachment 309359 [details]
Adding URL to Chromium license

View in context: https://bugs.webkit.org/attachment.cgi?id=309359&action=review

It's exciting to get this script working.

By the way, the question of where to put tests to be exported hasn't been settled.
e.g. Maciej in https://lists.webkit.org/pipermail/webkit-dev/2017-May/029022.html says:
" think it would be cleaner to have a separate directory of tests intended for [export], separate from imported tests. It could be right next to imported/w3c/web-platform-tests. I think mixing tests that are imported from upstream and tests intended for eventual upstreaming is more confusing than helpful."

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:587
> +    def reset_hard(self, revision):

Instead of just using the Git lingo, we should clarify what it does.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:588
> +        return self._run_git(['reset', '--hard', revision])

It's strange that we sometimes call this function with revision being a Git hashish but other times it's a branch name.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:590
> +    def am(self, options):

Please give a more descriptive name like "apply_mail_patch" or "apply_git_patch".

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:593
> +        args = ['am']
> +        args.extend(options)
> +        return self._run_git(args)

More idiomatic way to write this in Python would be self._run_git(['am'] + options)

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:595
> +    def commit(self, options):

I'd call this local_commit or commit_local since scm interface is shared with SVN.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:602
> +    def format_patch(self, options):
> +        args = ['format-patch']
> +        args.extend(options)

We should just use scm().create_patch(git_commit=commit_id) instead.
r- because of this feature duplication.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:51
> +        self.bugzilla = bugzillaClass()

These instance variables should be prefixed with _ since they're more or less private to this class.
If they're intended to be publicly accessible variable, then we have too many of those in this class.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:52
> +        self.webKitGit = gitClass('.', None, executive=self._host.executive, filesystem=self._filesystem)

In general, I don't really like tools relying on the current working directory like this.
It's fine to use the current working directory in the case no option was specified,
but it's better if the directory name of the WPT checkout was explicitly specifiable in an option
so that it's easier to call this function from another script for example.

Also, why are we assuming that WebKit checkout is a Git checkout?
Why can't we just let the host object automatically detect the SCM type?

> Tools/Scripts/webkitpy/w3c/test_exporter.py:60
> +        self.repository_directory = options.repository_directory

It's not obvious at all whether this repository refers to WebKit checkout to WPT checkout.
We should probably prefix each and every instance variable in this class by _webkit_ or _wpt_.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:66
> +        self.username = options.username
> +        if not self.username:
> +            self.username = os.environ.get('GITHUB_WPT_USER')
> +            if not self.username:
> +                raise ValueError("Missing GitHub username.")

We should also get it from git config user.name and keychain. See webkitpy/common/net/credentials.py
r- because of this.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:78
> +        self.branch_name = "webkit-" + str(self.bug_id)
> +        self.bugzilla_url = "https://bugs.webkit.org/show_bug.cgi?id=" + str(self.bug_id)
> +        self.commit_title = 'WebKit export of ' + self.bugzilla_url

We should use % or .format.

I also think "webkit-169462" is not exotic enough not to conflict with user-created branch name.
It should be something more unique like "wpt-export-for-webkit-169462"

In addition, it's ambiguous as to what commit_title means in this context. It's the commit message / PR description in WPT,
so I think a better name would be self._wpt_commit_message or self._wpt_pr_title.

Finally, "WebKit export of 169462" is a strange title for a PR request.
I would have phrased it like "Test updates for WebKit bug 169462".

> Tools/Scripts/webkitpy/w3c/test_exporter.py:82
> +        if not self.repository_remote:
> +            self.repository_remote = self.username

What's the point of this code? Why do we need to push to a specific user's repo?
I've never had to specify repository name like this since I usually only have one remote in my GitHub fork.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:84
> +        self.repository_origin_url = "https://" + self.username + "@github.com/" + self.username + "/web-platform-tests.git"

We should just run git config --get remote.origin.url instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:85
> +        self.repository_public_url = "https://github.com/" + self.username + "/web-platform-tests.git" if not options.repository_public_url else options.repository_public_url

This instance variable is never used other than the line below so it should be a local variable.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:86
> +        self.repository_public_tree_url = self.repository_public_url[:-4] + "/tree/"

Are we truncating ".git" by [:-4]? That is not obvious at all.
Also, it looks like this is only used in logging.
If we used "git config --get" above (please add a new method on Git class),
then we should be able to parse that result and construct this URL instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:95
> +    def _init_repository(self, url, repository_directory, gitClass):

I think we should call this _ensure_wpt_checkout or something since we're not init'ing a new git repository.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:109
> +        patchOptions = ["--no-update", "--no-clean", "--local-commit"]

Nit: should be patch_options.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:121
> +        # webKitPatch = WebKitPatch(self._filesystem.join(self._filesystem.abspath(__file__), "..", ".."))
> +        #(options, args) = webKitPatch.parse_args(patchOptions)
> +        #return webKitPatch.check_arguments_and_execute(options, args)

Please remove this commented out code. r- because of this.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:128
> +        self.git.reset_hard('origin/master')
> +        if not self.options.git_commit:
> +            self.webKitGit.reset_hard('HEAD~1')

It's confusing that we're calling reset_hard with a branch name or hashish.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:138
> +            _log.info("Retrying to create the branch")
> +            self.git.delete_branch(self.branch_name)
> +            self.git.checkout_new_branch(self.branch_name)

I don't think we should automatically delete a branch created by an user like this.
We should ask the user to confirm it's okay to delete the branch at minimum.
We can also add a flag to automatically delete the branch if there is a conflicting one
but doing so by default without giving any warning to the user is dangerous.
r- because of this.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:141
> +        try:
> +            self.git.am([patch, '--exclude', '*-expected.txt'])
> +        except Exception as e:

It's not a good idea to capture any exception like this.
Also, capturing the exception should probably happen in git.apply_patch (or whatever its name will be) instead.
That way, that function can take a patch name and files to exclude as arguments
instead o an arbitrary list of options.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:148
> +    def push_to_public_repository(self):

This is a very vague name. It should be something like push_to_remote_wpt_fork.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:157
> +        response = self.gitHub.create_pr(self.repository_remote + ':' + self.branch_name, self.commit_title, description)

It's not obvious at all that the first argument is a branch name.
I think we should use the name arguments as in: create_pr(remote_branch_name=self.repository_remote + ':' + self.branch_name).

> Tools/Scripts/webkitpy/w3c/test_exporter.py:170
> +        patch_data = self.webKitGit.format_patch([git_commit, 'LayoutTests/imported/w3c/web-platform-tests', '--stdout'])

Again, we should be using scm.create_patch(git_commit=commit_id) instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:175
> +        patch_data = patch_data.replace('LayoutTests/imported/w3c/web-platform-tests/', '')

It seems a little risky to do a global replaccement like this but I suppose text like this would never appear in the tests themselves.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:184
> +            _log.info("Will push branch to " + self.git.remote(["get-url", self.repository_remote]).strip())
> +            return
> +        _log.info("Will push branch to " + self.repository_origin_url)

Again, it's not obvious what repository_remote and repository_origin_url refer to.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:222
> +    -    USERNAME may be set using the environment variable GITHUB_WPT_USER or as a command line option.
> +    -    Github credential may be set using the environment variable GITHUB_WPT_AUTH or as a command line option. (Please provide a valid GitHub 'Personal access token' with 'repo' as scope)

We should support pulling username & password from the keychain.
Having to do this extra step is really annoying.
Even asking the user the password manually is better than requiring the user to get this extra token thing.
Also, if you have SSH key setup with GitHub, it should just work without having to do any authentication.
Comment 16 Dean Johnson 2017-07-12 12:57:40 PDT
Comment on attachment 309359 [details]
Adding URL to Chromium license

View in context: https://bugs.webkit.org/attachment.cgi?id=309359&action=review

Added some comments on the Git additions.

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:587
>> +    def reset_hard(self, revision):
> 
> Instead of just using the Git lingo, we should clarify what it does.

Maybe a docstring can do this? I find it written as-is to be the ideal way to write it as someone who's used git for some time. The same way for an SVN function that calls 'svn revert' I would expect it to be named 'revert' on an SVN class.

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:588
>> +        return self._run_git(['reset', '--hard', revision])
> 
> It's strange that we sometimes call this function with revision being a Git hashish but other times it's a branch name.

Maybe a better name for revision would be commit_identifier? Or commit_reference?

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:590
>> +    def am(self, options):
> 
> Please give a more descriptive name like "apply_mail_patch" or "apply_git_patch".

Given we're using the `am` argument, and this `Git` class is supposed to be representative of the `git` binary (as I understand it), am seems correct. That being said, I have not used this command before, so it may be worth writing a docstring explaining what the function is expected to do.

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:593
>> +        return self._run_git(args)
> 
> More idiomatic way to write this in Python would be self._run_git(['am'] + options)

+1.

>> Tools/Scripts/webkitpy/common/checkout/scm/git.py:595
>> +    def commit(self, options):
> 
> I'd call this local_commit or commit_local since scm interface is shared with SVN.

Maybe we should have commit do a few things as to adhere to the SVN interface (or maybe this is a terrible idea):
1. Add a push=True default argument to the `commit` function definition, which adheres to the SVN interface, and therefore unifies the SCM interface(s).
2. locally commit.
3. update the repository and push if push=True.

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:625
>          if quiet:

There should be Git unit tests near this file; can you please add some for the new functionality and ensure existing tests still pass?
Comment 17 youenn fablet 2017-10-05 05:40:40 PDT
Thanks for the review.
I'll try to provide a new version of the patch soon.

> By the way, the question of where to put tests to be exported hasn't been
> settled.
> e.g. Maciej in
> https://lists.webkit.org/pipermail/webkit-dev/2017-May/029022.html says:
> " think it would be cleaner to have a separate directory of tests intended
> for [export], separate from imported tests. It could be right next to
> imported/w3c/web-platform-tests. I think mixing tests that are imported from
> upstream and tests intended for eventual upstreaming is more confusing than
> helpful."

That is how all other browsers are doing.
Splitting the folders will make it very hard to export tests given that some links to common resources will no longer work.

> What's the point of this code? Why do we need to push to a specific user's
> repo?
> I've never had to specify repository name like this since I usually only
> have one remote in my GitHub fork.

The point here is that one pushes the commit to a GitHub repository.
Then a PR request is made from that repository to the WPT repository.
The local WPT repo will have these two remotes, one for getting updates from WPT and one to push changes.

Ideally, it should be a bot account/bot repository since the bot should run this script.
This should also work with whoever wants to run the script on its own.

> > Tools/Scripts/webkitpy/w3c/test_exporter.py:222
> > +    -    USERNAME may be set using the environment variable GITHUB_WPT_USER or as a command line option.
> > +    -    Github credential may be set using the environment variable GITHUB_WPT_AUTH or as a command line option. (Please provide a valid GitHub 'Personal access token' with 'repo' as scope)
> 
> We should support pulling username & password from the keychain.
> Having to do this extra step is really annoying.
> Even asking the user the password manually is better than requiring the user
> to get this extra token thing.
> Also, if you have SSH key setup with GitHub, it should just work without
> having to do any authentication.

The token is used for creating the PR not for pushing to the user repo.
Comment 18 Robert Ma 2017-10-05 07:34:11 PDT
Hi everyone, I'm the current maintainer of the Chromium import/export codebase. Glad to see the code is useful to you! Also happy to see WebKit setting up auto export! More contributors, better WPT :)

I notice the patch is a few months old. In the past few months, we've made some major cleanups and fixed a few bugs/TODOs in wpt_github.py. Most notably, the API and error handling are much more consistent now. Instead of swallowing the error and returning None, an exception is thrown for all unexpected API errors. It'd be great if you could pull in the newest version, Youenn.

The API indeed changed a bit in the process. Looks like the only change needed in this patch is the use of WPTGitHub.create_pr. Instead of returning the full response dict, a PR issue number (int) is now returned. Hence, you need to construct the PR URL from a string template rather than using `response['html_url']`. (And you might want to catch WPTGitHub.GitHubError and retry if you want to make the script more robust against server errors.)

Let me know if you have any questions! Thanks again!
Comment 19 Ryosuke Niwa 2017-10-05 15:11:52 PDT
(In reply to youenn fablet from comment #17)
> Thanks for the review.
> I'll try to provide a new version of the patch soon.
> 
> > By the way, the question of where to put tests to be exported hasn't been
> > settled.
> > e.g. Maciej in
> > https://lists.webkit.org/pipermail/webkit-dev/2017-May/029022.html says:
> > " think it would be cleaner to have a separate directory of tests intended
> > for [export], separate from imported tests. It could be right next to
> > imported/w3c/web-platform-tests. I think mixing tests that are imported from
> > upstream and tests intended for eventual upstreaming is more confusing than
> > helpful."
> 
> That is how all other browsers are doing.

I don't think that's a good enough reason to take that approach. You need to get the community consensus before you can take that approach.

> Splitting the folders will make it very hard to export tests given that some
> links to common resources will no longer work.

Why? We can just make a copy of resources or superimpose files to be exported on top of existing files when running tests.

> > > Tools/Scripts/webkitpy/w3c/test_exporter.py:222
> > > +    -    USERNAME may be set using the environment variable GITHUB_WPT_USER or as a command line option.
> > > +    -    Github credential may be set using the environment variable GITHUB_WPT_AUTH or as a command line option. (Please provide a valid GitHub 'Personal access token' with 'repo' as scope)
> > 
> > We should support pulling username & password from the keychain.
> > Having to do this extra step is really annoying.
> > Even asking the user the password manually is better than requiring the user
> > to get this extra token thing.
> > Also, if you have SSH key setup with GitHub, it should just work without
> > having to do any authentication.
> 
> The token is used for creating the PR not for pushing to the user repo.

Regardless, it's not okay to require users to set random environmental variables like these.
Comment 20 youenn fablet 2017-10-14 12:07:12 PDT
Created attachment 323815 [details]
Patch
Comment 21 Build Bot 2017-10-14 12:08:24 PDT
Attachment 323815 [details] did not pass style-queue:

Traceback (most recent call last):
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 54, in run
    args.extend(self._changed_files(state))
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 44, in _changed_files
    return self.cached_lookup(state, "changed_files")
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 62, in cached_lookup
    state[key] = promise(self, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 51, in <lambda>
    "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit),
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 235, in changed_files
    status_command = [self.executable_name, 'diff', '-r', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--']
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 215, in merge_base
    return self.remote_merge_base()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 517, in remote_merge_base
    return self._run_git(['merge-base', self.remote_branch_ref(), 'HEAD']).strip()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 524, in remote_branch_ref
    if not self._branch_ref_exists(remote_master_ref):
AttributeError: 'Git' object has no attribute '_branch_ref_exists'


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 youenn fablet 2017-10-14 16:02:02 PDT
Let's start with small steps.
The proposed script would be used by WebKit developers to make contributions to WPT by directly making edits to LayoutTests/imported/w3c/web-platform-tests.
The usual rules (commit WPT changes to WebKit when merged in WPT, merge WPT PR if r+ed in WebKit) remain unchanged.
Comment 23 Ryosuke Niwa 2017-10-14 17:42:59 PDT
Comment on attachment 323815 [details]
Patch

Again, I don't think we should take this approach until such a consensus is reached on webkit-dev.

People had varying opinions of how this should work, but one most obvious feedback was that we didn't want to modify imported tests directly.
Comment 24 youenn fablet 2017-10-14 22:17:14 PDT
> Again, I don't think we should take this approach until such a consensus is
> reached on webkit-dev.

I'll be happy to ping webkit-dev but I am not sure how much it will help for this particular patch.

This script is allowing to author changes to existing WPT tests/create new WPT tests in a WebKit checkout instead of a separate WPT checkout. Coupling this authoring with WebKit patch authoring has obvious benefits, as well as coupling bugzilla patch/WPT PR processes.

It may also be seen as a step towards one solution for which I am fine discussing more the details.
But it is useful on its own.

> People had varying opinions of how this should work, but one most obvious
> feedback was that we didn't want to modify imported tests directly.

The concern I heard was about WebKit repository containing changes in LayoutTests/imported/w3c/web-platform-tests that would not be in WPT GitHub repository. I do not see how this particular script goes against this valid concern.
Are there other concerns?
Comment 25 Ryosuke Niwa 2017-10-15 14:36:48 PDT
(In reply to youenn fablet from comment #24)
> > Again, I don't think we should take this approach until such a consensus is
> > reached on webkit-dev.
> 
> I'll be happy to ping webkit-dev but I am not sure how much it will help for
> this particular patch.
> 
> This script is allowing to author changes to existing WPT tests/create new
> WPT tests in a WebKit checkout instead of a separate WPT checkout. Coupling
> this authoring with WebKit patch authoring has obvious benefits, as well as
> coupling bugzilla patch/WPT PR processes.
> 
> It may also be seen as a step towards one solution for which I am fine
> discussing more the details.
> But it is useful on its own.

The problem here is that the previous webkit-dev discussion resulted in people opposing to the workflow of modifying imported scripts. This script will encourage that workflow.
Comment 26 Darin Adler 2017-11-22 11:37:00 PST
Oops, I probably should not have done review+ given the discussion above.
Comment 27 youenn fablet 2017-11-22 21:14:03 PST
We discussed this with Ryosuke and agreed with this first step.
There was no push back on WebKit dev either.
I’ll wait to land the patch for a few days in case I misunderstood things.
Comment 28 youenn fablet 2017-11-22 21:18:00 PST
(In reply to Robert Ma from comment #18)
> Hi everyone, I'm the current maintainer of the Chromium import/export
> codebase. Glad to see the code is useful to you! Also happy to see WebKit
> setting up auto export! More contributors, better WPT :)
> 
> I notice the patch is a few months old. In the past few months, we've made
> some major cleanups and fixed a few bugs/TODOs in wpt_github.py. Most
> notably, the API and error handling are much more consistent now. Instead of
> swallowing the error and returning None, an exception is thrown for all
> unexpected API errors. It'd be great if you could pull in the newest
> version, Youenn.
> 
> The API indeed changed a bit in the process. Looks like the only change
> needed in this patch is the use of WPTGitHub.create_pr. Instead of returning
> the full response dict, a PR issue number (int) is now returned. Hence, you
> need to construct the PR URL from a string template rather than using
> `response['html_url']`. (And you might want to catch WPTGitHub.GitHubError
> and retry if you want to make the script more robust against server errors.)
> 
> Let me know if you have any questions! Thanks again!

Thanks for the information, the patch now uses the latest wpt_github.py now.
Comment 29 youenn fablet 2017-12-10 19:43:09 PST
Created attachment 328944 [details]
Patch for landing
Comment 30 EWS Watchlist 2017-12-10 19:44:40 PST
Attachment 328944 [details] did not pass style-queue:

Traceback (most recent call last):
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/checkstyle.py", line 54, in run
    args.extend(self._changed_files(state))
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 44, in _changed_files
    return self.cached_lookup(state, "changed_files")
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 62, in cached_lookup
    state[key] = promise(self, state)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/tool/steps/abstractstep.py", line 51, in <lambda>
    "changed_files": lambda self, state: self._tool.scm().changed_files(self._options.git_commit),
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 235, in changed_files
    status_command = [self.executable_name, 'diff', '-r', '--name-status', "--no-renames", "--no-ext-diff", "--full-index", self.merge_base(git_commit), '--']
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 215, in merge_base
    return self.remote_merge_base()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 517, in remote_merge_base
    return self._run_git(['merge-base', self.remote_branch_ref(), 'HEAD']).strip()
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py", line 524, in remote_branch_ref
    if not self._branch_ref_exists(remote_master_ref):
AttributeError: 'Git' object has no attribute '_branch_ref_exists'


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 youenn fablet 2017-12-10 19:45:42 PST
Updated the patch to add a --remote-url option so that ssh GitHub configurations can be supported.
It might be beneficial to split the configuration of the WPT clone in its own dedicated script.
Comment 32 youenn fablet 2017-12-10 19:48:06 PST
Created attachment 328945 [details]
Patch for landing
Comment 33 EWS Watchlist 2017-12-10 19:49:33 PST
Attachment 328945 [details] did not pass style-queue:

Can't find a branch to diff against. svn-remote.svn.fetch is not in the git config and refs/remotes/origin/master does not exist


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 WebKit Commit Bot 2017-12-10 20:50:17 PST
The commit-queue encountered the following flaky tests while processing attachment 328945 [details]:

media/modern-media-controls/seek-backward-support/seek-backward-support.html bug 174916 (authors: graouts@apple.com, mcatanzaro@igalia.com, and ryanhaddad@apple.com)
The commit-queue is continuing to process your patch.
Comment 35 WebKit Commit Bot 2017-12-10 20:50:40 PST
The commit-queue encountered the following flaky tests while processing attachment 328945 [details]:

svg/animations/svgtransform-animation-1.html bug 179354 (authors: ap@webkit.org, arv@chromium.org, krit@webkit.org, mark.lam@apple.com, and zimmermann@kde.org)
The commit-queue is continuing to process your patch.
Comment 36 WebKit Commit Bot 2017-12-10 21:04:37 PST
Comment on attachment 328945 [details]
Patch for landing

Clearing flags on attachment: 328945

Committed r225737: <https://trac.webkit.org/changeset/225737>
Comment 37 WebKit Commit Bot 2017-12-10 21:04:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Radar WebKit Bug Importer 2017-12-10 21:06:37 PST
<rdar://problem/35960387>
Comment 39 Zan Dobersek 2017-12-11 02:03:33 PST
Comment on attachment 328945 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=328945&action=review

> Tools/Scripts/webkitpy/common/checkout/scm/git.py:524
> -            if not self._branch_ref_exists(remote_master_ref):
> +            if not self.branch_ref_exists(remote_master_ref):

This breaks uploads from a Git clone of the repository, since now `git show-ref` is verifying the 'refs/heads/refs/remotes/origin/master' ref, which doesn't exist, while before it verified the 'refs/remotes/origin/master' ref.
Comment 40 Zan Dobersek 2017-12-11 02:13:08 PST
(In reply to Build Bot from comment #33)
> Attachment 328945 [details] did not pass style-queue:
> 
> Can't find a branch to diff against. svn-remote.svn.fetch is not in the git
> config and refs/remotes/origin/master does not exist
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This is an example of that error.

All EWSs seem to be affected, so I'll roll this out.

Otherwise, a temporary workaround for this would be adding a svn-remote.svn.fetch entry:

$ git config --add svn-remote.svn.fetch "refs/remotes/origin/master"
Comment 41 Zan Dobersek 2017-12-11 02:17:06 PST
Reverted r225737 for reason:

Breaks

Committed r225742: <https://trac.webkit.org/changeset/225742>
Comment 42 youenn fablet 2017-12-16 17:34:26 PST
Created attachment 329587 [details]
Fixing refs/heads regression
Comment 43 EWS Watchlist 2017-12-16 17:35:51 PST
Attachment 329587 [details] did not pass style-queue:


ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/common/net/web_mock.py:44:  Bad option value 'unused-argument'  [pylint/E0012] [5]
ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:130:  [TestExporterTest.test_export] Instance of 'Bugzilla' has no 'calls' member (but some types could not be inferred)  [pylint/E1103] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:91:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:159:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:178:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:228:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:260:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:280:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:305:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:330:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github.py:409:  whitespace before ')'  [pep8/E202] [5]
ERROR: Tools/Scripts/webkitpy/w3c/wpt_github_mock.py:35:  Bad option value 'unused-argument'  [pylint/E0012] [5]
Total errors found: 13 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 youenn fablet 2017-12-16 17:38:23 PST
(In reply to Zan Dobersek from comment #41)
> Reverted r225737 for reason:
> 
> Breaks
> 
> Committed r225742: <https://trac.webkit.org/changeset/225742>

Thanks Zan for reverting it.
I fixed the patch and the style checker seems happy now.
Comment 45 WebKit Commit Bot 2017-12-16 18:54:17 PST
Comment on attachment 329587 [details]
Fixing refs/heads regression

Clearing flags on attachment: 329587

Committed r226009: <https://trac.webkit.org/changeset/226009>
Comment 46 WebKit Commit Bot 2017-12-16 18:54:19 PST
All reviewed patches have been landed.  Closing bug.