[igt-dev] [RFC] Use HAX for series which add igt_describe

[Zbigniew Kempczyński]

On Mon, Mar 21, 2022 at 11:26:28AM +0200, Petri Latvala wrote:

<cut>

> > > One downside is when developers document dynamic subtests, see
> > > "test/i915_pm_rpm: Remove igt_describe() from dynamic subtest"
> > > 
> > > ./i915_pm_rpm --run gem-execbuf-stress
> > > [cut]
> > > Starting subtest: gem-execbuf-stress
> > > documenting dynamic subsubtests is impossible, document the subtest instead.please refer to lib/igt_core documentation
> > > i915_pm_rpm: ../lib/igt_core.c:365: internal_assert: Assertion `0' failed.
> > > Received signal SIGABRT.
> > 
> > Eh, what a pity this is not detected on compile time. Unfortunately this
> > cannot be easily catched if test requirements are not met and test will
> > skip. So you're right, using HAX is bad idea.
> > 
> > Btw shouldn't igt_describe() be no-op with warning in such case instead
> > of asserting?
> 
> 
> We decided to make that assert loudly to catch dead code. Or rather,
> dead documentation that never reaches docs.
> 
> A way to catch those is to build the docs locally
> 
>    $ ninja -C build igt-gpu-tools-doc
> 
> and check the output. That would also preemptively avoid the review
> round that just points out incorrect use of multiline strings where a
> space is missing between the lines.
> 
> In general people should do some testing locally on their code before
> they send patches.
>

Agree, author as well as reviewer should test change locally. I was not
careful enough proposing HAX with igt_describe(). I wasn't aware that
for dynamic subtests it will assert. Anyway building docs is good tip
to catch this locally. 

--
Zbigniew
 
> -- 
> Petri Latvala
> 
> 
> 
> > 
> > --
> > Zbigniew
> > 
> > > 
> > > That patch deleted following lines "> -":
> > > 
> > > >   	igt_subtest_with_dynamic("gem-execbuf-stress") {
> > > >   		for_each_memory_region(r, drm_fd) {
> > > > -			igt_describe("Validate execbuf submission while exercising rpm "
> > > > -				     "suspend/resume cycles.");
> > > >   			igt_dynamic_f("%s", r->name)
> > > >   				gem_execbuf_stress_subtest(rounds, WAIT_STATUS, &r->ci);
> > > > -			igt_describe("Validate execbuf submission while exercising rpm "
> > > > -				     "suspend/resume cycles with extra wait.");
> > > >   			igt_dynamic_f("%s-%s", "extra-wait", r->name)
> > > >   				gem_execbuf_stress_subtest(rounds, WAIT_STATUS | WAIT_EXTRA, &r->ci);
> > > >   		}
> > > 
> > > This fail will not be catched when we will use HAX.
> > > 
> > > --
> > > Kamil
> > >