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

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

[Zbigniew Kempczyński]

Hi all.

Last time we got number of patches which don't change test logic so
they trigger BAT + IGT runs.

This is imo a waste of time for CI as it doesn't improve anything apart
of better in-test documentation.

As such patches needs to be reviewed I think we could use HAX patch in
which we remove all subtests in tests/intel-ci/fast-feedback.testlist
adding line with:

igt@meta_test@fail-result

Any comments are welcome.
--
Zbigniew

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

[Kamil Konieczny]

Dnia 2022-03-03 at 07:09:36 +0100, Zbigniew Kempczyński napisał(a):
> Hi all.
> 
> Last time we got number of patches which don't change test logic so
> they trigger BAT + IGT runs.
> 
> This is imo a waste of time for CI as it doesn't improve anything apart
> of better in-test documentation.
> 
> As such patches needs to be reviewed I think we could use HAX patch in
> which we remove all subtests in tests/intel-ci/fast-feedback.testlist
> adding line with:
> 
> igt@meta_test@fail-result
> 
> Any comments are welcome.
> --
> Zbigniew

imho good idea, but I assume that this should be done by the person
sending. I wonder (and I suppose it is question to CI team) if it
can be automated based on content of patch ? So no need for additional
HAX and cover-letter ?
One more idea - check tag in subject ? like [... NO-TESTS 1/1] ...

We can start with HAX though.

--
Kamil

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

[Zbigniew Kempczyński]

On Thu, Mar 03, 2022 at 10:40:37AM +0100, Kamil Konieczny wrote:
> Dnia 2022-03-03 at 07:09:36 +0100, Zbigniew Kempczyński napisał(a):
> > Hi all.
> > 
> > Last time we got number of patches which don't change test logic so
> > they trigger BAT + IGT runs.
> > 
> > This is imo a waste of time for CI as it doesn't improve anything apart
> > of better in-test documentation.
> > 
> > As such patches needs to be reviewed I think we could use HAX patch in
> > which we remove all subtests in tests/intel-ci/fast-feedback.testlist
> > adding line with:
> > 
> > igt@meta_test@fail-result
> > 
> > Any comments are welcome.
> > --
> > Zbigniew
> 
> imho good idea, but I assume that this should be done by the person
> sending. I wonder (and I suppose it is question to CI team) if it
> can be automated based on content of patch ? So no need for additional
> HAX and cover-letter ?
> One more idea - check tag in subject ? like [... NO-TESTS 1/1] ...
> 
> We can start with HAX though.

I will accept any solution which would lead to stop unnecessary runs.
Both - HAX or NO-TESTS base on people awereness but I think it is worth
to do this. @Petri - your opinion?

--
Zbigniew

> 
> --
> Kamil

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

[Kamil Konieczny]

Dnia 2022-03-03 at 12:58:05 +0100, Zbigniew Kempczyński napisał(a):
> On Thu, Mar 03, 2022 at 10:40:37AM +0100, Kamil Konieczny wrote:
> > Dnia 2022-03-03 at 07:09:36 +0100, Zbigniew Kempczyński napisał(a):
> > > Hi all.
> > > 
> > > Last time we got number of patches which don't change test logic so
> > > they trigger BAT + IGT runs.
> > > 
> > > This is imo a waste of time for CI as it doesn't improve anything apart
> > > of better in-test documentation.
> > > 
> > > As such patches needs to be reviewed I think we could use HAX patch in
> > > which we remove all subtests in tests/intel-ci/fast-feedback.testlist
> > > adding line with:
> > > 
> > > igt@meta_test@fail-result
> > > 
> > > Any comments are welcome.
> > > --
> > > Zbigniew
> > 
> > imho good idea, but I assume that this should be done by the person
> > sending. I wonder (and I suppose it is question to CI team) if it
> > can be automated based on content of patch ? So no need for additional
> > HAX and cover-letter ?
> > One more idea - check tag in subject ? like [... NO-TESTS 1/1] ...
> > 
> > We can start with HAX though.
> 
> I will accept any solution which would lead to stop unnecessary runs.
> Both - HAX or NO-TESTS base on people awereness but I think it is worth
> to do this. @Petri - your opinion?
> 
> --
> Zbigniew

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.

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

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

[Zbigniew Kempczyński]

On Thu, Mar 17, 2022 at 04:48:23PM +0100, Kamil Konieczny wrote:
> Dnia 2022-03-03 at 12:58:05 +0100, Zbigniew Kempczyński napisał(a):
> > On Thu, Mar 03, 2022 at 10:40:37AM +0100, Kamil Konieczny wrote:
> > > Dnia 2022-03-03 at 07:09:36 +0100, Zbigniew Kempczyński napisał(a):
> > > > Hi all.
> > > > 
> > > > Last time we got number of patches which don't change test logic so
> > > > they trigger BAT + IGT runs.
> > > > 
> > > > This is imo a waste of time for CI as it doesn't improve anything apart
> > > > of better in-test documentation.
> > > > 
> > > > As such patches needs to be reviewed I think we could use HAX patch in
> > > > which we remove all subtests in tests/intel-ci/fast-feedback.testlist
> > > > adding line with:
> > > > 
> > > > igt@meta_test@fail-result
> > > > 
> > > > Any comments are welcome.
> > > > --
> > > > Zbigniew
> > > 
> > > imho good idea, but I assume that this should be done by the person
> > > sending. I wonder (and I suppose it is question to CI team) if it
> > > can be automated based on content of patch ? So no need for additional
> > > HAX and cover-letter ?
> > > One more idea - check tag in subject ? like [... NO-TESTS 1/1] ...
> > > 
> > > We can start with HAX though.
> > 
> > I will accept any solution which would lead to stop unnecessary runs.
> > Both - HAX or NO-TESTS base on people awereness but I think it is worth
> > to do this. @Petri - your opinion?
> > 
> > --
> > Zbigniew
> 
> 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?

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

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

[Petri Latvala]

On Fri, Mar 18, 2022 at 07:12:33PM +0100, Zbigniew Kempczyński wrote:
> On Thu, Mar 17, 2022 at 04:48:23PM +0100, Kamil Konieczny wrote:
> > Dnia 2022-03-03 at 12:58:05 +0100, Zbigniew Kempczyński napisał(a):
> > > On Thu, Mar 03, 2022 at 10:40:37AM +0100, Kamil Konieczny wrote:
> > > > Dnia 2022-03-03 at 07:09:36 +0100, Zbigniew Kempczyński napisał(a):
> > > > > Hi all.
> > > > > 
> > > > > Last time we got number of patches which don't change test logic so
> > > > > they trigger BAT + IGT runs.
> > > > > 
> > > > > This is imo a waste of time for CI as it doesn't improve anything apart
> > > > > of better in-test documentation.
> > > > > 
> > > > > As such patches needs to be reviewed I think we could use HAX patch in
> > > > > which we remove all subtests in tests/intel-ci/fast-feedback.testlist
> > > > > adding line with:
> > > > > 
> > > > > igt@meta_test@fail-result
> > > > > 
> > > > > Any comments are welcome.
> > > > > --
> > > > > Zbigniew
> > > > 
> > > > imho good idea, but I assume that this should be done by the person
> > > > sending. I wonder (and I suppose it is question to CI team) if it
> > > > can be automated based on content of patch ? So no need for additional
> > > > HAX and cover-letter ?
> > > > One more idea - check tag in subject ? like [... NO-TESTS 1/1] ...
> > > > 
> > > > We can start with HAX though.
> > > 
> > > I will accept any solution which would lead to stop unnecessary runs.
> > > Both - HAX or NO-TESTS base on people awereness but I think it is worth
> > > to do this. @Petri - your opinion?
> > > 
> > > --
> > > Zbigniew
> > 
> > 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.

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

Re: [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
> > >