[asterisk-dev] Change in testsuite[master]: Add a test for PJSIP t38 with authentication based on normal...

[Matt Jordan (Code Review)]

Matt Jordan has posted comments on this change.

Change subject: Add a test for PJSIP t38 with authentication based on normal t38 test
......................................................................


Patch Set 1:

(15 comments)

https://gerrit.asterisk.org/#/c/22/1//COMMIT_MSG
Commit Message:

Line 7: Add a test for PJSIP t38 with authentication based on normal t38 test
I'd capitalize "T38"


Line 8: 
You need to add:

1. A full description of what the test does
2. The standard commit message tags, e.g., the ASTERISK issue related to the change


https://gerrit.asterisk.org/#/c/22/1/tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf
File tests/fax/pjsip/t38_with_auth/configs/ast1/extensions.conf:

Line 3: exten => 1234,n,SendFax(${ASTDATADIR}/send.tiff)
Use same =>


Line 6: exten => h,n,UserEvent(FaxStatus,operation: send,status: ${FAXOPT(status)},statusstr: ${FAXOPT(statusstr)},error: ${FAXOPT(error)})
Use same =>


https://gerrit.asterisk.org/#/c/22/1/tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf
File tests/fax/pjsip/t38_with_auth/configs/ast2/extensions.conf:

Line 3: exten => 1234,n,ReceiveFax(${ASTDATADIR}/receive.tiff)
Use same =>


Line 6: exten => h,n,UserEvent(FaxStatus,operation: receive,status: ${FAXOPT(status)},statusstr: ${FAXOPT(statusstr)},error: ${FAXOPT(error)})
Use same =>


https://gerrit.asterisk.org/#/c/22/1/tests/fax/pjsip/t38_with_auth/run-test
File tests/fax/pjsip/t38_with_auth/run-test:

Line 23: logger = logging.getLogger(__name__)
Our typical "practice" capitalizes LOGGER


Line 25: class T38Test(TestCase):
Add a pydoc comment for that this class is


Line 30:       TestCase.__init__(self)
Use the new style mechanism for invoking the base class init method:

super(T38Test, self).__init__()


Line 34:       # copy the tiff file we are going to send to a good known location
       :       shutil.copy("%s/send.tiff" % (os.path.dirname(os.path.realpath(__file__)),), "%s%s" % (self.ast[0].base, self.ast[0].directories['astdatadir']))
I'm fairly sure this will break the recommended line length :-)


Line 37:    def ami_connect(self, ami):
Add a pydoc comment


Line 43:          def handle_failure(reason):
       :             logging.error("error sending originate:")
       :             logging.error(reason.getTraceback())
       :             self.stop_reactor()
       : 
       :             return reason
       : 
       :          df.addErrback(handle_failure)
There's a standard handler for failed Originate calls in TestCase. Use that instead of providing your own.


Line 52:    def fax_result(self, ami, event):
pydoc comment please


https://gerrit.asterisk.org/#/c/22/1/tests/fax/pjsip/t38_with_auth/test-config.yaml
File tests/fax/pjsip/t38_with_auth/test-config.yaml:

Line 3:     description: |
      :         "This test starts two Asterisk instances and sends a fax between them
      :          using the PJSIP channel driver - The endpoints use userpass
      :          authentication."
I think it is worth mentioning why we needed this test. Referencing the ASTERISK issue (which there is a standard YAML tag for) would also be nice:

issue: ASTERISK-xxxxx


Line 9:     minversion: '13.3.1'
minversion should be '13.4.0'.

The patch that fixes this is not going into a point release.


-- 
To view, visit https://gerrit.asterisk.org/22
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id8fd9683dc1b61e7b1afd2b6ede857921beebb88
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Jonathan Rose <jrose at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes