[Asterisk-code-review] Added Debug Message for Test Objects Configured as Base Type (testsuite[master])

[Matt Jordan]

Matt Jordan has posted comments on this change.

Change subject: Added Debug Message for Test Objects Configured as Base Type
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/1775/1/lib/python/asterisk/test_runner.py
File lib/python/asterisk/test_runner.py:

Line 182:     try:
        :         module = __import__(module_name)
        :     except ImportError as e:
        :         LOGGER.error("ImportError: %s" % e)
        :         raise
I don't understand why this code change is necessary. If an ImportError occurs, we don't need to emit it as an ERROR message in the log; the exception is purposefully not handled so that the import fails and the test stops. Said exception is then printed out to stdout/stderr.


Line 323:     from test_case import TestCase
        : 
        :     if type(test_object) == TestCase:
        :         LOGGER.debug(
        :             "**Possible Test Misconfiguration: The test has been configured "
        :             "to instantiate a type of 'test_case.TestCase' as its "
        :             "test-object; however, this type is the base type for the Python "
        :             "tests and not a pluggable module. Did you mean to use "
        :             "'test_case.TestCaseModule?'")
> I'm not a huge fan of this. I think at the very least a comment in the Test
This code change limits us to only TestCase derived test objects. When the test runner code was written, that was not what I had envisioned - the whole point of the YAML driven tests is that you can pick your test object and its extensions from configuration. This hard codes the type of the object, which would be contrary to the overall purpose.

Practically, we currently only use TestCase derived test objects, but that's enforced by our current needs from the test case. Limiting our ability to use something else in the future only to aid debugging for a corner case doesn't feel right.

It is unfortunate that the debug message that is emitted by TestRunner for importing the TestObject is practically never displayed due to the logger not being set up by the test object yet. That feels like it could be handled easily, however, if a debug message was added here:

    # The test object must support injection of its location as a parameter
    # to the constructor, and its test-configuration object (or the full test
    # config object, if none is specified)
    test_obj = module_obj(test_path, test_object_config)

    LOGGER.debug('Created test object {0}'.format(test_object_spec['typename']))


That practically tells a developer all they need to know about which test object they made.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8a16d2f43ae5944680ba393f39bdb80f5df79e07
Gerrit-PatchSet: 1
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Ashley Sanders <asanders at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-HasComments: Yes