Bug 184914 – Export changes to web-platform-test as part of the webkit-patch upload workflow

• Component: New Bugs
• Status: RESOLVED FIXED

[Brendan McLoughlin]

Export changes to web-platform-test as part of the webkit-patch upload workflow

[Brendan McLoughlin]

Created attachment 338645 [details]
Patch

[Brendan McLoughlin]

Created attachment 338648 [details]
Patch

[Brendan McLoughlin]

Created attachment 338734 [details]
Patch

[Ryosuke Niwa]

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.

[Brendan McLoughlin]

Thanks for the feedback Ryosuke. I was out of town last week but I'm now back and will address your comments today.

[youenn fablet]

> > 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.

[Brendan McLoughlin]

Created attachment 340201 [details]
Patch

[Brendan McLoughlin]

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.

[youenn fablet]

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.

[Brendan McLoughlin]

> 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.

[Brendan McLoughlin]

Created attachment 340348 [details]
Patch

[Brendan McLoughlin]

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?

[youenn fablet]

(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.

[youenn fablet]

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?

[youenn fablet]

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?

[Brendan McLoughlin]

Created attachment 340423 [details]
Patch

[Brendan McLoughlin]

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.

[Brendan McLoughlin]

Created attachment 340424 [details]
Patch

[youenn fablet]

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.

[Brendan McLoughlin]

Created attachment 340579 [details]
Patch

[Brendan McLoughlin]

> 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.

[WebKit Commit Bot]

Comment on attachment 340579 [details]
Patch

Clearing flags on attachment: 340579

Committed r231912: <https://trac.webkit.org/changeset/231912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40338343>

[youenn fablet]

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?

[youenn fablet]

(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.

[Brendan McLoughlin]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 185750

[Brendan McLoughlin]

Created attachment 341510 [details]
Patch

[Brendan McLoughlin]

Created attachment 341514 [details]
Patch

[youenn fablet]

Comment on attachment 341514 [details]
Patch

LGTM.
Is it ready for cq+?

[Brendan McLoughlin]

Thanks Youenn. I think it is ready for cq+.

[WebKit Commit Bot]

Comment on attachment 341514 [details]
Patch

Clearing flags on attachment: 341514

Committed r232274: <https://trac.webkit.org/changeset/232274>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.