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

[Matt Jordan]

Matt Jordan 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 7: Code-Review-1

(2 comments)

Per Kevin's comment on draft 2 and the ability to add arbitrary paths to the module loader search path, I'd suggest removing the empty classes as well.

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

Line 83:         messages = dict()
       : 
       :         base_msg = '{0} '.format(self)
       :         messages['aborting_config_error'] = base_msg + \
       :             'Aborting test. Configuration contains errors.'
       :         messages['calculating_results'] = base_msg + \
       :             'Calculating test results.'
       :         messages['building_scenarios'] = base_msg + \
       :             'Building test scenarios.'
       :         messages['initializing_harness'] = base_msg + \
       :             'Initializing test harness.'
       :         messages['loading_config'] = base_msg + \
       :             'Loading module configuration.'
       :         messages['missing_attribute'] = base_msg + \
       :             '{0} is missing required attribute \'{1}\'.'
       :         messages['missing_config'] = base_msg + \
       :             'No configuration provided.'
       :         messages['querying_scenarios'] = base_msg + \
       :             'Querying test scenarios.'
       :         messages['starting_scenario'] = base_msg + \
       :             'Starting execution for scenario[{0}]'
       :         messages['test_complete'] = base_msg + \
       :             'Test case execution is complete.'
       :         messages['unsupported_attribute'] = base_msg + \
       :             'Unsupported configuration attribute \'{0}\' specified.'
       :         messages['validating_config'] = base_msg + \
       :             'Validating module configuration.'
       : 
       :         return messages
I think this is probably overkill. The Python logger already prepends log messages with the namespace of the particular logger instance, which is generally the name of the module. That provides context for who is generating the log message - adding a localized dictionary of messages with a prepended formatting of the class would only be useful if there were many, many classes in this module, and we had a need for localizing the log messages themselves.

I'd say just use the logger methods directly. That provides the full message in the context of the code responsible for raising it.


Line 123:         if not self.__validate_config(config):
        :             LOGGER.error(self.__messages['aborting_config_error'])
        :             self.test_object.stop_reactor()
This is more as a comment for the future:

I wouldn't go through the config validation steps here unless this was a truly reusable component in lib/python/asterisk. YAGNI, in this case, applies - only your test is using this, and it provides the configuration.

What's more, a misconfiguration will get caught at run-time with the same end result as this function: Something will throw an exception and the test will fail.

There's certainly no point in removing this (it is nice, after all) - but unless you are going to have a lot of test contributors using a piece of code, you can let programmer error raise an Exception - a misconfiguration is exceptional, after all.


-- 
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: 7
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-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes