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

[Matt Jordan]

Matt Jordan has posted comments on this change.

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


Patch Set 3:

(1 comment)

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
> Yeah, this was one of those parts where I really felt like this was kludgy 
> 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?

Purely from a technical perspective, of course that will work.

However, creating a discontinuity between the behaviour of the two feels wrong to me. Any time we end up with a 'if it is written this way, do this, otherwise if it is this way, do this other thing', I think we're marching down a slippery slope.

I'd only want to do this as an absolute last resort, where we have no other options available to us. To me, adding this option for 'realtime tests' creates a precedence that opens it up for more options.

> 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.

It shouldn't need to have the test object passed to it if it is a true pluggable module. Prior to this point, the pluggable module's initializer would have been called, which by definition, takes the module's configuration as well as the test object:

    obj = module_type(module_config, test_object)

If the pluggable module needs to hold a reference to the test object, it can do so in its __init__ function. When the 'setup_realtime' function is obtained from obj, obj has already had the ability to store a reference to the test_object if it needs it during the invocation of setup_realtime.

> 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).

I disagree. Hard-coding type limits you to that type, period. You can't have an object of another type. Test writers are forced to extend the same type if they need different functionality.

A 'function name' - which I admit is not wonderful - can, however, still be implemented by an object of any type. The object merely has to have that symbol.

I disagree as well with the notion that a realtime-enabled pluggable module doesn't work the same as other pluggable modules. They do: they're initialized in the exact same fashion, which is the only requirement a pluggable module has. From the perspective of test_runner, a pluggable module is simply one that has a type defined in the YAML, and that has an initializer that matches a particular signature. Everything else is an implementation detail. The fact that we would now support invoking a particular method on a pluggable module with a particular configuration section pulled from the global config may not be great, but it doesn't violate the existing contracts that a pluggable module has.

> 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'sconfiguration.
> 
> 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?

I think having a globally defined set of pluggable modules that can be loaded is a good idea. There's certainly nothing wrong with that.

I disagree with having an 'opt out' approach. Historically, things that have had to 'opt out' have been more problematic than opting in, and we've had to do a lot more test fixing up (and playing test failure whack-a-mole) when we've enabled something for everyone. Witness the failure of the pre/post-test condition framework as Exhibit A.

I'd much rather have tests specify that they want realtime. Yes, that means one patch will have to go in and enable that on a large set of tests - but at least we'll be slowly and carefully turning this on. To me, that beats the unknown of going through and fixing tests that previously weren't broken for the next two months.


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