[Avocado-devel] [RFC] Pre/Post test hooks

[Amador Pahim]

On 04/12/2016 10:10 AM, Cleber Rosa wrote:
>
>
> On 04/11/2016 09:44 PM, Amador Pahim wrote:
>>
>>
>> On 04/01/2016 11:00 AM, Cleber Rosa wrote:
>>> MOTIVATION
>>> ==========
>>>
>>> The idea of adding hooks to be run by Avocado before and after tests
>>> is general enough, and may be used by the community in unpredictable
>>> ways. And that is good.
>>>
>>> For this team, the initial motivation was to be able to bring back an
>>> Autotest feature that some of our users are missing: the ability to
>>> set the system-wide "kernel core pattern" configuration for tests.
>>>
>>> Having a pre-test hook would allow "/proc/sys/kernel/core_pattern" to
>>> be read, saved and modified to point to the test results directory.
>>> Having a post-test hook would allow "/proc/sys/kernel/core_pattern" to
>>> be reverted back to its original state.
>>>
>>> Other currently core features such as sysinfo collection, could be
>>> re-implemented as pre/post test hooks.
>>>
>>> GENERAL DESIGN POINTS
>>> =====================
>>>
>>> These are the most important design decisions to be acknowledged or
>>> questioned. Please reply with either ACK or your questions/suggestions.
>>>
>>> 1) Hooks are implemented as plugin classes, based on a given defined
>>> interface, in the same way current "CLICmd" and "CLI" interfaces allow
>>> plugin writers to extend Avocado and give it new commands and command
>>> line options.
>>
>> Given the discussion is way ahead and we already have a PR in place,
>> maybe this is too late, but I'd like to give my 2 cents here. I like the
>> plugins approach, but I don't think that's the best way to implement the
>> hooks mechanism. From the experience with oVirt, I'm used to think in
>> hooks as a directory where I put some executable(s) that will be
>> executed in a given moment. Each directory corresponds to a moment. The
>> order that the executables are executed is simple given by their names.
>> The hooking api itself can and should be a plugin, but user should not
>> have to bother with the plugin stuff.
>>
>> Example: directories '~/avocado/before_tests/' and
>> '~/avocado/after_tests/' are the first candidates to cover this RFC. In
>> the future, if we have a request for it, we can have '/before_test/',
>> '/after_test/', '/after_failed_test/' and so on.
>>
>
> This functionality is also included in the upcoming *test* pre/post 
> PR. It's built using the pre and post interfaces defined/discussed 
> here. So, for the majority of use cases the scripts will do and this 
> will probably not be used directly. Still, there are some use cases 
> which require closer coupling. Let's call this "test pre/post 
> scripts", a plugin that uses the "test pre/post" interfaces.

Ok, that sounds great.

>
>> For those executables inside the directories, if using python and
>> importing the avocado hooking module, then they can access avocado job
>> and/or tests information, probably the same information you're already
>> considering to expose in your current proposal.
>
> Do you mean something like:
>
>    from avocado.hooks import get_test_info
>
>    current_test = get_test_info()

Yes, but it will only make sense if we allow hooks to change the tests. 
Otherwise, the env variable will do it great.

>
> It hurts my brain when I think of possible implementations. Please 
> excuse if I'm mistaken, but it looks like this kind of approach maps 
> oVirt a lot better, because it has a central database and a remotely 
> accessible API.  So, it's a lot easier and cleaner to get information 
> and manipulate every aspect of every object in a hook.
>
> And for non Python code, this would require the definition of an API 
> based on either environment variables or command line parameters, 
> right?  My idea is to turn test state into a set environment variables 
> that the test pre/post scripts will have access to.
>
> Example test state:
>
>   {"results_dir": 
> "/home/foo/avocado/job-results/job-2016-04-11T19.38-9eb1b73/test-results/bar", 
> ... }
>
> Would become:
>
>
> AVOCADO_TEST_RESULTS_DIR="/home/foo/avocado/job-results/job-2016-04-11T19.38-9eb1b73/test-results/bar" 
>
>
> The test pre/post script could then be something like:
>
>    #!/bin/bash
>    echo "great success" > $AVOCADO_TEST_RESULTS_DIR/status

Ack.

>
>>
>>>
>>> 2) The hooks are executed by the *runner*, and not by the test
>>> process. The goal is not interfere with the test itself. The pre and
>>> post code that runs before and after the test should not *directly*
>>> change the test behavior and outcome. Of course, the test environment
>>> can be changed in a way (say having packages removed) that a test may
>>> fail because of hook actions.
>>
>> I don't see a problem in change the test directly. It can be dangerous,
>> of course, but user is expected to know what he's doing when using
>> hooks. In oVirt hooks we are able, for example, to edit everything we
>> want in the VM xml before using the xml to create the VM in libvirt.
>> Anyway, since we don't have a request or use case for this currently,
>> ACK for the 'not interfere with the test itself'.
>>
>>>
>>> 3) Test execution time should not be changed by pre and post hooks. If
>>> a pre-test hook takes "n" seconds to run, "n" should not be added to
>>> the test run time.
>>
>> Ack.
>>
>>>
>>> 4) Job run time: right now, Avocado times a Job based on the sum of
>>> individual test run times. With pre and post test hooks, this can be
>>> very different from job "wall clock" times. My instinct is to change
>>> that, so that a Job run time is the job "wall clock" time. I'm unsure
>>> if we should add yet another time measure, that is, the sum of
>>> individual test run time. This is also bound to be broken when
>>> parallel run of tests is implemented.
>>
>> Ack.
>>
>>>
>>> 5) The pre test hook is given the test "early status". Information
>>> such as the test tagged name, the fact that it has not yet started to
>>> run and the test results directory are all part of the early status.
>>
>> Ack.
>>
>>>
>>> 6) Because of point #5, the test is instantiated on the test process,
>>> its early state is sent, but the test execution itself is held until
>>> the runner finishes running the pre-test hooks.
>>
>> Ack.
>>>
>>> 7) The post test hook is given the last test status, which is also
>>> used by the runner to identify test success, failure, etc.
>>
>> With the advent of 'summary' being populated on the runner, 'summary'
>> might be more useful to the post test hook.
>>
>
> Wouldn't this make more sense for jobs?

Yes sorry, I misunderstood this as 'post tests hooks'. Yes, it makes 
sense only for a hook operating after all tests finish.

>
> Thanks for the feedback!
>  - Cleber
>
>>>
>>>
>>> Thanks,
>>>  - Cleber.
>>>
>>
>