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

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

[Abdiel Janulgue]

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@intel.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 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");
 
 		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");
 		}
 
 		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");
 
 		/* Check that we can clear remaps */
 		igt_system_cmd(exec_return,
@@ -119,7 +121,7 @@ igt_main
 					       &val);
 			igt_assert(val == 1);
 		} else {
-			igt_fail(IGT_EXIT_FAILURE);
+			igt_skip("intel_l3_parity not supported\n");
 		}
 	}
 
-- 
2.9.3

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

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

[Petri Latvala]

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@intel.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Abdiel Janulgue]

v3: Don't pipe the output of intel_l3_parity, parse it's output
    directly. (Petri)

v2: Check support before executing test.
    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@intel.com>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
---
 tests/tools_test.c | 64 ++++++++++++++++++++++++++++++------------------------
 1 file changed, 36 insertions(+), 28 deletions(-)

diff --git a/tests/tools_test.c b/tests/tools_test.c
index 1baf60b..572d5f4 100644
--- a/tests/tools_test.c
+++ b/tests/tools_test.c
@@ -27,23 +27,26 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 
+struct line_check {
+	bool found;
+	const char *substr;
+};
+
 /**
- * Parse the r-value of a [cmd] string.
+ * Our igt_log_buffer_inspect handler. Checks the output of the
+ * intel_l3_parity tool and returns true if a specific substring
+ * is found.
  */
-static bool check_cmd_return_value(const char *s, void *data)
+static bool check_cmd_return_value(const char *line, void *data)
 {
-	int *val = data;
-	char *cmd, *found;
-	const char *delim = "[cmd]";
-	const int delim_len = strlen(delim);
+	struct line_check *check = data;
 
-	if (!(cmd = strstr(s, delim)))
+	if (!strstr(line, check->substr)) {
+		check->found = false;
 		return false;
+	}
 
-	found = cmd + delim_len + 1;
-	igt_assert(delim_len + strlen(found) < strlen(cmd));
-
-	*val = atoi(found);
+	check->found = true;
 	return true;
 }
 
@@ -57,37 +60,42 @@ 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");
+		igt_assert_eq(exec_return, IGT_EXIT_SUCCESS);
 
-		igt_system_cmd(exec_return,
-			       "../tools/intel_l3_parity -l | "
-			       "grep -c 'Row 0, Bank 0, Subbank 0 "
-			       "is disabled'");
+		igt_system_cmd(exec_return, "../tools/intel_l3_parity -l");
 		if (exec_return == IGT_EXIT_SUCCESS) {
-			int val = -1;
+			struct line_check line;
+			line.substr = "Row 0, Bank 0, Subbank 0 is disabled";
 			igt_log_buffer_inspect(check_cmd_return_value,
-					       &val);
-			igt_assert(val == 1);
-		} else {
-			igt_fail(IGT_EXIT_FAILURE);
+					       &line);
+			igt_assert_eq(line.found, true);
 		}
 
 		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");
+		igt_assert_eq(exec_return, IGT_EXIT_SUCCESS);
 
-		/* Check that we can clear remaps */
+		/* Check that we can clear remaps:
+		 * In the original shell script, the output of intel_l3_parity -l
+		 * was piped thru wc -l to check if the tool would at least
+		 * return a line. Just watch for one of the expected output
+		 * string as an alternative.
+		 * ("is disabled" unique only to intel_l3_parity.c:dumpit())
+		 */
 		igt_system_cmd(exec_return,
 			       "../tools/intel_l3_parity -l | "
 			       "wc -l");
 		if (exec_return == IGT_EXIT_SUCCESS) {
-			int val = -1;
+			struct line_check line;
+			line.substr = "is disabled";
 			igt_log_buffer_inspect(check_cmd_return_value,
-					       &val);
-			igt_assert(val == 1);
-		} else {
-			igt_fail(IGT_EXIT_FAILURE);
+					       &line);
+			igt_assert_eq(line.found, true);
 		}
 	}
 
-- 
2.9.3

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

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

[Petri Latvala]

On Wed, Sep 13, 2017 at 12:24:18PM +0300, Abdiel Janulgue wrote:
> v3: Don't pipe the output of intel_l3_parity, parse it's output
>     directly. (Petri)
> 
> v2: Check support before executing test.
>     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@intel.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> ---
>  tests/tools_test.c | 64 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 36 insertions(+), 28 deletions(-)
> 
> diff --git a/tests/tools_test.c b/tests/tools_test.c
> index 1baf60b..572d5f4 100644
> --- a/tests/tools_test.c
> +++ b/tests/tools_test.c
> @@ -27,23 +27,26 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  
> +struct line_check {
> +	bool found;
> +	const char *substr;
> +};
> +
>  /**
> - * Parse the r-value of a [cmd] string.
> + * Our igt_log_buffer_inspect handler. Checks the output of the
> + * intel_l3_parity tool and returns true if a specific substring
> + * is found.


The return value is for the log_buffer_inspect interface, isn't it?
This rather sets line_check::found to true.


>   */
> -static bool check_cmd_return_value(const char *s, void *data)
> +static bool check_cmd_return_value(const char *line, void *data)
>  {
> -	int *val = data;
> -	char *cmd, *found;
> -	const char *delim = "[cmd]";
> -	const int delim_len = strlen(delim);
> +	struct line_check *check = data;
>  
> -	if (!(cmd = strstr(s, delim)))
> +	if (!strstr(line, check->substr)) {
> +		check->found = false;
>  		return false;
> +	}
>  
> -	found = cmd + delim_len + 1;
> -	igt_assert(delim_len + strlen(found) < strlen(cmd));
> -
> -	*val = atoi(found);
> +	check->found = true;
>  	return true;
>  }
>  
> @@ -57,37 +60,42 @@ 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");
> +		igt_assert_eq(exec_return, IGT_EXIT_SUCCESS);
>  
> -		igt_system_cmd(exec_return,
> -			       "../tools/intel_l3_parity -l | "
> -			       "grep -c 'Row 0, Bank 0, Subbank 0 "
> -			       "is disabled'");
> +		igt_system_cmd(exec_return, "../tools/intel_l3_parity -l");
>  		if (exec_return == IGT_EXIT_SUCCESS) {
> -			int val = -1;
> +			struct line_check line;
> +			line.substr = "Row 0, Bank 0, Subbank 0 is disabled";
>  			igt_log_buffer_inspect(check_cmd_return_value,
> -					       &val);
> -			igt_assert(val == 1);
> -		} else {
> -			igt_fail(IGT_EXIT_FAILURE);
> +					       &line);
> +			igt_assert_eq(line.found, true);
>  		}
>  
>  		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");
> +		igt_assert_eq(exec_return, IGT_EXIT_SUCCESS);
>  
> -		/* Check that we can clear remaps */
> +		/* Check that we can clear remaps:
> +		 * In the original shell script, the output of intel_l3_parity -l
> +		 * was piped thru wc -l to check if the tool would at least
> +		 * return a line. Just watch for one of the expected output
> +		 * string as an alternative.


This is an excellent change.


> +		 * ("is disabled" unique only to intel_l3_parity.c:dumpit())
> +		 */
>  		igt_system_cmd(exec_return,
>  			       "../tools/intel_l3_parity -l | "
>  			       "wc -l");

...but you still left the piping to wc -l here. :P




With those fixed, this is

Reviewed-by: Petri Latvala <petri.latvala@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Abdiel Janulgue]

On 09/13/2017 01:10 PM, Petri Latvala wrote:
> On Wed, Sep 13, 2017 at 12:24:18PM +0300, Abdiel Janulgue wrote:
>> v3: Don't pipe the output of intel_l3_parity, parse it's output
>>     directly. (Petri)
>>
>> v2: Check support before executing test.
>>     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@intel.com>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
>> ---
>>  tests/tools_test.c | 64 ++++++++++++++++++++++++++++++------------------------
>>  1 file changed, 36 insertions(+), 28 deletions(-)
>>
>> diff --git a/tests/tools_test.c b/tests/tools_test.c
>> index 1baf60b..572d5f4 100644
>> --- a/tests/tools_test.c
>> +++ b/tests/tools_test.c
>> @@ -27,23 +27,26 @@
>>  #include <sys/stat.h>
>>  #include <fcntl.h>
>>  
>> +struct line_check {
>> +	bool found;
>> +	const char *substr;
>> +};
>> +
>>  /**
>> - * Parse the r-value of a [cmd] string.
>> + * Our igt_log_buffer_inspect handler. Checks the output of the
>> + * intel_l3_parity tool and returns true if a specific substring
>> + * is found.
> 
> 
> The return value is for the log_buffer_inspect interface, isn't it?
> This rather sets line_check::found to true.
> 
> 
>>   */
>> -static bool check_cmd_return_value(const char *s, void *data)
>> +static bool check_cmd_return_value(const char *line, void *data)
>>  {
>> -	int *val = data;
>> -	char *cmd, *found;
>> -	const char *delim = "[cmd]";
>> -	const int delim_len = strlen(delim);
>> +	struct line_check *check = data;
>>  
>> -	if (!(cmd = strstr(s, delim)))
>> +	if (!strstr(line, check->substr)) {
>> +		check->found = false;
>>  		return false;
>> +	}
>>  
>> -	found = cmd + delim_len + 1;
>> -	igt_assert(delim_len + strlen(found) < strlen(cmd));
>> -
>> -	*val = atoi(found);
>> +	check->found = true;
>>  	return true;
>>  }
>>  
>> @@ -57,37 +60,42 @@ 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");
>> +		igt_assert_eq(exec_return, IGT_EXIT_SUCCESS);
>>  
>> -		igt_system_cmd(exec_return,
>> -			       "../tools/intel_l3_parity -l | "
>> -			       "grep -c 'Row 0, Bank 0, Subbank 0 "
>> -			       "is disabled'");
>> +		igt_system_cmd(exec_return, "../tools/intel_l3_parity -l");
>>  		if (exec_return == IGT_EXIT_SUCCESS) {
>> -			int val = -1;
>> +			struct line_check line;
>> +			line.substr = "Row 0, Bank 0, Subbank 0 is disabled";
>>  			igt_log_buffer_inspect(check_cmd_return_value,
>> -					       &val);
>> -			igt_assert(val == 1);
>> -		} else {
>> -			igt_fail(IGT_EXIT_FAILURE);
>> +					       &line);
>> +			igt_assert_eq(line.found, true);
>>  		}
>>  
>>  		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");
>> +		igt_assert_eq(exec_return, IGT_EXIT_SUCCESS);
>>  
>> -		/* Check that we can clear remaps */
>> +		/* Check that we can clear remaps:
>> +		 * In the original shell script, the output of intel_l3_parity -l
>> +		 * was piped thru wc -l to check if the tool would at least
>> +		 * return a line. Just watch for one of the expected output
>> +		 * string as an alternative.
> 
> 
> This is an excellent change.
> 
> 
>> +		 * ("is disabled" unique only to intel_l3_parity.c:dumpit())
>> +		 */
>>  		igt_system_cmd(exec_return,
>>  			       "../tools/intel_l3_parity -l | "
>>  			       "wc -l");
> 
> ...but you still left the piping to wc -l here. :P
> 

Oops. Fix coming, thanks for the review!
> 
> 
> 
> With those fixed, this is
> 
> Reviewed-by: Petri Latvala <petri.latvala@intel.com>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx