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

[Lukáš Doktor]

Dne 4.4.2016 v 19:46 Cleber Rosa napsal(a):
>
>
> 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!
>
My point was that when this code is part of the runner, we won't need 
the bi-directional communication (lock is also a communication channel), 
so it'll be actually easier :-)

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