diff mbox series

[FFmpeg-devel] fate: Allow running multiple rounds of tests with differing settings

Message ID 20231211221406.2385689-1-martin@martin.st
State Superseded
Headers show
Series [FFmpeg-devel] fate: Allow running multiple rounds of tests with differing settings | expand

Checks

Context Check Description
andriy/configure_x86 warning Failed to apply patch
yinshiyou/configure_loongarch64 warning Failed to apply patch

Commit Message

Martin Storsjö Dec. 11, 2023, 10:14 p.m. UTC
This can be used to run tests multple times, with e.g. differing
QEMU settings, by adding something like this to the FATE configuration
file:

    target_exec="qemu-aarch64-static"
    fate_targets="fate-checkasm fate-cpu"

    fate_environments="sve128 sve256 sve512"
    sve128_env="QEMU_CPU=max,sve128=on"
    sve256_env="QEMU_CPU=max,sve256=on"
    sve512_env="QEMU_CPU=max,sve512=on"

It's also possible to customize the target_exec command further
by injecting a sufficiently quoted variable into it, which then can
be updated for each run, e.g. target_exec="\$(CUR_EXEC_CMD)".

For each of the environment names in fate_environments, the tests
that are run get the name suffixed on the fate tests in the
test log and fate report, e.g. "fate-checkasm-h264dsp_sve128".
---
For adding the environment to the fate run, there's three
ways it can be done:
- (${curenv}; make fate)
- ${curenv} make fate
- make fate ${curenv}

The first has most freedom in what one can do, but requires explicit
"export VAR=value", while the other two are simpler for just setting
"VAR1=val1 VAR2=val2". For now, I picked the third form.
---
 tests/Makefile |  4 ++--
 tests/fate.sh  | 14 +++++++++++++-
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Rémi Denis-Courmont Dec. 13, 2023, 6:29 p.m. UTC | #1
Le tiistaina 12. joulukuuta 2023, 0.14.06 EET Martin Storsjö a écrit :
> This can be used to run tests multple times, with e.g. differing
> QEMU settings, by adding something like this to the FATE configuration
> file:
> 
>     target_exec="qemu-aarch64-static"
>     fate_targets="fate-checkasm fate-cpu"
> 
>     fate_environments="sve128 sve256 sve512"
>     sve128_env="QEMU_CPU=max,sve128=on"
>     sve256_env="QEMU_CPU=max,sve256=on"
>     sve512_env="QEMU_CPU=max,sve512=on"

I'm fine with that, but for the sake of generality, shouldn't rather the entire 
target_exec prefix be indirected? Some runners may want to use command line 
flags rather than environment variables.
Martin Storsjö Dec. 13, 2023, 8:45 p.m. UTC | #2
On Wed, 13 Dec 2023, Rémi Denis-Courmont wrote:

> Le tiistaina 12. joulukuuta 2023, 0.14.06 EET Martin Storsjö a écrit :
>> This can be used to run tests multple times, with e.g. differing
>> QEMU settings, by adding something like this to the FATE configuration
>> file:
>>
>>     target_exec="qemu-aarch64-static"
>>     fate_targets="fate-checkasm fate-cpu"
>>
>>     fate_environments="sve128 sve256 sve512"
>>     sve128_env="QEMU_CPU=max,sve128=on"
>>     sve256_env="QEMU_CPU=max,sve256=on"
>>     sve512_env="QEMU_CPU=max,sve512=on"
>
> I'm fine with that, but for the sake of generality, shouldn't rather the 
> entire target_exec prefix be indirected? Some runners may want to use 
> command line flags rather than environment variables.

Yes - that's also doable. One can e.g. do this:

---
target_exec="qemu-aarch64-static -cpu \$(MY_CPU)"
fate_targets="fate-checkasm fate-cpu"

fate_environments="sve128 sve256 sve512"
sve128_env="MY_CPU=max,sve128=on"
sve256_env="MY_CPU=max,sve256=on"
sve512_env="MY_CPU=max,sve512=on"
---

That way, one can also make the whole target_exec be e.g. \$(EXEC_CMD) and 
set the full command via the individual envs. It's not quite as 
comfortable, but should be doable and allow fully generic setups.

(One could also consider just doing feat1_env="TARGET_EXEC=..." and not 
setting target_exec in the fate config at all - however that's probably 
not a good option and it has one surprising gotcha. The makefile level 
TARGET_EXEC variable needs to have a trailing space when it is set to a 
nonempty value, and injecting that might be annoying or ugly.)

// Martin
Martin Storsjö Dec. 16, 2023, 10:02 p.m. UTC | #3
On Wed, 13 Dec 2023, Martin Storsjö wrote:

> On Wed, 13 Dec 2023, Rémi Denis-Courmont wrote:
>
>> Le tiistaina 12. joulukuuta 2023, 0.14.06 EET Martin Storsjö a écrit :
>>> This can be used to run tests multple times, with e.g. differing
>>> QEMU settings, by adding something like this to the FATE configuration
>>> file:
>>>
>>>     target_exec="qemu-aarch64-static"
>>>     fate_targets="fate-checkasm fate-cpu"
>>>
>>>     fate_environments="sve128 sve256 sve512"
>>>     sve128_env="QEMU_CPU=max,sve128=on"
>>>     sve256_env="QEMU_CPU=max,sve256=on"
>>>     sve512_env="QEMU_CPU=max,sve512=on"
>> 
>> I'm fine with that, but for the sake of generality, shouldn't rather the 
>> entire target_exec prefix be indirected? Some runners may want to use 
>> command line flags rather than environment variables.
>
> Yes - that's also doable. One can e.g. do this:
>
> ---
> target_exec="qemu-aarch64-static -cpu \$(MY_CPU)"
> fate_targets="fate-checkasm fate-cpu"
>
> fate_environments="sve128 sve256 sve512"
> sve128_env="MY_CPU=max,sve128=on"
> sve256_env="MY_CPU=max,sve256=on"
> sve512_env="MY_CPU=max,sve512=on"
> ---
>
> That way, one can also make the whole target_exec be e.g. \$(EXEC_CMD) and 
> set the full command via the individual envs. It's not quite as comfortable, 
> but should be doable and allow fully generic setups.

If this sounds reasonable enough, I would go ahead and land this, together 
with 
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20231127123118.3622784-1-martin@martin.st/.

I guess this feature should have some documentation as well - I can draft 
up a v2 with docs before going ahead with it, if the feature overall looks 
acceptable.

// Martin
diff mbox series

Patch

diff --git a/tests/Makefile b/tests/Makefile
index 444c09b3de..744dbcdfb3 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -305,8 +305,8 @@  $(FATE): export PROGSUF = $(PROGSSUF)
 $(FATE): export EXECSUF = $(EXESUF)
 $(FATE): export HOSTEXECSUF = $(HOSTEXESUF)
 $(FATE): $(FATE_UTILS:%=tests/%$(HOSTEXESUF)) | $(FATE_OUTDIRS)
-	@echo "TEST    $(@:fate-%=%)"
-	$(Q)$(SRC_PATH)/tests/fate-run.sh $@ "$(TARGET_SAMPLES)" "$(TARGET_EXEC)" "$(TARGET_PATH)" '$(CMD)' '$(CMP)' '$(REF)' '$(FUZZ)' '$(THREADS)' '$(THREAD_TYPE)' '$(CPUFLAGS)' '$(CMP_SHIFT)' '$(CMP_TARGET)' '$(SIZE_TOLERANCE)' '$(CMP_UNIT)' '$(GEN)' '$(HWACCEL)' '$(REPORT)' '$(KEEP_FILES)'
+	@echo "TEST    $(@:fate-%=%)$(FATE_SUFFIX)"
+	$(Q)$(SRC_PATH)/tests/fate-run.sh $@$(FATE_SUFFIX) "$(TARGET_SAMPLES)" "$(TARGET_EXEC)" "$(TARGET_PATH)" '$(CMD)' '$(CMP)' '$(REF)' '$(FUZZ)' '$(THREADS)' '$(THREAD_TYPE)' '$(CPUFLAGS)' '$(CMP_SHIFT)' '$(CMP_TARGET)' '$(SIZE_TOLERANCE)' '$(CMP_UNIT)' '$(GEN)' '$(HWACCEL)' '$(REPORT)' '$(KEEP_FILES)'
 
 fate-list:
 	@printf '%s\n' $(sort $(FATE))
diff --git a/tests/fate.sh b/tests/fate.sh
index 95408ea109..c04eb41cbe 100755
--- a/tests/fate.sh
+++ b/tests/fate.sh
@@ -101,7 +101,19 @@  compile_extra()(
 fate()(
     test "$build_only" = "yes" && return
     cd ${build} || return
-    ${make} ${makeopts_fate-${makeopts}} -k ${fate_targets}
+    if [ -n "${fate_environments}" ]; then
+        ret=0
+        for e in ${fate_environments}; do
+            eval "curenv=\${${e}_env}"
+            echo Testing environment ${e}: ${curenv}
+            ${make} ${makeopts_fate-${makeopts}} -k ${fate_targets} FATE_SUFFIX=_${e} ${curenv}
+            cur_ret=$?
+            test $cur_ret != 0 && ret=$cur_ret
+        done
+        return $ret
+    else
+        ${make} ${makeopts_fate-${makeopts}} -k ${fate_targets}
+    fi
 )
 
 clean(){