Bug 186140 – Enhancement request: Make export-w3c-test-changes add the PR to "See Also" links

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

It seems https://bugs.webkit.org/show_bug.cgi?id=170996 could be used as a test bug.

[darshan]

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

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

(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)

[darshan]

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!!!

[Frédéric Wang (:fredw)]

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.

[Frédéric Wang (:fredw)]

Comment on attachment 345509 [details]
New patch with see_aslo and coment in one function with unit tests

r- per comment 7

[darshan]

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

[Frédéric Wang (:fredw)]

Comment on attachment 345458 [details]
Patch which contains the new function for adding PR link to the see also section

r- per comment 4

[Frédéric Wang (:fredw)]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[darshan]

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

[EWS Watchlist]

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.

[Frédéric Wang (:fredw)]

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)).

[darshan]

Created attachment 346017 [details]
corrected style error

run the styled test passed it successfully, 
made string operation more readable

[EWS Watchlist]

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

[EWS Watchlist]

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

[Frédéric Wang (:fredw)]

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.

[youenn fablet]

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

[darshan]

Created attachment 347077 [details]
Patch

[youenn fablet]

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.

[darshan]

Created attachment 347078 [details]
Patch

[darshan]

Created attachment 347083 [details]
Patch

[darshan]

Created attachment 347084 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 347084 [details]
Patch

Clearing flags on attachment: 347084

Committed r234856: <https://trac.webkit.org/changeset/234856>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43293584>