[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 7:

(2 comments)

Response to some of Matt Jordan's feedback.

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 m
The only reason this is there is because the lines were getting too long as I was nesting if-statements. 

I knew it at the time and I agree with 100%, this is overkill. I am going to remove it for the next patch.


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:
After thinking about this, I sort of chuckled to myself. I am using the validate functions as a poor-man's 'unit test' for my integration test - a way of keeping me honest. It also helps me quickly recall the expected structure of the config files as the documentation is all in the validate method. 

However, **I agree with your statement**. I am a big believer in avoiding YAGNI code. This file is not intended to become part of the overall API, therefore, this code is adding unnecessary weight to the system. Since, I most likely won't need this again, this is definitely YAGNI.

I will be cognizant of this 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: 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