Currently the export script only adds a Bugzilla comment with the link to the PR created. It would be nice to also add it to the "See also" links, so that it is more easily accessible.
It looks like this would be modifying webkitpy/common/net/bugzilla/bugzilla.py to have a function similar to add_cc_to_bug but for adding new see_also links ; then call it from make_pull_request in webkitpy/w3c/test_exporter.py
It seems https://bugs.webkit.org/show_bug.cgi?id=170996 could be used as a test bug.
Created attachment 345458 [details] Patch which contains the new function for adding PR link to the see also section I simply added a function which will add the link to see_also section
Comment on attachment 345458 [details] Patch which contains the new function for adding PR link to the see also section View in context: https://bugs.webkit.org/attachment.cgi?id=345458&action=review Thanks, that Looks a good start. Before review, can you please: - Run unit tests and add a test for this change? (in Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py I guess) - Create a change log (see https://webkit.org/contributing-code/ and check existing ChangeLog content for examples). > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:854 > + def post_see_also_to_bug(self, bug_id, pr_url, cc=None): I guess you can merge this function into post_comment_to_bug? That way you only submit the form once. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:860 > + self.browser["see_also"] = pr_url What happens when the see_also list already contains some URLs (or even pr_url)? I think it should merge the list similarly to what is done for cc. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:862 > + self.browser["newcc"] = ", ".join(cc) I don't think you need to modify the cc' list here, right? It's already done in post_comment_to_bug.
(In reply to Frédéric Wang (:fredw) from comment #4) > - Run unit tests and add a test for this change? (in > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_unittest.py I guess) Or maybe Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py (use Tools/Scripts/test-webkitpy to run python tests)
Created attachment 345509 [details] New patch with see_aslo and coment in one function with unit tests new patch which has one function for comment and see also the section I also have modified unit tests so!!!
Comment on attachment 345509 [details] New patch with see_aslo and coment in one function with unit tests View in context: https://bugs.webkit.org/attachment.cgi?id=345509&action=review OK that looks better, but it is likely to break current API (see below). I think you still need to set a ChangeLog. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:854 > + def post_comment_and_see_also_to_bug(self, bug_id, comment_text, pr_url, cc=None): A quick search with find Tools/Scripts/ -type f -name '*.py' | xargs grep post_comment_to_bug shows it is called elsewhere, so I guess this change will break some code (maybe you want to run the whole webkitpy test suite). I would recommend to set the function signature to post_comment_to_bug(self, bug_id, comment_text, cc=None, see_also=None) where see_also is an optional list (like cc) that is appended when not None. That way you don't break current API and you keep it generic. > Tools/Scripts/webkitpy/w3c/test_exporter.py:327 > + self._bugzilla.post_comment_and_see_also_to_bug(self._bug_id, "Submitted web-platform-tests pull request: " , WPT_PR_URL + str(pr_number)) Then here it would just be adding the argument see_also=[WPT_PR_URL] > Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:45 > + self.calls.append(pr_url) Again, you'll only need to take the see_also list into account, probably with some text like "Add PR to See Also list" to make the action more explicit.
Comment on attachment 345509 [details] New patch with see_aslo and coment in one function with unit tests r- per comment 7
Created attachment 345693 [details] Made the function signature different added the changelog Made the changes as per the previous review, Modified the function signature, made corresponding unit test changes, Added changelog
Comment on attachment 345458 [details] Patch which contains the new function for adding PR link to the see also section r- per comment 4
Comment on attachment 345693 [details] Made the function signature different added the changelog View in context: https://bugs.webkit.org/attachment.cgi?id=345693&action=review > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:863 > + self.browser["see_also"] = see_also My suggestion was to make the see_also parameter a list of URLs to append (similar to the cc list) so that the API is more generic. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla_mock.py:473 > bug_id, cc, comment_text)) I guess you need to edit _log.info too. > Tools/Scripts/webkitpy/w3c/test_exporter.py:328 > + self._bugzilla.post_comment_to_bug(self._bug_id, "Submitted web-platform-tests pull request: " + pr_url, see_also=pr_url) see_also=[pr_url] > Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:46 > + self.calls.append(see_also) As I said in my previous review, maybe some text description would be better than just the URL.
Comment on attachment 345693 [details] Made the function signature different added the changelog Attachment 345693 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8655788 New failing tests: http/tests/security/canvas-remote-read-remote-video-hls.html
Created attachment 345807 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 346016 [details] More Correct unit test, made see_also as list parameter Modified unit tests for see_also, made see also a list parameter
Attachment 346016 [details] did not pass style-queue: ERROR: Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:46: missing whitespace around operator [pep8/E225] [5] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 346016 [details] More Correct unit test, made see_also as list parameter View in context: https://bugs.webkit.org/attachment.cgi?id=346016&action=review @Darshan: Thanks, the patch now looks good to me, with some minor comment below. @Youenn: Can you please take a look? > Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:46 > + self.calls.append("Append "+ ", ".join(see_also) + " to see also list") Spacing around the first "+" operator is wrong (that's probably what style check bot is complaining about). I know this is not done in this function, but in general you can use the % operator like you did elsewhere in this patch in order to make things more readable: self.calls.append("Append %s to see also list" % ", ".join(see_also)).
Created attachment 346017 [details] corrected style error run the styled test passed it successfully, made string operation more readable
Comment on attachment 346017 [details] corrected style error Attachment 346017 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8694099 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 346044 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 346017 [details] corrected style error View in context: https://bugs.webkit.org/attachment.cgi?id=346017&action=review > Tools/ChangeLog:11 > + (MockBugzilla.post_comment_to_bug): The changelog is not up-to-date. I think it would be worth giving some per-file explanations too.
Comment on attachment 346017 [details] corrected style error View in context: https://bugs.webkit.org/attachment.cgi?id=346017&action=review >> Tools/ChangeLog:11 >> + (MockBugzilla.post_comment_to_bug): > > The changelog is not up-to-date. I think it would be worth giving some per-file explanations too. Agreed
Created attachment 347077 [details] Patch
Comment on attachment 347077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=347077&action=review > Tools/ChangeLog:8 > + Added a optional paramter see_also to post_comment_to_bug and added see_also s/a/an/ and s/paramter/parameter/ The sentence does not seem terminated. Is it something like s/added see_also/made use of see_also new parameter in test_exporter/? > Tools/ChangeLog:11 > + No need for space line. > Tools/ChangeLog:15 > + Ditto.
Created attachment 347078 [details] Patch
Created attachment 347083 [details] Patch
Created attachment 347084 [details] Patch
Comment on attachment 347084 [details] Patch Clearing flags on attachment: 347084 Committed r234856: <https://trac.webkit.org/changeset/234856>
All reviewed patches have been landed. Closing bug.
<rdar://problem/43293584>