[PATCH i-g-t] igt-runner fact checking

[Knop, Ryszard]

On Thu, 2024-11-07 at 09:55 -0600, Lucas De Marchi wrote:
> +Ryszard to know if the CI part would be doable.
> 
> On Thu, Nov 07, 2024 at 08:18:52AM +0100, Peter Senna Tschudin wrote:
> > > > +    scan_kernel_loaded_kmods(list, last_test);
> > > 
> > > 
> > > So... igt_facts() hardcodes what is the info collected. Maybe we
> > > should rather plug this into the hooks framework as part of
> > > "built-in hooks".
> > 
> > I do not follow what you are trying to communicate here. Also,
> > please see what Zbigniew said here*. Facts are not test
> > requirements, but the line is blurry. In the context here a fact is
> > something in the environment that can change but that should not.
> > An example is the disappearing GPU.
> 
> $ ./build/runner/igt_runner --help | grep -A1 hook
>    --hook HOOK_STR
>                          Forward HOOK_STR to the --hook option of
> each test.
>    --help-hook
>                          Show detailed usage information for --hook.
> 
> So... today we already have "generic support" for running arbitrary
> things before/after each test/subtest/dynamic-subtest.
> 
> 	$ cat << EOF > testlist.txt
> 	igt at xe_exec_basic@once-basic
> 	EOF
> 	$ chmod +x lspci.sh
> 	$ sudo ./build/runner/igt_runner \
> 		--hook 'post-subtest:lspci -vv -d 8086:*:03xx' \
> 		--test-list testlist.txt build/tests/ results/
> 	$ head -n4 results/0/out.txt
> 	03:00.0 VGA compatible controller: Intel Corporation DG2
> [Arc A580] (rev 08) (prog-if 00 [VGA controller])
>          Subsystem: Intel Corporation Device 1425
>          Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
> ParErr- Stepping- SERR- FastB2B- DisINTx-
>          Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast
> >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> 
> So, instead of creating a separate thing called "igt_facts", what I'm
> proposing is that we build this on top of the hooks and these "facts"
> are simply built-in hooks that are commonly used. We may even keep
> them
> as scripts rather than translate to C.
> 
> What I'd like is something liked this (with whatever name we decide):
> 
> 	sudo ./build/runner/igt_runner --builtin-hooks lspci,drm-
> cards
> 
> which is a shorthand or equivalent to:
> 
> 	sudo ./build/runner/igt_runner \
> 		--hook post-subtest:$PWD/scripts/built-in-
> hooks/lspci.sh \
> 		--hook post-subtest:$PWD/scripts/built-in-hooks/drm-
> cards.sh \
> 		--test-list testlist.txt build/tests/ results/
> 
> The cherry on top would be for CI to parse the cover letter and
> enable those ondemand, so we only do that in CI when we want to:
> 
> /ci-built-in-hooks: lspci,drm-cards
> 
> Could also be:
> 
> /ci-hook: 'post-subtest:lspci -vv -d 8086:*:03xx'

Extra hooks more or less hardcoded in the command line for a given run
are easy, just let us know when to add them. Reading extras from the
cover letter is doable, but would require time (think: early next year,
not a week or two), as we are switching the Patchwork integration to a
new implementation at the moment.

Although, two comments about the "facts" infra:

- I like the simple text format, but if possible, make the output
easier to parse with a consistent format for all state changes, like:
[FACT <test>] <new|changed|deleted>: <prop>: <value>[ -> <new_value>]

- While running these facts within hooks could work, it would require
extending the hooks with something that can understand that a given
command is supposed to output a bunch of facts in a well-known format
(ideally JSON, alternatively a "key: value" text block), then diff them
against the state from the previou run of the same hook.

Just dumping arbitrary logs into the main runner log is going to be
rather noisy, and having this done in hooks would make that infra more
complicated - not sure if having separate infra just for facts is
unwarranted here. I feel like most of the value here comes from a very
short and readable diff of the interesting system state before/after a
given test executes, not from just having a couple of commands run on
each subtest.

Thanks, Ryszard

> 
> (note that the current way we have is "Test-with:", but that is
> problematic as it's then parsed as a git-trailer and added to the
> patches by some tools)
> 
> Lucas De Marchi
> 
> > 
> > So I suggest we start with the hardcoded simple code to assess the
> > potential value of these facts. If the facts turns out to be useful
> > in helping us debug corner cases, then we evaluate how to extend
> > it.
> > 
> > Any other fact you want to add? Asking because I miss this code in
> > our CI to debug two issues that are on my queue...
> > 
> > > 
> > > This way it's easier to control what and when info is collected,
> > > allowing developers (and CI) to pick and choose what gets
> > > executed.
> > 
> > The original idea was to have static and dynamic facts. The dynamic
> > idea suggested a convenient mechanism for each test to define facts
> > as needed. Zbigniew did not appreciate the potential to bloating.
> > 
> > > 
> > > +Gustavo
> > > 
> > > Lucas De Marchi
> > 
> > * -
> > https://patchwork.freedesktop.org/patch/621849/?series=140566&rev=1
> >