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

Mon Apr 4 17:46:42 UTC 2016

On 04/04/2016 08:01 AM, Lukáš Doktor wrote:
> Dne 1.4.2016 v 16:00 Cleber Rosa napsal(a):
>> 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.
> I'd prefer "pluginizing" the whole "runner" instead of custom pre and
> post classes. What am I talking about:
>
> The CLICmd and CLI allows one to add several methods + "run" method
> which is executed to do the action. It makes sense for CLI, but IMO it
> does not suit this case.
>
> Instead we can create plugin interface which allows to do things on
> certain occasions (hooks), one of them `start_test` and `stop_test`.
> It's similar to `ResultsProxy`, `LoaderProxy`, ....
>
> They both can achieve the same, the main reason is convenience:
>
> The CLI-like:
>
> + clearly defines the interface
> + adds itself by publishing itself into the correct namespace
> - for pre+post plugins requires double plugin initialization
> - to reuse information from pre-hook in post-hook one needs to store the
> state inside the results.
>
> The *Proxy-like:
>
> + defines the interface
> + adds itself by publishing itself into the correct namespace
> + pre+post plugins are initialized just once (pure pre-plugins define
> only `pre_test` hook, post-plugins only `post_test` hook...)
> + the state is preserved throughout the execution, so one can store the
> details inside `self`.
> + is easily extensible of another hooks related to this
>
> Details in
> https://github.com/avocado-framework/avocado/pull/1106#discussion_r58193746
>

I believe this actually escapes the scope of this RFC, so I'll write a 
separate RFC regarding how we define the Plugin interfaces and its 
granularity.

>
>>
>> 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.
> ACK
>
>>
>> 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.
> I'm fine with either "real time" `time.time - start`, or with the
> "user+sys time" `sum(test.time for test in job.tests)` (so sum of all
> test times). I don't think we should do anything smart in here as it
> might be misleading.
>
>      time stress -c 8 -t 10
>      stress: info: [23182] dispatching hogs: 8 cpu, 0 io, 0 vm, 0 hdd
>      stress: info: [23182] successful run completed in 10s
>
>      real    0m10.001s
>      user    1m19.005s
>      sys     0m0.003s
>

I created a card to set Avocado job time as the "wall clock time":

https://trello.com/c/TXZlbQ4u/639-job-time-should-be-the-wall-clock-time

>
>>
>> 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.
> Does it means the test execution would wait for the pre-job hooks
> completion? It's logical, but currently it requires bi-directional
> communication with the runner (not after the Test/runner cleanup).

A simple `multiprocessing.Lock` does the trick here, no need for 
bi-directional communication.

>
> Anyway yes, the pre-job should get the early status with all benefits
> and drawbacks. Users are responsible to understand them.
>
>>
>> 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.
> OMG, I should first read and then think... For now, yes, but once we
> move the test workflow from test to runner (as proposed in the
> https://trello.com/c/W58vhyHR/539-bug-some-avocado-test-methods-and-atributes-are-public
> ).
>

Once we move...? I didn't get what kind of incompatible change you 
anticipate by the "move the test workflow from the test to runner" task.

I see that it would basically change how a test state could be 
generated, but it still seems like it wouldn't block the current flow:

a) instantiate test
b) generate test state (from either within the test or externally from 
the runner)
c) running pre test hooks based on the acquired state
d) running the test

> PS: The pre and post test hooks are acctually also mentioned in that
> card...
>

I do see start_test_hooks() and stop_test_hooks() in the referenced 
card. Again, I don't see how it would be made incompatible with this 
proposal. Please enlighten me!

>>
>> 7) The post test hook is given the last test status, which is also used
>> by the runner to identify test success, failure, etc.
> Yep
>
>>
>>
>> Thanks,
>>   - Cleber.
>>

-- 
Cleber Rosa
[ Sr Software Engineer - Virtualization Team - Red Hat ]
[ Avocado Test Framework - avocado-framework.github.io ]