[PATCH] lib: allow igt_skip_on_simulation outside of fixtures.

[PATCH] lib: allow igt_skip_on_simulation outside of fixtures.

[Daniel Vetter]

Thomas noticed that in simulation mode a lot of the tests fall over
instead of skipping properly. This is due to recently added
self-checks which ensure that any call to igt_skip happens either
within a fixture or subtest block (or it's a simple test without
subtests). This is to catch bugs since pretty much always not wrapping
up hardware setup and checks into these blocks is a bug.

Bug simulation skipping is a bit different, so allow that exception.
Otherwise we'd need to fix up piles of tests (and likely need to play
a game of whack-a-mole).

Also add a library testcase for all the different variants to make
sure it really works.

Cc: Thomas Wood <thomas.wood@intel.com>
Cc: Ben Widawsky <benjamin.widawsky@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 lib/igt_core.c         |  12 ++++-
 tests/.gitignore       |   1 +
 tests/Makefile.sources |   1 +
 tests/igt_simulation.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 tests/igt_simulation.c

diff --git a/lib/igt_core.c b/lib/igt_core.c
index bdace83ab382..dbfcd74f9160 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -1117,13 +1117,23 @@ bool igt_run_in_simulation(void)
  *
  * Skip tests when INTEL_SIMULATION environment variable is set. It uses
  * igt_skip() internally and hence is fully subtest aware.
+ *
+ * Note that in contrast to all other functions which use igt_skip() internally
+ * it is allowed to use this outside of an #igt_fixture block in a test with
+ * subtests. This is because in contrast to most other test requirements,
+ * checking for simulation mode doesn't depend upon the present hardware and it
+ * so makes a lot of sense to have this check in the outermost #igt_main block.
  */
 void igt_skip_on_simulation(void)
 {
 	if (igt_only_list_subtests())
 		return;
 
-	igt_require(!igt_run_in_simulation());
+	if (!in_fixture) {
+		igt_fixture
+			igt_require(!igt_run_in_simulation());
+	} else
+		igt_require(!igt_run_in_simulation());
 }
 
 /* structured logging */
diff --git a/tests/.gitignore b/tests/.gitignore
index 623a621ccace..60aa3b497f88 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -107,6 +107,7 @@ igt_list_only
 igt_no_exit
 igt_no_exit_list_only
 igt_no_subtest
+igt_simulation
 kms_addfb
 kms_cursor_crc
 kms_fbc_crc
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 88866ac7ea75..8aeaac0ed15f 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -179,6 +179,7 @@ TESTS_testsuite = \
 	igt_fork_helper \
 	igt_list_only \
 	igt_no_subtest \
+	igt_simulation \
 	$(NULL)
 
 TESTS = \
diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c
new file mode 100644
index 000000000000..b9c6241d12e4
--- /dev/null
+++ b/tests/igt_simulation.c
@@ -0,0 +1,139 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Daniel Vetter <daniel.vetter@ffwll.ch>
+ *
+ */
+
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <sys/types.h>
+
+#include "drmtest.h"
+#include "igt_core.h"
+
+bool simple;
+bool list_subtests;
+bool in_fixture;
+
+char test[] = "test";
+char list[] = "--list-subtests";
+char *argv_list[] = { test, list };
+char *argv_run[] = { test };
+
+static int do_fork(void)
+{
+	int pid, status;
+
+	switch (pid = fork()) {
+	case -1:
+		assert(0);
+	case 0:
+		if (simple) {
+			igt_simple_init();
+
+			igt_skip_on_simulation();
+
+			exit(0);
+		} else {
+			if (list_subtests)
+				igt_subtest_init(2, argv_list);
+			else
+				igt_subtest_init(1, argv_run);
+
+			if (in_fixture) {
+				igt_fixture
+					igt_skip_on_simulation();
+			} else
+				igt_skip_on_simulation();
+
+			igt_subtest("foo")
+				;
+
+			printf("baz\n");
+			igt_exit();
+		}
+	default:
+		while (waitpid(pid, &status, 0) == -1 &&
+		       errno == EINTR)
+			;
+
+		assert(WIFEXITED(status));
+
+		return WEXITSTATUS(status);
+	}
+}
+
+int main(int argc, char **argv)
+{
+	/* simple tests */
+	simple = true;
+	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
+	assert(do_fork() == 77);
+
+	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
+	assert(do_fork() == 0);
+
+	/* subtests, list mode */
+	simple = false;
+	list_subtests = true;
+
+	in_fixture = false;
+	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
+	assert(do_fork() == 0);
+
+	in_fixture = false;
+	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
+	assert(do_fork() == 0);
+
+	in_fixture = true;
+	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
+	assert(do_fork() == 0);
+
+	in_fixture = true;
+	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
+	assert(do_fork() == 0);
+
+	/* subtest, run mode */
+	simple = false;
+	list_subtests = false;
+
+	in_fixture = false;
+	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
+	assert(do_fork() == 77);
+
+	in_fixture = false;
+	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
+	assert(do_fork() == 0);
+
+	in_fixture = true;
+	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
+	assert(do_fork() == 77);
+
+	in_fixture = true;
+	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
+	assert(do_fork() == 0);
+
+
+	return 0;
+}
-- 
1.8.5.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib: allow igt_skip_on_simulation outside of fixtures.

[Ben Widawsky]

On Sat, Mar 22, 2014 at 01:27:07PM +0100, Daniel Vetter wrote:
> Thomas noticed that in simulation mode a lot of the tests fall over
> instead of skipping properly. This is due to recently added
> self-checks which ensure that any call to igt_skip happens either
> within a fixture or subtest block (or it's a simple test without
> subtests). This is to catch bugs since pretty much always not wrapping
> up hardware setup and checks into these blocks is a bug.
> 
> Bug simulation skipping is a bit different, so allow that exception.
  But

> Otherwise we'd need to fix up piles of tests (and likely need to play
> a game of whack-a-mole).
> 
> Also add a library testcase for all the different variants to make
> sure it really works.
> 
> Cc: Thomas Wood <thomas.wood@intel.com>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

You do realize you've just written a test for your test suite? Yo dawg,
I heard you like tests....

I honestly don't know enough about the internals here to do a proper
review in a minute, but sounds fine from the commit msg. Just one
comment below and it's:
Acked-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  lib/igt_core.c         |  12 ++++-
>  tests/.gitignore       |   1 +
>  tests/Makefile.sources |   1 +
>  tests/igt_simulation.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 152 insertions(+), 1 deletion(-)
>  create mode 100644 tests/igt_simulation.c
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index bdace83ab382..dbfcd74f9160 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -1117,13 +1117,23 @@ bool igt_run_in_simulation(void)
>   *
>   * Skip tests when INTEL_SIMULATION environment variable is set. It uses
>   * igt_skip() internally and hence is fully subtest aware.
> + *
> + * Note that in contrast to all other functions which use igt_skip() internally
> + * it is allowed to use this outside of an #igt_fixture block in a test with
> + * subtests. This is because in contrast to most other test requirements,
> + * checking for simulation mode doesn't depend upon the present hardware and it
> + * so makes a lot of sense to have this check in the outermost #igt_main block.
>   */
>  void igt_skip_on_simulation(void)
>  {
>  	if (igt_only_list_subtests())
>  		return;
>  
> -	igt_require(!igt_run_in_simulation());
> +	if (!in_fixture) {
> +		igt_fixture
> +			igt_require(!igt_run_in_simulation());
> +	} else
> +		igt_require(!igt_run_in_simulation());
>  }
>  
>  /* structured logging */
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 623a621ccace..60aa3b497f88 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -107,6 +107,7 @@ igt_list_only
>  igt_no_exit
>  igt_no_exit_list_only
>  igt_no_subtest
> +igt_simulation
>  kms_addfb
>  kms_cursor_crc
>  kms_fbc_crc
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 88866ac7ea75..8aeaac0ed15f 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -179,6 +179,7 @@ TESTS_testsuite = \
>  	igt_fork_helper \
>  	igt_list_only \
>  	igt_no_subtest \
> +	igt_simulation \
>  	$(NULL)
>  
>  TESTS = \
> diff --git a/tests/igt_simulation.c b/tests/igt_simulation.c
> new file mode 100644
> index 000000000000..b9c6241d12e4
> --- /dev/null
> +++ b/tests/igt_simulation.c
> @@ -0,0 +1,139 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Daniel Vetter <daniel.vetter@ffwll.ch>
> + *
> + */
> +
> +#include <stdlib.h>
> +#include <sys/wait.h>
> +#include <sys/types.h>
> +
> +#include "drmtest.h"
> +#include "igt_core.h"
> +
> +bool simple;
> +bool list_subtests;
> +bool in_fixture;
> +
> +char test[] = "test";
> +char list[] = "--list-subtests";
> +char *argv_list[] = { test, list };
> +char *argv_run[] = { test };
> +
> +static int do_fork(void)
> +{
> +	int pid, status;
> +
> +	switch (pid = fork()) {
> +	case -1:
> +		assert(0);
> +	case 0:
> +		if (simple) {
> +			igt_simple_init();
> +
> +			igt_skip_on_simulation();
> +
> +			exit(0);
> +		} else {
> +			if (list_subtests)
> +				igt_subtest_init(2, argv_list);
> +			else
> +				igt_subtest_init(1, argv_run);
> +
> +			if (in_fixture) {
> +				igt_fixture
> +					igt_skip_on_simulation();
> +			} else
> +				igt_skip_on_simulation();
> +
> +			igt_subtest("foo")
> +				;
> +
> +			printf("baz\n");
> +			igt_exit();
> +		}
> +	default:
> +		while (waitpid(pid, &status, 0) == -1 &&
> +		       errno == EINTR)
> +			;
> +
> +		assert(WIFEXITED(status));
> +
> +		return WEXITSTATUS(status);
> +	}
> +}
> +
> +int main(int argc, char **argv)
> +{
> +	/* simple tests */
> +	simple = true;
> +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> +	assert(do_fork() == 77);
> +
> +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> +	assert(do_fork() == 0);
> +
> +	/* subtests, list mode */
> +	simple = false;
> +	list_subtests = true;
> +
> +	in_fixture = false;
> +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> +	assert(do_fork() == 0);
> +
> +	in_fixture = false;
> +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> +	assert(do_fork() == 0);
> +
> +	in_fixture = true;
> +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> +	assert(do_fork() == 0);
> +
> +	in_fixture = true;
> +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> +	assert(do_fork() == 0);
> +
> +	/* subtest, run mode */
> +	simple = false;
> +	list_subtests = false;
> +
> +	in_fixture = false;
> +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> +	assert(do_fork() == 77);
> +
> +	in_fixture = false;
> +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> +	assert(do_fork() == 0);
> +
> +	in_fixture = true;
> +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> +	assert(do_fork() == 77);
> +
> +	in_fixture = true;
> +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> +	assert(do_fork() == 0);
> +

You could have saved some setenvs here and above by doing all the "trues",
then "falses." If you want to keep the same pattern though, it probably
makes sense to dump this into a helper

test(bool fixture, char *sim_env, int retval);

OR
	in_fixture = false;
	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
	assert(do_fork() == 77);
	in_fixture = true;
	assert(do_fork() == 77);

	in_fixture = false;
	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
	assert(do_fork() == 0);
	in_fixture = true;
	assert(do_fork() == 0);


you can even get down to just 2 setenvs by permuting in_fixture, simple, and
list_subtests.


> +
> +	return 0;
> +}
> -- 
> 1.8.5.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] lib: allow igt_skip_on_simulation outside of fixtures.

[Daniel Vetter]

On Sat, Mar 22, 2014 at 10:14:41AM -0700, Ben Widawsky wrote:
> On Sat, Mar 22, 2014 at 01:27:07PM +0100, Daniel Vetter wrote:
> > Thomas noticed that in simulation mode a lot of the tests fall over
> > instead of skipping properly. This is due to recently added
> > self-checks which ensure that any call to igt_skip happens either
> > within a fixture or subtest block (or it's a simple test without
> > subtests). This is to catch bugs since pretty much always not wrapping
> > up hardware setup and checks into these blocks is a bug.
> > 
> > Bug simulation skipping is a bit different, so allow that exception.
>   But
> 
> > Otherwise we'd need to fix up piles of tests (and likely need to play
> > a game of whack-a-mole).
> > 
> > Also add a library testcase for all the different variants to make
> > sure it really works.
> > 
> > Cc: Thomas Wood <thomas.wood@intel.com>
> > Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> You do realize you've just written a test for your test suite? Yo dawg,
> I heard you like tests....

You do realize that this is about number 6 in my set of tests for my test
suite?

I think the proper conclusion is simply that at least occasionally I
should hand in my "C programmer" lincense due to accute incompetence ;-)

~/xorg/intel-gpu-tools$ ls tests/igt_*c
tests/igt_fork_helper.c  tests/igt_no_exit.c            tests/igt_no_subtest.c
tests/igt_list_only.c    tests/igt_no_exit_list_only.c  tests/igt_simulation.c

Fyi you can run the with

$ make check

which is always recommended when you touch some of the library code.

> I honestly don't know enough about the internals here to do a proper
> review in a minute, but sounds fine from the commit msg. Just one
> comment below and it's:
> Acked-by: Ben Widawsky <ben@bwidawsk.net>

Already pushed the patch ...

> > +int main(int argc, char **argv)
> > +{
> > +	/* simple tests */
> > +	simple = true;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 77);
> > +
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	/* subtests, list mode */
> > +	simple = false;
> > +	list_subtests = true;
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	/* subtest, run mode */
> > +	simple = false;
> > +	list_subtests = false;
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 77);
> > +
> > +	in_fixture = false;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> > +	assert(do_fork() == 77);
> > +
> > +	in_fixture = true;
> > +	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> > +	assert(do_fork() == 0);
> > +
> 
> You could have saved some setenvs here and above by doing all the "trues",
> then "falses." If you want to keep the same pattern though, it probably
> makes sense to dump this into a helper
> 
> test(bool fixture, char *sim_env, int retval);
> 
> OR
> 	in_fixture = false;
> 	assert(setenv("INTEL_SIMULATION", "1", 1) == 0);
> 	assert(do_fork() == 77);
> 	in_fixture = true;
> 	assert(do_fork() == 77);
> 
> 	in_fixture = false;
> 	assert(setenv("INTEL_SIMULATION", "0", 1) == 0);
> 	assert(do_fork() == 0);
> 	in_fixture = true;
> 	assert(do_fork() == 0);
> 
> 
> you can even get down to just 2 setenvs by permuting in_fixture, simple, and
> list_subtests.

Yeah, it's not pretty. If I'd go totally ocd I'd do a for_boolean macro
for simple, list_subtests and in_fixture and for the simulation flag. But
my thinking here was that I want to explicitly list all the case, since
it's no totally obvious which should return 77 and which 0. So I've left
it at the very verbose but explicit version above.

After all this is a testcase for a now fairly established (and documented)
piece of igt infrastructure, so I don't think anyone needs to look at this
any time soon (i.e. hopefully not in the next few years).

Also, alread pushed ;-)

Thanks for the comments anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch