[Asterisk-code-review] testsuite: Python 3 compatibility inital commit (testsuite[development/16/python3])

Mon Apr 25 18:30:53 CDT 2022

Attention is currently required from: Michael Bradeen.
Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/testsuite/+/18199 )

Change subject: testsuite: Python 3 compatibility inital commit
......................................................................

Patch Set 7: Code-Review-1

(53 comments)

Patchset:

PS7: 
I think this one patch needs to be broken into several patches based on the relevant changes done:

1) A use SIPp 3pcc instead of pjsua patch
2) A patch for pjsua->pjsua2 changes
3) A python2.7/python3 compatibility patch
4) A debug/info extra logging patch

File contrib/scripts/install_prereq:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/352bd7ed_3d71b6b1 
PS7, Line 18: PACKAGES_DEBIAN="$PACKAGES_DEBIAN python3 python3-pip python3-setuptools python3-dev cython"
            : # Testsuite: basic requirements:
            : PACKAGES_DEBIAN="$PACKAGES_DEBIAN python3-yaml python3-twisted-core python3-lxml liblua5.1-0-dev lua5.1 gdb"
            : # Sipp requirements
            : PACKAGES_DEBIAN="$PACKAGES_DEBIAN libpcap-dev libssl-dev libsctp-dev"
            : # pjsua requirements
            : #PACKAGES_DEBIAN="$PACKAGES_DEBIAN python3-dev"
            : 
            : # Basic build system:
            : PACKAGES_RH="subversion git gcc gcc-c++ patch"
            : # Python tools
            : PACKAGES_RH="$PACKAGES_RH python3 python3-pip python3-setuptools python3-devel Cython"
            : # Testsuite: basic requirements:
            : PACKAGES_RH="$PACKAGES_RH PyYAML python3-twisted-core python3-lxml lua-devel gdb"
            : # Sipp requirements
            : PACKAGES_RH="$PACKAGES_RH libpcap-devel openssl-devel lksctp-tools-devel"
            : # pjsua requirements
            : #PACKAGES_RH="$PACKAGES_RH python3-devel"
I wonder if we should have it check if python is already installed. If so install other libs based on the version, otherwise install python3 like you have here. Thoughts?

File lib/python/asterisk/astdicts.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/615df5c0_33bb9bc9 
PS7, Line 274: #            print("__setitem__ key = ", key, " val = ", val)
Since this began as commented code I'd just remove it vs updating it.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/abb2a976_7cde9dc7 
PS7, Line 278: #        print("inserting key = ", key, " val = ", val)
Same here. This line can be removed altogether.

File lib/python/asterisk/pcap.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/c7e088d7_6bb20008 
PS7, Line 709:                 LOGGER.debug("Packet interpreted")
What's the reason for this debug statement? Seems like this has the potential to spam the debug log. I'd suggest removing it.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/e0cbf02c_d283f78a 
PS7, Line 712:         #interpreted_packet = self._packet_factories.SIPPacketFactory.interpret_packet(packet)
Remove this line

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/c0c76248_8ee4bf0f 
PS7, Line 753:             LOGGER.debug("Interpreted packet is empty")
Same for this one? How needed is it?

File lib/python/asterisk/phones.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/244d122c_56f252da 
PS7, Line 13: import pjsua2 as pj
This changes the dependency. The install_prereq script should also be updated.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/9ae81ef9_be3acb4b 
PS7, Line 13: import pjsua2 as pj
            : from asterisk import pjsua_mod
I feel like most, if not all the changes in this file are in response to the upgrade to pjsua2. Is upgrading to pjsua2 a requirement for, or related to python3? If not I'd think these changes should be separated out and done in another commit.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/2ea2317d_b865a48e 
PS7, Line 55:         LOGGER.info("Phone Account success")
I think this would be more helpful if it was to within below the "if (self.num_accts_created != self.num_accts)" block and the number of accounts created was added.

Also would it make more sense for it to be a debug message?

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/aa38fc39_5bbecb21 
PS7, Line 65:         LOGGER.info("Creating Phones")
should this be a debug vs info?

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/aeca0255_d3a44127 
PS7, Line 394:     LOGGER.info("phone attempting to make call")
seems more like a debug message?

File lib/python/asterisk/pjsua_mod.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/f61abf8f_b6f1ea48 
PS7, Line 18: import pjsua2 as pj
Same question as before. Are the changes here due to moving to python3 or pjsua2? If the latter then separate the changes into another pjsua2 only patch.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/be617531_5d3308d9 
PS7, Line 143:         time.sleep(1)
In a twisted environment the use of a "sleep" is usually undesired. This will pause/halt all twisted processing, which could cause other test timing issues.

If there is a better way to achieve what you want here?

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/e73f83df_d9412231 
PS7, Line 184:         LOGGER.info("ami connect finished!")
Is there a way to also print which AMI connection finished? Could be multiple.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/d5fecaeb_d85ef63c 
PS7, Line 260:         LOGGER.info("worker thread started..")
Seems more of a debug message to me.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/c493f4ff_ba5afa23 
PS7, Line 267:         LOGGER.info('worker thread exited..')
Seems more of a debug message to me.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/2e77e49f_bcbd43ea 
PS7, Line 270:         LOGGER.info("Starting worker thread")
Seems more of a debug message to me.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/656cf103_2ba0d09f 
PS7, Line 345:         LOGGER.info("Account success")
Which account?

File lib/python/asterisk/pluggable_modules.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/5f17c82f_0ff70ab0 
PS7, Line 53:             'nowait': 'False',
I think we can leave the string values alone and just change the field param below. That way configurations don't need to change at all for this.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/cf1c80fb_e909d081 
PS7, Line 127:                                        nowait=self.config['nowait'])
Leave the string 'async' here.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/0255db69_5133ab2f 
PS7, Line 135:                                        nowait=self.config['nowait'])
Leave the string 'async' here.

File lib/python/asterisk/realtime_test_module.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/aff0f80e_f239c029 
PS7, Line 12: import html
I don't think this module is backwards compat with 2.7?

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/9123b3bf_24c13b15 
PS7, Line 14: from wsgiref.util import request_uri
This appears unused, so can be removed.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/bbfb052a_777f4ca9 
PS7, Line 284:         string = '&'.join(['{0}={1}'.format(html.escape(key), html.escape(val))
The html.escape function appears to have been added in version 3.2, so I don't think is backwards compat.

File lib/python/asterisk/test_case.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/9fed91f3_83690476 
PS7, Line 863:         if 'nowait' not in call_details:
             :             call_details['nowait'] = False
Leave these as 'async'

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/97d148e9_a4728146 
PS7, Line 876:                 nowait=call_details['nowait'],
Leave the string value as 'async'

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/72f32789_5e9107d2 
PS7, Line 890:                 nowait=call_details['nowait'],
Same here. Leave string as 'async'

File lib/python/asterisk/test_runner.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/d3b91fde_449c4ad5 
PS7, Line 18: from importlib.machinery import SourceFileLoader
This is causing the CI to fail.

This wasn't introduced until 3.1, and so is no in 2.7. Probably need to wrap in a try/except similar to how the yaml import below is handled, or by some other version distinguishing mechanism.

File lib/python/protocols/ipstack.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/48f43b63_2026e8a9 
PS7, Line 9: #from construct import Struct, HexDump, Switch, Pass, Computed, Hex, Bytes, setGlobalPrintFullStrings
           : #from construct import Struct, HexDump, Switch, Pass, Computed, Hex, Bytes, setglobalfullprinting
Remove commented code

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/ef41d25a_dfdf858f 
PS7, Line 18: #setglobalfullprinting(True)
            : #setGlobalPrintFullStrings(True)
Remove commented code.

Why is setglobalfullprinting being removed?

File lib/python/protocols/layer4/tcp.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/bcd0f3ec_3c842c37 
PS7, Line 9: setGlobalPrintFullStrings(True)
Why is this being removed?

File tests/channels/SIP/sip_blind_transfer/callee_with_reinvite/run-test:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/024792c0_2373ab1e 
PS7, Line 10: from builtins import bytes
builtins 2.7 compat? I think it'll require an install of "futures".

File tests/channels/SIP/sip_blind_transfer/caller_refer_only/run-test:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/8d9a7a77_52a5c48e 
PS7, Line 10: from builtins import bytes
I don't think this is in 2.7

File tests/channels/SIP/sip_blind_transfer/caller_with_reinvite/run-test:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/11711de9_c549a03d 
PS7, Line 10: from builtins import bytes
Here too

File tests/channels/pjsip/rtp/bind_rtp_to_media_address/rtp.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/a51e9c63_94949e5f 
PS7, Line 21: def datagramReceived(self, data, addr):
Hrm, this appears to be a twisted library version difference. I don't believe this will be backwards compatible with whatever twisted python2.7 version is available?

You might be able to "monkey patch" this somehow based on twisted method(s) available.

File tests/channels/pjsip/rtp/rtp_keepalive/base/rtp.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/eae50cf8_917c7b8e 
PS7, Line 28:     def datagramReceived(self, data, addr):
Same problem here. This is a twisted version incompatibility issue. You'll need to figure out a way to make such things backwards compat.

File tests/channels/pjsip/rtp/rtp_keepalive/direct_media/rtp.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/e2c22cc0_9250fbc1 
PS7, Line 19:     def datagramReceived(self, data, addr):
This needs to be backwards compat

File tests/channels/pjsip/subscriptions/rls/rls_test.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/05274117_a49f64b2 
PS7, Line 33:     LOGGER.debug("request line")
            :     LOGGER.debug(packet.request_line)
The addition of debug/info logging should be in a separate patch.

File tests/channels/pjsip/transfers/attended_transfer/nominal/callee_local_app/test-config.yaml:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/b7977e0b_535c81af 
PS7, Line 178:         #- python : pjsua2
Why commented out?

File tests/codecs/audio_analyzer.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/e6cb3737_9d402c26 
PS7, Line 188:                 'nowait': 'True'
This can be left as 'async' if other references change

File tests/hep/hep_capture_node.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/7947dd85_4e88c6ee 
PS7, Line 85:             self.src_addr = socket.inet_ntop(socket.AF_INET, bytes(src_addr.ipv4_addr,"utf-8"))
            :             self.dst_addr = socket.inet_ntop(socket.AF_INET, bytes(dst_addr.ipv4_addr,"utf-8"))
            :         elif self.ip_family == IP_FAMILY.v6:
            :             self.src_addr = socket.inet_ntop(socket.AF_INET6, bytes(src_addr.ipv6_addr,"utf-8"))
            :             self.dst_addr = socket.inet_ntop(socket.AF_INET6, bytes(dst_addr.ipv6_addr,"utf-8"))
"bytes" is not backwards compat

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/e6be7112_05bb8826 
PS7, Line 147:     def datagramReceived(self, data, addr):
Twisted compat issue here too

File tests/rest_api/applications/stasisstatus/test_case.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/d5872f46_f40c3590 
PS7, Line 10: from builtins import StopIteration, super
What are these imports for?

File tests/rest_api/channels/redirect/nominal/run-test:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/0ad2e21a_d8654ca4 
PS7, Line 12: from builtins import range, super
Is this line needed? If so is "super" strictly required?

File tests/rest_api/chunked-transfer/run-test:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/e24bb7c0_cb720787 
PS7, Line 10: from asyncio.log import logger
            : from builtins import bytes, len, list, str
I'm not sure all of this is 2.7 backwards compat. For sure the asyncio stuff.

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/d324afb2_7e08f7a9 
PS7, Line 33: bytes
I don't think this function call is backwards compat

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/c3ac85e3_955d9c40 
PS7, Line 34: #    return url
remove

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/445b28a3_9a6c71c6 
PS7, Line 67: #                data=bytes(chunkizer(b"{ b'variable': b'foo', b'value': b'bar'' }"),"utf-8"),
            : #                data={b'variable': b'foo', b'value': b'bar'},
Remove unused code

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/604b2029_411804ac 
PS7, Line 70:                 auth = (USERNAME, PASSWORD))
Is this a python3 compat change? If so is it backward compat with the library?

File tests/rest_api/request-bodies/run-test:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/4924ec37_ef4f8c3b 
PS7, Line 28:     params = urllib.parse.urlencode({'api_key': 'testsuite:testsuite'})
I don't think this too is 2.7 compat

File tests/rtp/strict_rtp/strict_rtp_seqno/strict_rtp.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/dca4ef89_0254b7a1 
PS7, Line 35:         def datagramReceived(self, data, addr):
Another twisted compat issue here

File tests/rtp/strict_rtp/strict_rtp_yes/strict_rtp.py:

https://gerrit.asterisk.org/c/testsuite/+/18199/comment/fa32df3c_58de8245 
PS7, Line 37:         def datagramReceived(self, data, addr):
Twisted compat issue

-- 
To view, visit https://gerrit.asterisk.org/c/testsuite/+/18199
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: testsuite
Gerrit-Branch: development/16/python3
Gerrit-Change-Id: Ic7a1d72b174df59107370fcb03fae9dc4cdfc9d3
Gerrit-Change-Number: 18199
Gerrit-PatchSet: 7
Gerrit-Owner: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Attention: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Comment-Date: Mon, 25 Apr 2022 23:30:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220425/9a6adc06/attachment-0001.html>