Commit queue should post identifiers when it lands changes.
<rdar://problem/75908321>
Created attachment 424424 [details] Patch
Created attachment 424683 [details] Patch
Created attachment 424688 [details] Patch
Created attachment 424691 [details] Patch
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.
Created attachment 424695 [details] Patch
Created attachment 424699 [details] Patch
Created attachment 424702 [details] Patch
Created attachment 424705 [details] Patch (webkitcorepy)
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.
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).
Created attachment 424780 [details] Patch
Created attachment 424782 [details] Patch
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.
Created attachment 424832 [details] Patch
Created attachment 424837 [details] Patch
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()
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)
(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.
Created attachment 424922 [details] Patch
Created attachment 424925 [details] Patch
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.
Created attachment 424932 [details] Patch
Created attachment 424941 [details] Patch
(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
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).
Created attachment 425335 [details] Patch
(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
Another test: https://ews-build.webkit-uat.org/#/builders/26/builds/1716 Bugzilla comment: https://bugs.webkit.org/show_bug.cgi?id=223686#c11
Committed r275613: <https://commits.webkit.org/r275613> All reviewed patches have been landed. Closing bug and clearing flags on attachment 425335 [details].