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

[Ashley Sanders]

Ashley Sanders 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:

(4 comments)

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 
-- This comment was also added to your issue raised in happy_config.py --

TestHarness is an abstraction of the common code between happy_config and duplicate_sections. Unfortunately, the way the TestModule logic works does not easily lend itself to be friendly to code abstraction outside of the [lib/python/asterisk] path, so some tricks are required. 

Even if the module is added to the search path via the test config (which it is), the module's specific path will be appended, which is one step outside of TestHarness's 'namespace'. Here, like in the case of HappyConfigTestHarness, this class serves as a bootstrapper for the parent type, TestHarness, whose path is inaccessible to the TestModuleFinder at the time each test's [test-config.yaml] is loaded.


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 te
-- This comment was also added to your issue raised in duplicate_sections.py --

TestHarness is an abstraction of the common code between happy_config and duplicate_sections. Unfortunately, the way the TestModule logic works does not easily lend itself to be friendly to code abstraction outside of the [lib/python/asterisk] path, so some tricks are required. 

Even if the module is added to the search path via the test config (which it is), the module's specific path will be appended, which is one step outside of TestHarness's 'namespace'. Here, like in the case of DuplicateSectionsTestHarness, this class serves as a bootstrapper for the parent type, TestHarness, whose path is inaccessible to the TestModuleFinder at the time each test's [test-config.yaml] is loaded.


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

Line 111:         LOGGER.debug('{0} Quering test scenarios.'.format(self))
> s/Quering/Querying/
Thanks! I blame my long fingernails :p.


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 j
I want it to fail so that the author has immediate information that he/she has authored the test in an unsupported way. 

Regarding 'if key count > 2' -- yes, the test could be refactored to use that logic however, the log message as/is gives the author some useful feedback as to what specific unsupported attribute was included in the test. A generic check is only going to tell the author that some number of attributes was included that was greater than the number the test was expecting.

One thing I could do, however, is continue the loop so that the test does not fail the first time it encounters an invalid option. If an invalid attribute is detected, a flag can be set that the test is invalid, but the loop would continue such that there is a log message for all unsupported attributes that have been included in the file. I could see those messages being helpful for anyone using this pattern going forward.


-- 
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: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-HasComments: Yes