* [igt-dev] [RFC] Use HAX for series which add igt_describe @ 2022-03-03 6:09 Zbigniew Kempczyński 2022-03-03 9:40 ` Kamil Konieczny 0 siblings, 1 reply; 7+ messages in thread From: Zbigniew Kempczyński @ 2022-03-03 6:09 UTC (permalink / raw) To: igt-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [RFC] Use HAX for series which add igt_describe 2022-03-03 6:09 [igt-dev] [RFC] Use HAX for series which add igt_describe Zbigniew Kempczyński @ 2022-03-03 9:40 ` Kamil Konieczny 2022-03-03 11:58 ` Zbigniew Kempczyński 0 siblings, 1 reply; 7+ messages in thread From: Kamil Konieczny @ 2022-03-03 9:40 UTC (permalink / raw) To: igt-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [RFC] Use HAX for series which add igt_describe 2022-03-03 9:40 ` Kamil Konieczny @ 2022-03-03 11:58 ` Zbigniew Kempczyński 2022-03-17 15:48 ` Kamil Konieczny 0 siblings, 1 reply; 7+ messages in thread From: Zbigniew Kempczyński @ 2022-03-03 11:58 UTC (permalink / raw) To: Kamil Konieczny, igt-dev, Petri Latvala 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [RFC] Use HAX for series which add igt_describe 2022-03-03 11:58 ` Zbigniew Kempczyński @ 2022-03-17 15:48 ` Kamil Konieczny 2022-03-18 18:12 ` Zbigniew Kempczyński 0 siblings, 1 reply; 7+ messages in thread From: Kamil Konieczny @ 2022-03-17 15:48 UTC (permalink / raw) To: igt-dev 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [RFC] Use HAX for series which add igt_describe 2022-03-17 15:48 ` Kamil Konieczny @ 2022-03-18 18:12 ` Zbigniew Kempczyński 2022-03-21 9:26 ` Petri Latvala 0 siblings, 1 reply; 7+ messages in thread From: Zbigniew Kempczyński @ 2022-03-18 18:12 UTC (permalink / raw) To: Kamil Konieczny, igt-dev 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [RFC] Use HAX for series which add igt_describe 2022-03-18 18:12 ` Zbigniew Kempczyński @ 2022-03-21 9:26 ` Petri Latvala 2022-03-22 7:22 ` Zbigniew Kempczyński 0 siblings, 1 reply; 7+ messages in thread From: Petri Latvala @ 2022-03-21 9:26 UTC (permalink / raw) To: Zbigniew Kempczyński; +Cc: igt-dev 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [RFC] Use HAX for series which add igt_describe 2022-03-21 9:26 ` Petri Latvala @ 2022-03-22 7:22 ` Zbigniew Kempczyński 0 siblings, 0 replies; 7+ messages in thread From: Zbigniew Kempczyński @ 2022-03-22 7:22 UTC (permalink / raw) To: Petri Latvala; +Cc: igt-dev 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-22 7:22 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-03 6:09 [igt-dev] [RFC] Use HAX for series which add igt_describe Zbigniew Kempczyński 2022-03-03 9:40 ` Kamil Konieczny 2022-03-03 11:58 ` Zbigniew Kempczyński 2022-03-17 15:48 ` Kamil Konieczny 2022-03-18 18:12 ` Zbigniew Kempczyński 2022-03-21 9:26 ` Petri Latvala 2022-03-22 7:22 ` Zbigniew Kempczyński
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.