[Asterisk-code-review] lib/python/asterisk/pcap/py Replace construct_legacy.protocols (testsuite[13])
George Joseph
asteriskteam at digium.com
Tue Nov 19 08:21:13 CST 2019
George Joseph has posted comments on this change. ( https://gerrit.asterisk.org/c/testsuite/+/13222 )
Change subject: lib/python/asterisk/pcap/py Replace construct_legacy.protocols
......................................................................
Patch Set 1:
(20 comments)
> Patch Set 1: Code-Review-1
>
> (34 comments)
>
> Since you ported most things to contructs 2.9 would it make sense to submit those changes upstream to that project for inclusion there? Then we can eventually remove it from our code base.
They actually deleted the entire protocols module after 2.5.5. There's nothing to send upstream.
https://gerrit.asterisk.org/c/testsuite/+/13222/1//COMMIT_MSG
Commit Message:
https://gerrit.asterisk.org/c/testsuite/+/13222/1//COMMIT_MSG@30
PS1, Line 30: directle
> s/directle/directly
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py
File lib/python/asterisk/pcap.py:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@63
PS1, Line 63: # if (module_config.get('register-observer')):
: #
> Did you mean to leave this commented? No tests use it in their configs? If so the commented code can […]
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@69
PS1, Line 69:
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@119
PS1, Line 119:
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@166
PS1, Line 166:
> more whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@734
PS1, Line 734: # self.packet_factory.create_factory(RTPPacketFactory)
: # self.packet_factory.create_factory(RTCPPacketFactory)
> Can remove commented code?
Heh, no. I should have uncommented it. :)
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@875
PS1, Line 875: # if not 'register-observer' in module_config:
: # raise Exception('VOIPListener needs register-observer to be set')
:
: VOIPSniffer.__init__(self, module_config, test_object)
:
: packet_type = module_config.get("packet-type")
: bpf = module_config.get("bpf-filter")
:
: if packet_type:
: self.add_callback(packet_type, module_config.get("callback"))
:
> Commented code that can be removed? And 4 items of whitespace.
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@903
PS1, Line 903: # This is a unit test for capture and parsing.
> This unit test should be moved into its own file and put under the lib/python/asterisk/self_test director
The unit test isn't standalone. You need to create network activity for it to run and you need to examine the results. Maybe at a later date we can create a sipp scenario or a full blown test for it.
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/pcap.py@905
PS1, Line 905: # UDP port 5060.
> eol whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/test_case.py
File lib/python/asterisk/test_case.py:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/asterisk/test_case.py@688
PS1, Line 688: # It _appears_ no tests use this method but we'll throw an exception
: # to catch tests we might have missed.
:
: # raise NotImplementedError()
> looks like this can all be removed. […]
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/README.md
File lib/python/protocols/README.md:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/README.md@6
PS1, Line 6: need that functionality.
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/README.md@10
PS1, Line 10: module but was removed after v2.5.5. Unfortunately,
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/README.md@12
PS1, Line 12: module available.
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/README.md@35
PS1, Line 35: # PYTHONPATH=/usr/src/asterisk/testsuite/lib/python python3
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/layer4/tcp.py
File lib/python/protocols/layer4/tcp.py:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/layer4/tcp.py@10
PS1, Line 10:
> whitespace
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/layer4/tcp.py@16
PS1, Line 16: "header_length" / ExprAdapter(Nibble,
> whitespace at eol
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/layer4/udp.py
File lib/python/protocols/layer4/udp.py:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/lib/python/protocols/layer4/udp.py@14
PS1, Line 14: "payload_length" / ExprAdapter(Int16ub,
> whitespace at eol
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/tests/channels/SIP/pcap_demo/run-test
File tests/channels/SIP/pcap_demo/run-test:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/tests/channels/SIP/pcap_demo/run-test@12
PS1, Line 12: #try:
> remove commented code.
Done
https://gerrit.asterisk.org/c/testsuite/+/13222/1/tests/channels/SIP/pcap_demo/test-config.yaml
File tests/channels/SIP/pcap_demo/test-config.yaml:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/tests/channels/SIP/pcap_demo/test-config.yaml@12
PS1, Line 12: # skip: True
> This can be deleted from the file vs commented.
Actually this test was already skipped. I commented it out to test it. It hasn't worked in a long time.
https://gerrit.asterisk.org/c/testsuite/+/13222/1/tests/channels/pjsip/rtp/rtp_keepalive/base/rtp.py
File tests/channels/pjsip/rtp/rtp_keepalive/base/rtp.py:
https://gerrit.asterisk.org/c/testsuite/+/13222/1/tests/channels/pjsip/rtp/rtp_keepalive/base/rtp.py@31
PS1, Line 31: header = 'rtp_packet' / BitStruct(
: 'header' / Struct(
: 'version' / BitsInteger(2),
: 'padding' / Bit,
: 'extension' / Bit,
: 'csrc_count' / Nibble,
: 'marker' / Bit,
: 'payload' / BitsInteger(7)
: ),
: 'sequence_number' / Bytewise(Int16ub),
: 'timestamp' / Bytewise(Int32ub),
: 'ssrc' / Bytewise(Int32ub)
: )
> This is pretty much the same as what's in pcap.py. […]
Done
--
To view, visit https://gerrit.asterisk.org/c/testsuite/+/13222
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: testsuite
Gerrit-Branch: 13
Gerrit-Change-Id: Id38d01a2cd073b240fde909a38c95d69313bbbe7
Gerrit-Change-Number: 13222
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Tue, 19 Nov 2019 14:21:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20191119/90779c24/attachment.html>
More information about the asterisk-code-review
mailing list