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@xxxxxxxxx>
> Cc: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxx>
> + *
> + */
> +
> +#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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
