Bug 219602 - [webkitscmpy] Support remote GitHub repository
Summary: [webkitscmpy] Support remote GitHub repository
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: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-07 09:21 PST by Jonathan Bedard
Modified: 2020-12-15 15:35 PST (History)
5 users (show)

See Also:


Attachments
Patch (37.90 KB, patch)
2020-12-07 09:29 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (37.98 KB, patch)
2020-12-07 15:58 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (38.22 KB, patch)
2020-12-11 10:19 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (36.16 KB, patch)
2020-12-14 12:10 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch for landing (36.41 KB, patch)
2020-12-15 15:01 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2020-12-07 09:21:15 PST
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.
Comment 1 Radar WebKit Bug Importer 2020-12-07 09:21:31 PST
<rdar://problem/72050026>
Comment 2 Jonathan Bedard 2020-12-07 09:29:29 PST
Created attachment 415559 [details]
Patch
Comment 3 Jonathan Bedard 2020-12-07 15:58:59 PST
Created attachment 415597 [details]
Patch
Comment 4 Stephanie Lewis 2020-12-09 15:33:17 PST
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
Comment 5 Jonathan Bedard 2020-12-11 10:07:49 PST
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:`
Comment 6 Jonathan Bedard 2020-12-11 10:19:56 PST
Created attachment 416006 [details]
Patch
Comment 7 Jonathan Bedard 2020-12-14 12:10:56 PST
Created attachment 416181 [details]
Patch
Comment 8 Stephanie Lewis 2020-12-15 13:45:06 PST
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.
Comment 9 Stephanie Lewis 2020-12-15 13:45:37 PST
Talked over the rest of the patch with Jonathan
Comment 10 Jonathan Bedard 2020-12-15 15:01:24 PST
Created attachment 416297 [details]
Patch for landing
Comment 11 EWS 2020-12-15 15:35:05 PST
Committed r270871: <https://trac.webkit.org/changeset/270871>

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