Bug 202465 – Python 3: Add support in webkitpy.common.net.bugzilla

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

Add webkitpy.common.net.bugzilla to test-webkitpy-python3

[Jonathan Bedard]

Created attachment 380783 [details]
Patch

[EWS Watchlist]

Comment on attachment 380783 [details]
Patch

Attachment 380783 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13121916

New failing tests:
http/tests/is-logged-in/available-in-secure-contexts.https.html
http/tests/websocket/tests/hybi/non-document-mixed-content-blocked-https-with-embedded-http-with-embedded-https.https.html
http/tests/security/navigate-when-restoring-cached-page.html

[EWS Watchlist]

Created attachment 380808 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Dean Johnson]

Comment on attachment 380783 [details]
Patch

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

Looks good other than a few small suggestions. Unofficial r+.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:423
> +            attachment['is_obsolete'] = (element.has_attr('isobsolete') and element['isobsolete'] == "1")

This is probably cleaner (if it works in Python3):
attachment['is_obsolete'] = getattr(element, 'isobsolete', "0") == "1"

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:424
> +            attachment['is_patch'] = (element.has_attr('ispatch') and element['ispatch'] == "1")

Ditto, but with `ispatch`.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:426
> +            attachment['is_obsolete'] = (element.has_key('isobsolete') and element['isobsolete'] == "1")

Ditto.

> Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:427
> +            attachment['is_patch'] = (element.has_key('ispatch') and element['ispatch'] == "1")

Ditto.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:211
> +        if self._install("https://files.pythonhosted.org/packages/86/cd/495c68f0536dcd25f016e006731ba7be72e072280305ec52590012c1e6f2/beautifulsoup4-4.8.1.tar.gz",

Can you assign the value of this to a variable, then check the value for the if check? I think it would read better.

Comment on attachment 380783 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/file_uploader.py:35
> +if sys.version_info > (3, 0):

Python has 3.0 release, do we want to use '>=' here instead?
Also, since we are repeating this a lot, maybe it's easier to make a helper function?

> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:43
> +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup

Is there any specific reason to swap the import order? Or just make it follow the case insensitive alphabetical order?

> Tools/Scripts/webkitpy/thirdparty/BeautifulSoup.py:30
> +            # This hack isn't strictly accurate, but getting lxml to work with the autoinstaller is a non-trivial task

Is this a FIXME then?

> Tools/Scripts/webkitpy/thirdparty/__init__.py:210
> +                             "soupsieve-1.9.4/soupsieve")

Nit: this indentation doesn't match one style used in this file.

> Tools/Scripts/webkitpy/thirdparty/__init__.py:213
> +            self._executive.run_command(['2to3', '-w', self._fs.join(_AUTOINSTALLED_DIR, 'bs4')])

Ditto

r=me according to the fact that no test failure occurrs because of this change for both python 2&3.

[Jonathan Bedard]

Comment on attachment 380783 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/net/file_uploader.py:35
>> +if sys.version_info > (3, 0):
> 
> Python has 3.0 release, do we want to use '>=' here instead?
> Also, since we are repeating this a lot, maybe it's easier to make a helper function?

We discussed this in the last patch, basically, our helper function would not be any shorter, so we're better off using the standard library.

>> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:43
>> +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup
> 
> Is there any specific reason to swap the import order? Or just make it follow the case insensitive alphabetical order?

Just making it follow the case-insensitive alphabetical order.

>> Tools/Scripts/webkitpy/thirdparty/BeautifulSoup.py:30
>> +            # This hack isn't strictly accurate, but getting lxml to work with the autoinstaller is a non-trivial task
> 
> Is this a FIXME then?

Yeah, I can make it a FIXME

[Jonathan Bedard]

Created attachment 380938 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 380938 [details]
Patch for landing

Clearing flags on attachment: 380938

Committed r251112: <https://trac.webkit.org/changeset/251112>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ling Ho]

<rdar://problem/56270611>