Re: [PATCH] tests/qtest: add TIMEOUT_MULTIPLIER

[Fabiano Rosas <farosas@suse.de>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 27/11/24 19:44, Pierrick Bouvier wrote:
>> On 11/26/24 23:40, Дмитрий Фролов wrote:
>>> Hello, Daniel
>>>
>>> On 26.11.2024 21:32, Daniel P. Berrangé wrote:
>>>> On Wed, Nov 13, 2024 at 12:43:40PM +0300, Dmitry Frolov wrote:
>>>>> Some tests need more time when qemu is built with
>>>>> "--enable-asan --enable-ubsan"
>>>>>
>>>>> As was discussed here:
>>>>> https://patchew.org/QEMU/20241112120100.176492-2-frolov@swemel.r/u
>>>>>
>>>>> TIMEOUT_MULTIPLIER enviroment variable will be
>>>>> a useful option, allowing non-invasive timeouts
>>>>> increasing for a specific build.
>>>>>
>>>>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>>>>> ---
>>>>>    scripts/mtest2make.py | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/scripts/mtest2make.py b/scripts/mtest2make.py
>>>>> index eb01a05ddb..ff60b62724 100644
>>>>> --- a/scripts/mtest2make.py
>>>>> +++ b/scripts/mtest2make.py
>>>>> @@ -27,7 +27,9 @@ def names(self, base):
>>>>>    .speed.slow = $(foreach s,$(sort $(filter-out %-thorough, $1)), 
>>>>> --suite $s)
>>>>>    .speed.thorough = $(foreach s,$(sort $1), --suite $s)
>>>>> +ifndef TIMEOUT_MULTIPLIER
>>>>>    TIMEOUT_MULTIPLIER = 1
>>>>> +endif
>>>> Can you explain what scenario this is needed for, as unless I'm
>>>> missing something this change has no purpose. This assignment is
>>>> merely defining the defalt value, which can already be overridden
>>>> at runtime without this 'ifndef'
>>>>
>>>> eg
>>>>
>>>> $ make check-unit TIMEOUT_MULTIPLIER=7
>>>>
>>>> In another shell:
>>>>
>>>> $ ps -axuwwf | grep 'meson test'
>>>> berrange 1931657  3.9  0.1 330904 99344 pts/1    S+   18:29   
>>>> 0:00                  \_ 
>>>> /var/home/berrange/src/virt/qemu/build/pyvenv/bin/python3 
>>>> /var/home/berrange/src/virt/qemu/build/pyvenv/bin/meson test 
>>>> --no-rebuild -t 7 --num-processes 1 --print-errorlogs --suite unit
>>>>
>>>> shows TIMEOUT_MULTIPLIER being honoured
>>> Yeah... You are right!
>>> It is possible to set TIMEOUT_MULTIPLIER only to run tests.
>>> It is not necessary to set it for the whole build.
>>>
>>> Sorry, and thanks a lot!
>>>>
>>>>
>>>> With regards,
>>>> Daniel
>>> regards,
>>> Dmitry
>>>
>> 
>> This patch is still useful if we want to set TIMEOUT_MULTIPLIER as 
>> global env variable, and not explicitely on each make invocation.
>
> If so, I'd rather use conditional assignment [*]:
>
> - TIMEOUT_MULTIPLIER = 1
> + TIMEOUT_MULTIPLIER ?= 1
>
> [*] 
> https://www.gnu.org/software/make/manual/html_node/Conditional-Assignment.html

I can fix that up in qtest-next, thanks.