Re: [PATCH i-g-t] tests/tools_test: Make sure l3_parity is supported

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 07, 2017 at 09:49:42AM +0300, Abdiel Janulgue wrote:
> Check support before executing test.
> v2: Skip test only if intel_l3_parity tool tells us to skip. (Petri)
> 
> bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101650
> Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@xxxxxxxxxxxxxxx>
> ---
>  tests/tools_test.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/tools_test.c b/tests/tools_test.c
> index ccd165d..ebd4a9d 100644
> --- a/tests/tools_test.c
> +++ b/tests/tools_test.c
> @@ -89,7 +89,8 @@ igt_main
>  		igt_system_cmd(exec_return,
>  			       "../tools/intel_l3_parity -r 0 -b 0 "
>  			       "-s 0 -e");
> -		igt_assert(exec_return == IGT_EXIT_SUCCESS);
> +		igt_skip_on_f(exec_return == IGT_EXIT_SKIP,
> +			      "intel_l3_parity not supported\n");
>

Asserting that exec_return equals IGT_EXIT_SUCCESS would be nice here,
after the skip_on_f.

Btw, use igt_assert_eq for that assert instead of using == so we get
the values in the failure log.


>  		igt_system_cmd(exec_return,
>  			       "../tools/intel_l3_parity -l | "
> @@ -101,13 +102,14 @@ igt_main
>  					       &val);
>  			igt_assert(val == 1);
>  		} else {
> -			igt_fail(IGT_EXIT_FAILURE);
> +			igt_skip("intel_l3_parity not supported\n");
>  		}
>


This part still causes problems that are misreported.

In this pipeline

  #  intel_l3_parity -l | grep whatever

the exit status of the shell is the exit status of grep, not
intel_l3_parity. Not to mention we don't get to see what
intel_l3_parity even printed here.

Make that call to igt_system_cmd just call intel_l3_parity -l, and
check for the "Row blah, Bank blah" string in your check_cmd_return
value function (needs to also be renamed and the comment explaining it
reworded, that makes no sense to me now and will make less sense with
that change). Make sure you account for the log buffer possibly having
more than just that one line, maybe like this:

-- begin untested code --

static bool find_the_line(const char *haystack, void *data)
{
	int *val = data;
	const char *needle = "Row blah etc";

	if (strstr(haystack, needle)) {
		*val = 1;
		return false;
	}

	return true;
}

/* pseudo in the subtest */
igt_system_cmd(exec_return, "path/intel_l3_parity -l");
if (succeeded) {
	igt_log_buffer_inspect(find_the_line, &val);
	igt_assert_eq(val, 1);
}

-- end untested code --

Bonus points for making the find_the_line function more reusable by
having the needle be specifiable.



>  		igt_system_cmd(exec_return,
>  			       "../tools/intel_l3_parity -r 0 -b 0 "
>  			       "-s 0 -e");
> -		igt_assert(exec_return == IGT_EXIT_SUCCESS);
> +		igt_skip_on_f(exec_return == IGT_EXIT_SKIP,
> +			      "intel_l3_parity not supported\n");
>

The "intel_l3_parity not supported" should be reworded everywhere to
state that the particular attempted operation is not supported.

Same here for asserting EXIT_SUCCESS after checking for EXIT_SKIP



-- 
Petri Latvala
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux