Bug 219602 – [webkitscmpy] Support remote GitHub repository

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Much like we have a remote Subversion repository, we also need a remote GitHub repository so that commit information can be retrieved without the presence of a checkout.

[Radar WebKit Bug Importer]

<rdar://problem/72050026>

[Jonathan Bedard]

Created attachment 415559 [details]
Patch

[Jonathan Bedard]

Created attachment 415597 [details]
Patch

[Stephanie Lewis]

Comment on attachment 415597 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/scm_base.py:40
> +    GIT_SVN_REVISION = re.compile(r'git-svn-id: \S+:\/\/.+@(?P<revision>\d+) .+-.+-.+-.+')

nit: I'd have added regex or pattern to these names - it's not the revision is the pattern to match against to find the revision

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/remote/git_hub.py:43
> +        self.api_remote = 'api.{}/repos/{}'.format(

nit: with the multiple values and the splitting/splciing I'd use named variables for the {}

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/remote/git_hub.py:50
> +        super(GitHub, self).__init__(self.remote.split('/')[0], self.api_remote.split('/')[0])

nit: you're doing this splicing multiple times, maybe use a local variable, should also make the code more clear about what part you're passing around?

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/remote/git_hub.py:54
> +        self.commits = {

it's weird to me that the commits and branches are hard coded in the git mock object.  I think it also places limits on what can be tested.  I think it would make more sense to store that kind of information in a config file and load it into the git mock.  Especially since this data is nowhere near the tests themselves.  I feel like the unit tests should be a bit more self-contained for clarity

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/remote/git_hub.py:152
> +        if not url.startswith('http://') and not url.startswith('https://'):

same as with branches and commits, hardcoding the responses rather than taking in a list of dictionary urls and responses seems like it will limit what we can test

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:69
> +        url = '{}/repos/{}/{}{}'.format(self.api_url, self.owner, self.name, '/{}'.format(path) if path else '')

nit: with so many substitutions nice to use named options

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:221
> +            # A commit not on any branches cannot have an identifier

are you including main in that comment?  From what I can tell you're setting identifiers on the main branch to None so you can recompute? If that's true this comment doesn't make much sense

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/git_unittest.py:254
> +        self.assertEqual(remote.GitHub.is_webserver('https://svn.webkit.org/repository/webkit'), False)

nit: test cases should probably not refer to real things

[Jonathan Bedard]

Comment on attachment 415597 [details]
Patch

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

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/mocks/remote/git_hub.py:152
>> +        if not url.startswith('http://') and not url.startswith('https://'):
> 
> same as with branches and commits, hardcoding the responses rather than taking in a list of dictionary urls and responses seems like it will limit what we can test

This part isn't really hard-coded, it's derived from the commits. We're already using the more general "define the responses your mocking" API, this is just a stubbed implementation of GitHub's API. It's a bit more advanced than hardcoding a set of responses, but not by much. Most of these endpoints are basically "find the commit matching the provided ref, dump it out as json/html"

>> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:221
>> +            # A commit not on any branches cannot have an identifier
> 
> are you including main in that comment?  From what I can tell you're setting identifiers on the main branch to None so you can recompute? If that's true this comment doesn't make much sense

Oops! The comment is correct, the code is incorrect. Line 233 should be `elif branch:`

[Jonathan Bedard]

Created attachment 416006 [details]
Patch

[Jonathan Bedard]

Created attachment 416181 [details]
Patch

[Stephanie Lewis]

Comment on attachment 416181 [details]
Patch

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

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:84
> +                return result

you didn't set the result to the error?  Did you want to return the good data and throw away the error?  I tend to regret throwing away requests errors

> Tools/Scripts/libraries/webkitscmpy/webkitscmpy/remote/git_hub.py:88
> +    def _count(self, ref=None):

name should reflect that its returning the count and the hash.

[Stephanie Lewis]

Talked over the rest of the patch with Jonathan

[Jonathan Bedard]

Created attachment 416297 [details]
Patch for landing

[EWS]

Committed r270871: <https://trac.webkit.org/changeset/270871>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416297 [details].