Bug 184914 - Export changes to web-platform-test as part of the webkit-patch upload workflow
Summary: Export changes to web-platform-test as part of the webkit-patch upload workflow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 185750
Blocks:
  Show dependency treegraph
 
Reported: 2018-04-24 06:31 PDT by Brendan McLoughlin
Modified: 2018-05-29 14:30 PDT (History)
14 users (show)

See Also:


Attachments
Patch (23.26 KB, patch)
2018-04-24 06:46 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (23.26 KB, patch)
2018-04-24 07:07 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (23.25 KB, patch)
2018-04-25 07:30 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (23.09 KB, patch)
2018-05-11 10:47 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (24.06 KB, patch)
2018-05-14 13:26 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (26.79 KB, patch)
2018-05-15 11:30 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (27.05 KB, patch)
2018-05-15 11:58 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (27.01 KB, patch)
2018-05-17 07:01 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (31.53 KB, patch)
2018-05-29 13:38 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff
Patch (32.68 KB, patch)
2018-05-29 13:47 PDT, Brendan McLoughlin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brendan McLoughlin 2018-04-24 06:31:33 PDT
Export changes to web-platform-test as part of the webkit-patch upload workflow
Comment 1 Brendan McLoughlin 2018-04-24 06:46:39 PDT
Created attachment 338645 [details]
Patch
Comment 2 Brendan McLoughlin 2018-04-24 07:07:53 PDT
Created attachment 338648 [details]
Patch
Comment 3 Brendan McLoughlin 2018-04-25 07:30:46 PDT
Created attachment 338734 [details]
Patch
Comment 4 Ryosuke Niwa 2018-04-25 23:42:48 PDT
Comment on attachment 338734 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/commands/upload.py:289
> +        steps.W3CTestExport,

We should rename this to WPTChangeExport since this is all about web-platform-tests,
and doesn't apply to other W3C test repositories.

> Tools/Scripts/webkitpy/tool/steps/w3ctestexport.py:46
> +        args = []

The initial declaration of this variable should contain --bug, and other options that area always specified
instead of adding each one separately down below.

> Tools/Scripts/webkitpy/tool/steps/w3ctestexport.py:52
> +        bug_id = state.get("bug_id")
> +        if not bug_id:

Please check this condition first.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:53
> +    """the W3cChangeset class is responsible for deteching changes in
> +    the web-platform-test directory and generating patches exporting
> +    those changes.
> +    """

We don't add these comments. If we find ourselves needing to a comment like this,
it's a good indication that the class name isn't descriptive enough. Please remove the comment.
I would call this class WebPlatformTestPatchGenerator instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:67
> +        git_commit = "HEAD...." if not self._options.git_commit else self._options.git_commit + "~1.." + self._options.git_commit
> +        patch_data = self._host.scm().create_patch(git_commit, [WEBKIT_WPT_DIR])

This assumes scm is a git. This is simply not true for many WebKit contributors.
r- because of this. Use scm.create_patch instead.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:73
> +        patch_file = './patch.temp.' + str(time.clock())

Please create a temporary file in a temporary directory via self._filesystem.open_binary_tempfile
r- because of this as well.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:139
> +        """
> +        Ask the user to provide a username and token if the
> +        --interactive flag is passed and username and token
> +        are not already set.
> +        """

This comment repeats the code below. Please remove.
In general, we don't add these kinds of "what" comment in WebKit.
We only add "why" comments.
r- due to this and the earlier class-level "what" comments.
Also, please use https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/system/user.py

> Tools/Scripts/webkitpy/w3c/test_exporter.py:142
> +            self._username = self._git.local_config('github.username').rstrip()

Please use https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/net/credentials.py
and read username/password from KeyChain as well.
Comment 5 Brendan McLoughlin 2018-05-07 05:37:55 PDT
Thanks for the feedback Ryosuke. I was out of town last week but I'm now back and will address your comments today.
Comment 6 youenn fablet 2018-05-10 10:47:55 PDT
> > Tools/Scripts/webkitpy/w3c/test_exporter.py:67
> > +        git_commit = "HEAD...." if not self._options.git_commit else self._options.git_commit + "~1.." + self._options.git_commit
> > +        patch_data = self._host.scm().create_patch(git_commit, [WEBKIT_WPT_DIR])
> 
> This assumes scm is a git. This is simply not true for many WebKit
> contributors.
> r- because of this. Use scm.create_patch instead.

This code is already using scm.create_patch so it should work with SVN (although I never tested it).

> > Tools/Scripts/webkitpy/w3c/test_exporter.py:142
> > +            self._username = self._git.local_config('github.username').rstrip()
> 
> Please use
> https://trac.webkit.org/browser/webkit/trunk/Tools/Scripts/webkitpy/common/
> net/credentials.py
> and read username/password from KeyChain as well.

That would indeed be good to do.
That said, I would separate the integration refactoring from adding additional support like reading from KeyChain.
Seems fine to do those in parallel.
Comment 7 Brendan McLoughlin 2018-05-11 10:47:43 PDT
Created attachment 340201 [details]
Patch
Comment 8 Brendan McLoughlin 2018-05-11 10:56:36 PDT
I've updated this patch based on the provided feedback. I'm going to take Youenn's suggestion and make a separate patch to move storing the github username and token into the KeyChain.
Comment 9 youenn fablet 2018-05-14 07:38:28 PDT
Comment on attachment 340201 [details]
Patch

Looks almost ready to go for me.
A couple of neats and some questions below.
Btw, I created a test bugzilla entry on https://bugs.webkit.org/show_bug.cgi?id=185610.
It worked like a charm.

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

> Tools/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

That seems really nice.
I wonder whether we should provide the possibility to just create a remote branch but not create the PR right away.
The main advantage I see is that in that case, you do not have to provide the OAuth token.
Maybe, if the OAuth token is not provided, we could still create the remote branch?

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:1
> +# Copyright (C) 2018 Apple Inc. All rights reserved.

I think you have the right to put your own copyright.

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:13
> +#     * Neither the name of Google Inc. nor the names of its

Probably Apple Inc. here instead.
Also we now use a two clause license like in test_exporter.py.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:128
> +    def _ensure_username_and_token(self, options):

Can we add a FIXME to use the keychain?

> Tools/Scripts/webkitpy/w3c/test_exporter.py:362
> +    parser.add_argument('-i', '--interactive', dest='interactive_mode', action='store_true', default=False, help='Prompts the user for their github credentials and asks for confirmation before exporting the changes.')

Would it make sense to set the default value to true, or to add a no-interactive mode?

I also wonder whether it would make sense to ask user whether it is fine to store the credentials.
I think we might want to do that once we will be using the key chain.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:395
> +    host = host or Host()

Could be inlined maybe.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:402
> +        _log.info('No changes to upstream. Exiting...')

This is only style but we usually do early return.
Something like:
if not wpt_patch_generator.has_wpt_changes():
    if not silence_noop:
        log.info('No changes to upstream. Exiting...')
    return;

test_exporter = ...

Maybe silent_noop should be inverted to be something like log_if_nowptchange.
Comment 10 Brendan McLoughlin 2018-05-14 07:46:45 PDT
> I wonder whether we should provide the possibility to just create a remote branch but not create the PR right away.
> The main advantage I see is that in that case, you do not have to provide the OAuth token.
> Maybe, if the OAuth token is not provided, we could still create the remote branch?

Sounds like a good idea. I'll try it out.

> Can we add a FIXME to use the keychain?

Will do.

> Would it make sense to set the default value to true, or to add a no-interactive mode?

I was originally trying to avoid changing the existing behavior but a no-interactive seems like a better option.


> This is only style but we usually do early return.

I'll update this to use the early return.


> Maybe silent_noop should be inverted to be something like log_if_nowptchange.
I like the log_if_nowptchange suggestion. I'll make that change too.
Comment 11 Brendan McLoughlin 2018-05-14 13:26:02 PDT
Created attachment 340348 [details]
Patch
Comment 12 Brendan McLoughlin 2018-05-14 13:49:10 PDT
I've updated the workflow so it now prompts the user to check if they would like to export the patch before asking them for their github username/token. If a user doesn't provide a token when prompted it creates the remote fork but does not attempt to open a pull request. Youenn, should I make the prompt for the token be more upfront about this behavior?
Comment 13 youenn fablet 2018-05-15 00:11:48 PDT
(In reply to Brendan McLoughlin from comment #12)
> I've updated the workflow so it now prompts the user to check if they would
> like to export the patch before asking them for their github username/token.

Sounds good.

> If a user doesn't provide a token when prompted it creates the remote fork
> but does not attempt to open a pull request. Youenn, should I make the
> prompt for the token be more upfront about this behavior?

I'll try the new workflow later today but this seems fine to me, as long as it is clear that not providing the OAuth token will remove the ability to create the PR.
Comment 14 youenn fablet 2018-05-15 00:17:51 PDT
Comment on attachment 340348 [details]
Patch

Some minor comments.
I'll try the workflow later today before r+ing it.

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

> Tools/Scripts/webkitpy/tool/commands/upload.py:539
> +    help_text = "Opens a pull request to sync any changes in the LayoutTests/imported/w3c/web-platform-tests directory"

s/sync/synchronize

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:51
> +        message = 'web-platform-tests changes detected. Would you like to create a pull-request to the WPT github repo now?'

Maybe rephrase to something like: "Would you like to export the changes and/or create a PR to the WPT GitHub repository?:

> Tools/Scripts/webkitpy/w3c/test_exporter.py:125
> +        return self._host.user.prompt_password('Enter github OAuth token: ')

s/github/GitHub/
Maybe make it more explicit that the token is for the creation of the PR.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:130
> +        return self._host.user.prompt('Enter github username: ')

s/github/your GitHub/

> Tools/Scripts/webkitpy/w3c/test_exporter.py:250
> +            _log.info('Skipping pull request because OAuth token was not provided. You can open the pull request manually using the branch ' + self.wpt_fork_branch_github_url)

OK, message seems clear to me.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:399
> +    wpt_patch_generator = WebPlatformTestPatchGenerator(host, options)

Might not be worth creating twice the same wpt_patch_generator.
Maybe there is a way to improve this?
Comment 15 youenn fablet 2018-05-15 00:17:53 PDT
Comment on attachment 340348 [details]
Patch

Some minor comments.
I'll try the workflow later today before r+ing it.

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

> Tools/Scripts/webkitpy/tool/commands/upload.py:539
> +    help_text = "Opens a pull request to sync any changes in the LayoutTests/imported/w3c/web-platform-tests directory"

s/sync/synchronize

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:51
> +        message = 'web-platform-tests changes detected. Would you like to create a pull-request to the WPT github repo now?'

Maybe rephrase to something like: "Would you like to export the changes and/or create a PR to the WPT GitHub repository?:

> Tools/Scripts/webkitpy/w3c/test_exporter.py:125
> +        return self._host.user.prompt_password('Enter github OAuth token: ')

s/github/GitHub/
Maybe make it more explicit that the token is for the creation of the PR.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:130
> +        return self._host.user.prompt('Enter github username: ')

s/github/your GitHub/

> Tools/Scripts/webkitpy/w3c/test_exporter.py:250
> +            _log.info('Skipping pull request because OAuth token was not provided. You can open the pull request manually using the branch ' + self.wpt_fork_branch_github_url)

OK, message seems clear to me.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:399
> +    wpt_patch_generator = WebPlatformTestPatchGenerator(host, options)

Might not be worth creating twice the same wpt_patch_generator.
Maybe there is a way to improve this?
Comment 16 Brendan McLoughlin 2018-05-15 11:30:37 PDT
Created attachment 340423 [details]
Patch
Comment 17 Brendan McLoughlin 2018-05-15 11:39:35 PDT
I updated the wording of most of the messaging based on your feedback Youenn. 

I also refactored the TestExporter class to lazily instantiate many of
the properties that have side effects. This made it possible to get
rid of the WebPlatformTestPatchGenerator class because I originally
introduced that class to be able to check if there were WPT changes
before instantiating the TestExporter class with the side effect of
prompting the user for their github username / token and cloning the
web-platform-test github repo.
Comment 18 Brendan McLoughlin 2018-05-15 11:58:14 PDT
Created attachment 340424 [details]
Patch
Comment 19 youenn fablet 2018-05-16 16:10:31 PDT
Comment on attachment 340424 [details]
Patch

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

> Tools/Scripts/webkitpy/tool/steps/wptchangeexport.py:52
> +        message = 'Would you like to export the web-platform-tests changes and/or create a PR to the WPT GitHub repository?'

Sorry to change my mind.
I think that we should phrase it as: "Would you like to export the web-platform-tests changes to a WPT GitHub repository?"
" and/or ..." is not very clear.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:140
> +        return bool(self._create_patch())

We will probably create twice the patch, once when we will check for changes and once when writing the git patch file.
Maybe we should store the patch data in TestExporter.

> Tools/Scripts/webkitpy/w3c/test_exporter.py:195
> +        # validate token and username

Comment is probably not needed.
Comment 20 Brendan McLoughlin 2018-05-17 07:01:08 PDT
Created attachment 340579 [details]
Patch
Comment 21 Brendan McLoughlin 2018-05-17 07:05:43 PDT
> I think that we should phrase it as: "Would you like to export the web-platform-tests changes to a WPT GitHub repository?"
" and/or ..." is not very clear.

I've updated this line to use your suggestion.

> We will probably create twice the patch, once when we will check for changes and once when writing the git patch file.
> Maybe we should store the patch data in TestExporter.

I changed this function into a property with @memoized so the result gets cached the first time it is called.
Comment 22 WebKit Commit Bot 2018-05-17 11:29:06 PDT
Comment on attachment 340579 [details]
Patch

Clearing flags on attachment: 340579

Committed r231912: <https://trac.webkit.org/changeset/231912>
Comment 23 WebKit Commit Bot 2018-05-17 11:29:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2018-05-17 11:30:33 PDT
<rdar://problem/40338343>
Comment 25 youenn fablet 2018-05-17 14:34:16 PDT
Brendan, I updated my build and did changes to -expected.txt files in LayoutTests/imported/w3c/web-platform-tests.
It was then suggesting me to export the changes to WPT GitHub, while it should not.
Can you look at this?
Comment 26 youenn fablet 2018-05-17 14:39:00 PDT
(In reply to youenn fablet from comment #25)
> Brendan, I updated my build and did changes to -expected.txt files in
> LayoutTests/imported/w3c/web-platform-tests.
> It was then suggesting me to export the changes to WPT GitHub, while it
> should not.
> Can you look at this?

The issue is that we are currently filtering out expected.txt files at the time we do apply_mail_patch.
Maybe we could do it at time of creation of the patch instead.
Or we could update our heuristic to check for lines starting with diff and without '-expected.txt' in it.

If it is difficult to fix it quickly, we can roll the patch out.
Comment 27 Brendan McLoughlin 2018-05-17 14:47:15 PDT
I will look into filtering out expected files when the patch is created. If I think it's going to take longer then an hour I'll let you know so we can back out this change.
Comment 28 WebKit Commit Bot 2018-05-17 15:10:56 PDT
Re-opened since this is blocked by bug 185750
Comment 29 Brendan McLoughlin 2018-05-29 13:38:20 PDT
Created attachment 341510 [details]
Patch
Comment 30 Brendan McLoughlin 2018-05-29 13:47:54 PDT
Created attachment 341514 [details]
Patch
Comment 31 youenn fablet 2018-05-29 13:52:04 PDT
Comment on attachment 341514 [details]
Patch

LGTM.
Is it ready for cq+?
Comment 32 Brendan McLoughlin 2018-05-29 14:01:14 PDT
Thanks Youenn. I think it is ready for cq+.
Comment 33 WebKit Commit Bot 2018-05-29 14:30:03 PDT
Comment on attachment 341514 [details]
Patch

Clearing flags on attachment: 341514

Committed r232274: <https://trac.webkit.org/changeset/232274>
Comment 34 WebKit Commit Bot 2018-05-29 14:30:05 PDT
All reviewed patches have been landed.  Closing bug.