[Asterisk-code-review] RFC: Add runtime option that allows automatic realtime execu... (testsuite[master])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: RFC: Add runtime option that allows automatic realtime execution.
......................................................................


Patch Set 3:

(2 comments)

https://gerrit.asterisk.org/#/c/1803/3/lib/python/asterisk/test_case.py
File lib/python/asterisk/test_case.py:

Line 232:             'asterisk-instances' in self.global_config.config):
> If asterisk-instances is not in self.global_config.config, wouldn't this me
No, the else statement gets executed instead, so local Asterisk instances are created.


https://gerrit.asterisk.org/#/c/1803/3/lib/python/asterisk/test_runner.py
File lib/python/asterisk/test_runner.py:

Line 327:    if (len(args) >= 4):
        :         realtime = True
        :     else:
        :         realtime = False
> I've been trying to think of another way to deal with the notion of having 
Yeah, this was one of those parts where I really felt like this was kludgy as all heck, so I agree in principle that something could probably be done better here, but I'm not necessarily following your train of logic in your comment here.

>The real problem with passing a command line option is not
>TestRunner, nor even with TestClass. It's all the tests that
>have their own run-test. Even if we don't ever envision
>those tests making use of the 'automatic realtime' test
>option, we're still breaking the contract of what is passed
>to the run-test script. I'll grant that it's rare that we
>write tests in that fashion, but it is something that is a
>concern, and at least a reason to try and find another path.
{quote}

Since runtests.py already has checks for whether test_runner or a custom run-test script is going to be executed, would it be as problematic if the realtime command line option were only passed if running test_runner.py?

(Pretend I inserted a quote of the rest of your comment here)

The downside to what you've suggested here is that the test object is not being passed to setup_realtime. The test object is necessary so that the realtime converter can get attributes about the test itself (such as the directory where Asterisk configuration files can be found). That may have just been an oversight when you were typing the comment though, as passing the test object to setup_realtime() seems like it could be done just as easily.

The thing about this is, to me, it's not really much better than what I already have here. You're trading the hard-coded realtime class for a hard-coded realtime setup function. It also means that realtime-enabled pluggable modules don't work the same as any other pluggable modules. (i.e. there's a hardcoded setup function rather than just initializing a class).

What about an alternative that circles back to what Kevin suggested, only more so. Global config can look something like this:

    global-settings:
        test-configuration: my-test-config

    my-test-config:
        modules:
            -
                typename: realtime_converter.RealtimeConverter
                config-section: realtime-config
                override-tags:
                    - no-realtime
    
    realtime-config:
        engine: 'postgresql'
        username: 'asterisk'
        password: 'asterisk'
        host: 'localhost'
        port: '5432'
        db: 'asterisk'
        dsn: 'asterisk'

This way, global configuration will have a modules section just like any test does. And just like any test, we have a list of modules and their config section within the yaml. The only new addition is the 'override-tags' option. This makes it so that individual tests can tag themselves so that the global module will  not be applied if that tag is present in the individual test's configuration.

With this, test_runner.py doesn't have to have any sort of hard-coded values, and there's no need for a command line option since the global test configuration will indicate what global pluggable modules to load. This also opens up the possibility of other types of global pluggable modules being created easily, if it turns out to be useful for some other purpose.

What do you think of that?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieec1b1cfa48cadab108c4ab65122ce36ab697e4e
Gerrit-PatchSet: 3
Gerrit-Project: testsuite
Gerrit-Branch: master
Gerrit-Owner: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
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