[Asterisk-code-review] chan pjsip: Creating Channel Causes Asterisk to Crash When D... (testsuite[master])

[Kevin Harwell]

Kevin Harwell has posted comments on this change.

Change subject: chan_pjsip: Creating Channel Causes Asterisk to Crash When Duplicate AOR Sections Exist in pjsip.conf
......................................................................


Patch Set 2:

(3 comments)

The duplicate entry problem/check should be more of a sorcery thing than being res_pjsip specific right? If that is the case then one scenario for a duplicate entry should be sufficient. Meaning if one duplicate entry fails then every time a duplicate entry exist it should fail. If that's not the case, and it is more of a res_pjsip thing, then yes all cases (and any future new ones) will need to be iterated through.

https://gerrit.asterisk.org/#/c/241/2/tests/channels/pjsip/configuration/duplicate_sections/duplicate_sections.py
File tests/channels/pjsip/configuration/duplicate_sections/duplicate_sections.py:

Line 1: #!/usr/bin/env python
      : """
      : Copyright (C) 2015, Digium, Inc.
      : Ashley Sanders <asanders at digium.com>
      : 
      : This program is free software, distributed under the terms of
      : the GNU General Public License Version 2.
      : """
      : 
      : import sys
      : 
      : sys.path.append("lib/python")
      : sys.path.append("tests/channels/pjsip/configuration")
      : 
      : from test_harness import TestHarness
      : 
      : 
      : class DuplicateSectionsTestHarness(TestHarness):
      :     """The test harness for the duplicate sections test."""
      : 
      :     def __init__(self, config, test_object):
      :         """Constructor.
      : 
      :         Keyword Arguments:
      :         config                 -- The YAML configuration for this test.
      :         test_object            -- The TestCaseModule instance for this test.
      :         """
      : 
      :         super(DuplicateSectionsTestHarness, self).__init__(config,
      :                                                            test_object)
Since this class doesn't really offer any extra functionality I would just dispose of it.


https://gerrit.asterisk.org/#/c/241/2/tests/channels/pjsip/configuration/happy_config/happy_config.py
File tests/channels/pjsip/configuration/happy_config/happy_config.py:

Line 1: #!/usr/bin/env python
      : """
      : Copyright (C) 2015, Digium, Inc.
      : Ashley Sanders <asanders at digium.com>
      : 
      : This program is free software, distributed under the terms of
      : the GNU General Public License Version 2.
      : """
      : 
      : import sys
      : 
      : sys.path.append("lib/python")
      : sys.path.append("tests/channels/pjsip/configuration")
      : 
      : from test_harness import TestHarness
      : 
      : 
      : class HappyConfigTestHarness(TestHarness):
      :     """The test harness for the duplicate sections test."""
      : 
      :     def __init__(self, config, test_object):
      :         """Constructor.
      : 
      :         Keyword Arguments:
      :         config                 -- The YAML configuration for this test.
      :         test_object            -- The TestCaseModule instance for this test.
      :         """
      : 
      :         super(HappyConfigTestHarness, self).__init__(config, test_object)
No real need for this class as well. I would just delete it and have the test-config(s) reference TestHarness directly.


https://gerrit.asterisk.org/#/c/241/2/tests/channels/pjsip/configuration/test_harness.py
File tests/channels/pjsip/configuration/test_harness.py:

Line 146:         for key in config.keys():
        :             k = key.lower()
        :             if k != 'cli_command' and k != 're_query':
        :                 msg = '{0} Unsupported configuration attribute {1} specified.'
        :                 LOGGER.error(msg.format(self, key))
        :                 return False
If extra options are specified should that be a cause for test failure or just a noop? Also if you wanted this could be refactored to just check if key count > 2.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I36078aae985050cff323ace3ccfd7464fe1de35f
Gerrit-PatchSet: 2
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes