Bug 223829 – [ews] Commit queue should post the identifier

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Commit queue should post identifiers when it lands changes.

[Radar WebKit Bug Importer]

<rdar://problem/75908321>

[Jonathan Bedard]

Created attachment 424424 [details]
Patch

[Jonathan Bedard]

Created attachment 424683 [details]
Patch

[Jonathan Bedard]

Created attachment 424688 [details]
Patch

[Jonathan Bedard]

Created attachment 424691 [details]
Patch

[Aakash Jain]

Comment on attachment 424691 [details]
Patch

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

> Tools/CISupport/ews-build/steps.py:3251
> +        if response.status_code == 200:

we should handle the else case gracefully (since there might be a network/infrastructure issue with the other server) and revert back to original behavior of posting svn revision urls.

> Tools/CISupport/ews-build/steps.py:3254
> +        comment = 'Committed r{} ({}): <{}>'.format(svn_revision, ref if '@' in ref else '?', self.url_for_ref(ref))

can we make it multi-line statement for clarify, instead of using embedded single line if else.

[Jonathan Bedard]

Created attachment 424695 [details]
Patch

[Jonathan Bedard]

Created attachment 424699 [details]
Patch

[Jonathan Bedard]

Created attachment 424702 [details]
Patch

[Jonathan Bedard]

Created attachment 424705 [details]
Patch (webkitcorepy)

[Jonathan Bedard]

The first change just mocks requests.get explicitly. The second utilizes the more complete mocking system provided by webkitcorepy. The issue with using this system is that our buildbot unit tests don't have a dependency on webkitcorepy yet, and it's unclear that we want to add that dependency.

[Aakash Jain]

Comment on attachment 424699 [details]
Patch

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

> Tools/CISupport/ews-build/steps.py:3251
> +        if response.status_code == 200:

Need to handle the case when response is None.

> Tools/CISupport/ews-build/steps.py:3252
> +            ref = response.json().get('identifier', ref)

Need to gracefully handle the case when response.json() returns an exception.

Better to do all this network handling in a separate method, e.g.: get_commit_identifier_for_revision()

> Tools/CISupport/ews-build/steps.py:3254
> +        id_string = ref if '@' in ref else '?'

Having a invalid url when commit-identifier is not available seems like a regression. Better to have svn revision url when commit identifier is not available.

> Tools/CISupport/ews-build/steps_unittest.py:3978
> +                self.status_code = 200

we should also add unit-tests to test non-200 status codes. Probably a good idea to make this status code an argument to init (with default value of 200).

[Jonathan Bedard]

Created attachment 424780 [details]
Patch

[Jonathan Bedard]

Created attachment 424782 [details]
Patch

[Aakash Jain]

Comment on attachment 424782 [details]
Patch

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

> Tools/CISupport/ews-build/steps.py:3271
> +    def url_for_ref(self, ref, json=False):

This method is little confusing, can we have two separate methods to generate commit identifier url and svn revision info url, something like: url_for_commit_identifer() and url_for_svn_revision_info()

Also, when we remove svn support, we can then simply delete the corresponding svn method.

> Tools/CISupport/ews-build/steps.py:3274
> +    def identifier_for_ref(self, ref):

can we rename it to identifier_for_svn_revision, and rename ref to svn_revision here for clarity.

> Tools/CISupport/ews-build/steps.py:3279
> +        except (requests.exceptions.ConnectionError, json.decoder.JSONDecodeError):

Let's do generic except here to catch any exception here. I know in general we avoid that so as to know the specific expectation and properly handle that, but this is not the place to take risk. Having any unexpected exception here is too bad, we would have already committed to the repo, but the build would be marked as "exception" and patch would still have cq+ flag and no comment/indication on it.

Also, let's add a print statement here so that server logs have some indication of this error.

> Tools/CISupport/ews-build/steps.py:3291
> +        id_string = ref if '@' in ref else '?'

can we rename id_string to commit_identifier, because that's what it is.

[Jonathan Bedard]

Created attachment 424832 [details]
Patch

[Jonathan Bedard]

Created attachment 424837 [details]
Patch

[Aakash Jain]

Comment on attachment 424837 [details]
Patch

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

> Tools/CISupport/ews-build/steps_unittest.py:4121
>          self.expectOutcome(result=SUCCESS, state_string='Committed r256729')

The build and step summary should also be updated to use/include Commit Identifier.
build summary is set using build_summary property which is set in PushCommitToWebKitRepo::evaluateCommand()

[Aakash Jain]

Comment on attachment 424837 [details]
Patch

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

> Tools/CISupport/ews-build/steps_unittest.py:4124
> +        self.assertEqual(self.getProperty('bugzilla_comment_text'), 'Committed r256729 (220797@main): <https://commits.webkit.org/220797@main>\n\nAll reviewed patches have been landed. Closing bug and clearing flags on attachment 1234.')

Should we use
Committed r256729 (220797@main)
or
Committed 220797@main (r256729)

[Jonathan Bedard]

(In reply to Aakash Jain from comment #19)
> Comment on attachment 424837 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424837&action=review
> 
> > Tools/CISupport/ews-build/steps_unittest.py:4124
> > +        self.assertEqual(self.getProperty('bugzilla_comment_text'), 'Committed r256729 (220797@main): <https://commits.webkit.org/220797@main>\n\nAll reviewed patches have been landed. Closing bug and clearing flags on attachment 1234.')
> 
> Should we use
> Committed r256729 (220797@main)
> or
> Committed 220797@main (r256729)

Dean and I discussed this a few weeks back, we're matching what webkit-patch land does.

[Jonathan Bedard]

Created attachment 424922 [details]
Patch

[Jonathan Bedard]

Created attachment 424925 [details]
Patch

[Aakash Jain]

Comment on attachment 424925 [details]
Patch

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

> Tools/CISupport/ews-build/steps.py:3290
> +            response = requests.get('{}/json'.format(self.url_for_revision(revision)))

would be a good idea to include a timeout in this requests, say 1 or 2 minutes, or ensure that this doesn't get stuck forever in case of network issue.

Also, can consider separating url in a separate line, e.g.: url = '{}/json'.format(self.url_for_revision(revision))

> Tools/CISupport/ews-build/steps.py:3291
> +            if response and response.status_code == 200:

we should have a print statement for else case, which prints an error message and status code to the logs.

> Tools/CISupport/ews-build/steps.py:3304
> +        identifier = identifier if identifier and '@' in identifier else '?'

In case we can't find identifier, I wonder if we should print "(?)", or just not include it at all.

[Jonathan Bedard]

Created attachment 424932 [details]
Patch

[Jonathan Bedard]

Created attachment 424941 [details]
Patch

[Jonathan Bedard]

(In reply to Aakash Jain from comment #23)
> Comment on attachment 424925 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424925&action=review
> 
> > Tools/CISupport/ews-build/steps.py:3290
> > +            response = requests.get('{}/json'.format(self.url_for_revision(revision)))
> 
> would be a good idea to include a timeout in this requests, say 1 or 2
> minutes, or ensure that this doesn't get stuck forever in case of network
> issue.
> 
> Also, can consider separating url in a separate line, e.g.: url =
> '{}/json'.format(self.url_for_revision(revision))
> 
> > Tools/CISupport/ews-build/steps.py:3291
> > +            if response and response.status_code == 200:
> 
> we should have a print statement for else case, which prints an error
> message and status code to the logs.
> 
> > Tools/CISupport/ews-build/steps.py:3304
> > +        identifier = identifier if identifier and '@' in identifier else '?'
> 
> In case we can't find identifier, I wonder if we should print "(?)", or just
> not include it at all.

I think we should print "(?)". Otherwise folks are unlikely to notice something is broken, because the pure-revision representation looks reasonable

[Aakash Jain]

Comment on attachment 424941 [details]
Patch

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

> Tools/CISupport/ews-build/steps.py:3267
> +            self.setProperty('build_summary', 'Committed {}'.format(identifier))

Nit: can use commit_summary variable here.

> Tools/CISupport/ews-build/steps.py:3282
>      def url_for_revision(self, revision):

Seems like this method is used only once, to create another url (for json). We can rename this method to something like url_for_revision_details() and include /json in the returned url.

> Tools/CISupport/ews-build/steps.py:3308
> +        str_if_identifier = identifier if identifier and '@' in identifier else '?'

Nit: did you mean 'str_of_identifier'? or maybe identifier_str.

> Tools/CISupport/ews-build/steps_unittest.py:4126
> +            self.assertEqual(self.getProperty('build_finish_summary'), None)

let's test build_summary property as well here (in all these unit-tests).

[Jonathan Bedard]

Created attachment 425335 [details]
Patch

[Aakash Jain]

(In reply to Jonathan Bedard from comment #28)
> Created attachment 425335 [details]
Tested in https://ews-build.webkit-uat.org/#/builders/26/builds/1715
Bugzilla comment: https://bugs.webkit.org/show_bug.cgi?id=223606#c6

[Aakash Jain]

Another test: https://ews-build.webkit-uat.org/#/builders/26/builds/1716
Bugzilla comment: https://bugs.webkit.org/show_bug.cgi?id=223686#c11

[EWS]

Committed r275613: <https://commits.webkit.org/r275613>

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