checkasm: Allow ignoring certain test groups

Message ID 20161022105136.20307-1-martin@martin.st
State Superseded
Headers show

Commit Message

Martin Storsjö Oct. 22, 2016, 10:51 a.m.
The test groups to be skipped can be specified via the --ignore-tests
parameter to configure as checkasm.groupname. I.e.,
./configure --ignore-tests="filter-hqdn3d checkasm.vp8dsp checkasm.h264idct".

This allows keeping running the rest of the checkasm tests, even if
one test is known to be broken, instead of skipping all of checkasm.
---
This goes on top of Diego's patch for skipping/ignoring certain
fate tests.
---
 tests/checkasm/checkasm.c | 43 +++++++++++++++++++++++++++++++++----------
 tests/fate/checkasm.mak   |  2 +-
 2 files changed, 34 insertions(+), 11 deletions(-)

Comments

Janne Grunau Oct. 22, 2016, 11:34 a.m. | #1
On 2016-10-22 13:51:36 +0300, Martin Storsjö wrote:
> The test groups to be skipped can be specified via the --ignore-tests
> parameter to configure as checkasm.groupname. I.e.,
> ./configure --ignore-tests="filter-hqdn3d checkasm.vp8dsp checkasm.h264idct".
> 
> This allows keeping running the rest of the checkasm tests, even if
> one test is known to be broken, instead of skipping all of checkasm.
> ---
> This goes on top of Diego's patch for skipping/ignoring certain
> fate tests.
> ---
>  tests/checkasm/checkasm.c | 43 +++++++++++++++++++++++++++++++++----------
>  tests/fate/checkasm.mak   |  2 +-
>  2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
> index 040c4eb..da3e5e2 100644
> --- a/tests/checkasm/checkasm.c
> +++ b/tests/checkasm/checkasm.c
> @@ -179,6 +179,7 @@ static struct {
>      int nop_time;
>      int cpu_flag;
>      const char *cpu_flag_name;
> +    const char *ignore_pattern;
>  } state;
>  
>  /* PRNG state */
> @@ -455,6 +456,21 @@ static CheckasmFunc *get_func(CheckasmFunc **root, const char *name)
>      return f;
>  }
>  
> +static int test_ignored(const char *name)
> +{
> +    const char *start = state.ignore_pattern, *match;
> +    int len = strlen(name);
> +    if (!start)
> +        return 0;
> +    while ((match = strstr(start, name)) != NULL) {
> +        if ((match == state.ignore_pattern || match[-1] == ' ') &&
> +            (match[len] == '\0' || match[len] == ' '))
> +            return 1;
> +        start = match + 1;
> +    }
> +    return 0;
> +}
> +
>  /* Perform tests and benchmarks for the specified cpu flag if supported by the host */
>  static void check_cpu_flag(const char *name, int flag)
>  {
> @@ -469,6 +485,8 @@ static void check_cpu_flag(const char *name, int flag)
>  
>          state.cpu_flag_name = name;
>          for (i = 0; tests[i].func; i++) {
> +            if (test_ignored(tests[i].name))
> +                continue;
>              state.current_test_name = tests[i].name;
>              tests[i].func();
>          }
> @@ -486,7 +504,7 @@ static void print_cpu_name(void)
>  
>  int main(int argc, char *argv[])
>  {
> -    unsigned int seed;
> +    unsigned int seed = av_get_random_seed();
>      int i, ret = 0;
>  
>  #if ARCH_ARM && HAVE_ARMV5TE_EXTERNAL
> @@ -499,22 +517,27 @@ int main(int argc, char *argv[])
>          return 0;
>      }
>  
> -    if (argc > 1 && !strncmp(argv[1], "--bench", 7)) {
> +    while (argc > 1) {
> +        if (!strncmp(argv[1], "--bench", 7)) {
>  #ifndef AV_READ_TIME
> -        fprintf(stderr, "checkasm: --bench is not supported on your system\n");
> -        return 1;
> +            fprintf(stderr, "checkasm: --bench is not supported on your system\n");
> +            return 1;
>  #endif
> -        if (argv[1][7] == '=') {
> -            state.bench_pattern = argv[1] + 8;
> -            state.bench_pattern_len = strlen(state.bench_pattern);
> -        } else
> -            state.bench_pattern = "";
> +            if (argv[1][7] == '=') {
> +                state.bench_pattern = argv[1] + 8;
> +                state.bench_pattern_len = strlen(state.bench_pattern);
> +            } else
> +                state.bench_pattern = "";
> +        } else if (!strncmp(argv[1], "--ignore=", 9)) {
> +            state.ignore_pattern = argv[1] + 9;
> +        } else {
> +            seed = strtoul(argv[1], NULL, 10);
> +        }
>  
>          argc--;
>          argv++;
>      }
>  
> -    seed = (argc > 1) ? strtoul(argv[1], NULL, 10) : av_get_random_seed();
>      fprintf(stderr, "checkasm: using random seed %u\n", seed);
>      av_lfg_init(&checkasm_lfg, seed);
>  
> diff --git a/tests/fate/checkasm.mak b/tests/fate/checkasm.mak
> index daefe69..1ce3b11 100644
> --- a/tests/fate/checkasm.mak
> +++ b/tests/fate/checkasm.mak
> @@ -1,5 +1,5 @@
>  fate-checkasm: tests/checkasm/checkasm$(EXESUF)
> -fate-checkasm: CMD = run tests/checkasm/checkasm
> +fate-checkasm: CMD = run tests/checkasm/checkasm --ignore="$(subst checkasm.,,$(filter checkasm.%,$(IGNORE_TESTS)))"
>  fate-checkasm: REF = /dev/null
>  
>  FATE += fate-checkasm

ok, although I'm still undecided if ignoring failing tests completly is 
a good idea (the test will never again run that config). But I can see 
that just ignoring test results might cause problems too. in checkasm 
for example tests that segfault.

Janne
Diego Biurrun Oct. 22, 2016, 2:04 p.m. | #2
On Sat, Oct 22, 2016 at 01:51:36PM +0300, Martin Storsjö wrote:
> The test groups to be skipped can be specified via the --ignore-tests
> parameter to configure as checkasm.groupname. I.e.,
> ./configure --ignore-tests="filter-hqdn3d checkasm.vp8dsp checkasm.h264idct".
> 
> This allows keeping running the rest of the checkasm tests, even if
> one test is known to be broken, instead of skipping all of checkasm.
> ---
> This goes on top of Diego's patch for skipping/ignoring certain
> fate tests.
> ---
>  tests/checkasm/checkasm.c | 43 +++++++++++++++++++++++++++++++++----------
>  tests/fate/checkasm.mak   |  2 +-
>  2 files changed, 34 insertions(+), 11 deletions(-)

Hmmmmm.....

So, I've thought about the checkasm tests in FATE before (that stuff was
added while I was away). I fixed some of the little oddments already.
However, the main problem I see is that it is run as one big monolithic
test. I think each test (group at least) should be scheduled individually
through Make, i.e. fate-checkasm-vp8dsp, fate-checkasm-audiodsp, .. instead
of just fate-checkasm. So instead of picking out certain tests to skip in
the big block of tests, we should split the big block into individual tests.
Once they are split, we can ignore individual checkasm tests just like any
other fate test with no special code in checkasm required.

Diego
Martin Storsjö Oct. 22, 2016, 5:59 p.m. | #3
On Sat, 22 Oct 2016, Janne Grunau wrote:

> On 2016-10-22 13:51:36 +0300, Martin Storsjö wrote:
>> The test groups to be skipped can be specified via the --ignore-tests
>> parameter to configure as checkasm.groupname. I.e.,
>> ./configure --ignore-tests="filter-hqdn3d checkasm.vp8dsp checkasm.h264idct".
>>
>> This allows keeping running the rest of the checkasm tests, even if
>> one test is known to be broken, instead of skipping all of checkasm.
>> ---
>> This goes on top of Diego's patch for skipping/ignoring certain
>> fate tests.
>> ---
>>  tests/checkasm/checkasm.c | 43 +++++++++++++++++++++++++++++++++----------
>>  tests/fate/checkasm.mak   |  2 +-
>>  2 files changed, 34 insertions(+), 11 deletions(-)
>>
>> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
>> index 040c4eb..da3e5e2 100644
>> --- a/tests/checkasm/checkasm.c
>> +++ b/tests/checkasm/checkasm.c
>> @@ -179,6 +179,7 @@ static struct {
>>      int nop_time;
>>      int cpu_flag;
>>      const char *cpu_flag_name;
>> +    const char *ignore_pattern;
>>  } state;
>>
>>  /* PRNG state */
>> @@ -455,6 +456,21 @@ static CheckasmFunc *get_func(CheckasmFunc **root, const char *name)
>>      return f;
>>  }
>>
>> +static int test_ignored(const char *name)
>> +{
>> +    const char *start = state.ignore_pattern, *match;
>> +    int len = strlen(name);
>> +    if (!start)
>> +        return 0;
>> +    while ((match = strstr(start, name)) != NULL) {
>> +        if ((match == state.ignore_pattern || match[-1] == ' ') &&
>> +            (match[len] == '\0' || match[len] == ' '))
>> +            return 1;
>> +        start = match + 1;
>> +    }
>> +    return 0;
>> +}
>> +
>>  /* Perform tests and benchmarks for the specified cpu flag if supported by the host */
>>  static void check_cpu_flag(const char *name, int flag)
>>  {
>> @@ -469,6 +485,8 @@ static void check_cpu_flag(const char *name, int flag)
>>
>>          state.cpu_flag_name = name;
>>          for (i = 0; tests[i].func; i++) {
>> +            if (test_ignored(tests[i].name))
>> +                continue;
>>              state.current_test_name = tests[i].name;
>>              tests[i].func();
>>          }
>> @@ -486,7 +504,7 @@ static void print_cpu_name(void)
>>
>>  int main(int argc, char *argv[])
>>  {
>> -    unsigned int seed;
>> +    unsigned int seed = av_get_random_seed();
>>      int i, ret = 0;
>>
>>  #if ARCH_ARM && HAVE_ARMV5TE_EXTERNAL
>> @@ -499,22 +517,27 @@ int main(int argc, char *argv[])
>>          return 0;
>>      }
>>
>> -    if (argc > 1 && !strncmp(argv[1], "--bench", 7)) {
>> +    while (argc > 1) {
>> +        if (!strncmp(argv[1], "--bench", 7)) {
>>  #ifndef AV_READ_TIME
>> -        fprintf(stderr, "checkasm: --bench is not supported on your system\n");
>> -        return 1;
>> +            fprintf(stderr, "checkasm: --bench is not supported on your system\n");
>> +            return 1;
>>  #endif
>> -        if (argv[1][7] == '=') {
>> -            state.bench_pattern = argv[1] + 8;
>> -            state.bench_pattern_len = strlen(state.bench_pattern);
>> -        } else
>> -            state.bench_pattern = "";
>> +            if (argv[1][7] == '=') {
>> +                state.bench_pattern = argv[1] + 8;
>> +                state.bench_pattern_len = strlen(state.bench_pattern);
>> +            } else
>> +                state.bench_pattern = "";
>> +        } else if (!strncmp(argv[1], "--ignore=", 9)) {
>> +            state.ignore_pattern = argv[1] + 9;
>> +        } else {
>> +            seed = strtoul(argv[1], NULL, 10);
>> +        }
>>
>>          argc--;
>>          argv++;
>>      }
>>
>> -    seed = (argc > 1) ? strtoul(argv[1], NULL, 10) : av_get_random_seed();
>>      fprintf(stderr, "checkasm: using random seed %u\n", seed);
>>      av_lfg_init(&checkasm_lfg, seed);
>>
>> diff --git a/tests/fate/checkasm.mak b/tests/fate/checkasm.mak
>> index daefe69..1ce3b11 100644
>> --- a/tests/fate/checkasm.mak
>> +++ b/tests/fate/checkasm.mak
>> @@ -1,5 +1,5 @@
>>  fate-checkasm: tests/checkasm/checkasm$(EXESUF)
>> -fate-checkasm: CMD = run tests/checkasm/checkasm
>> +fate-checkasm: CMD = run tests/checkasm/checkasm --ignore="$(subst checkasm.,,$(filter checkasm.%,$(IGNORE_TESTS)))"
>>  fate-checkasm: REF = /dev/null
>>
>>  FATE += fate-checkasm
>
> ok, although I'm still undecided if ignoring failing tests completly is
> a good idea (the test will never again run that config). But I can see
> that just ignoring test results might cause problems too. in checkasm
> for example tests that segfault.

Indeed.

Additionally, I'd ideally want to ignore even a smaller subset of tests, 
like vp8dsp.mc.mmxext, but getting even the rest of the tests run is 
better than nothing at all. And this patch wouldn't at least be in the way 
of supporting that later.

// Martin
Martin Storsjö Oct. 22, 2016, 6:05 p.m. | #4
On Sat, 22 Oct 2016, Diego Biurrun wrote:

> On Sat, Oct 22, 2016 at 01:51:36PM +0300, Martin Storsjö wrote:
>> The test groups to be skipped can be specified via the --ignore-tests
>> parameter to configure as checkasm.groupname. I.e.,
>> ./configure --ignore-tests="filter-hqdn3d checkasm.vp8dsp checkasm.h264idct".
>> 
>> This allows keeping running the rest of the checkasm tests, even if
>> one test is known to be broken, instead of skipping all of checkasm.
>> ---
>> This goes on top of Diego's patch for skipping/ignoring certain
>> fate tests.
>> ---
>>  tests/checkasm/checkasm.c | 43 +++++++++++++++++++++++++++++++++----------
>>  tests/fate/checkasm.mak   |  2 +-
>>  2 files changed, 34 insertions(+), 11 deletions(-)
>
> Hmmmmm.....
>
> So, I've thought about the checkasm tests in FATE before (that stuff was
> added while I was away). I fixed some of the little oddments already.
> However, the main problem I see is that it is run as one big monolithic
> test. I think each test (group at least) should be scheduled individually
> through Make, i.e. fate-checkasm-vp8dsp, fate-checkasm-audiodsp, .. instead
> of just fate-checkasm. So instead of picking out certain tests to skip in
> the big block of tests, we should split the big block into individual tests.
> Once they are split, we can ignore individual checkasm tests just like any
> other fate test with no special code in checkasm required.

I can certainly see the value in that.

How much I'd like the actual patches doing that is a different story - I 
kinda like checkasm as it is right now. What I think about it I guess 
remains to be seen if such patches are posted.

I can hold off of this one for now, in case you want to pursue that 
direction; getting the generic test ignoring mechanism will/would be a 
great first step at least.

// Martin
Diego Biurrun Oct. 25, 2016, 12:49 p.m. | #5
On Sat, Oct 22, 2016 at 09:05:22PM +0300, Martin Storsjö wrote:
> On Sat, 22 Oct 2016, Diego Biurrun wrote:
> >On Sat, Oct 22, 2016 at 01:51:36PM +0300, Martin Storsjö wrote:
> >>The test groups to be skipped can be specified via the --ignore-tests
> >>parameter to configure as checkasm.groupname. I.e.,
> >>./configure --ignore-tests="filter-hqdn3d checkasm.vp8dsp checkasm.h264idct".
> >>
> >>This allows keeping running the rest of the checkasm tests, even if
> >>one test is known to be broken, instead of skipping all of checkasm.
> >>---
> >>This goes on top of Diego's patch for skipping/ignoring certain
> >>fate tests.
> >>---
> >> tests/checkasm/checkasm.c | 43 +++++++++++++++++++++++++++++++++----------
> >> tests/fate/checkasm.mak   |  2 +-
> >> 2 files changed, 34 insertions(+), 11 deletions(-)
> >
> >Hmmmmm.....
> >
> >So, I've thought about the checkasm tests in FATE before (that stuff was
> >added while I was away). I fixed some of the little oddments already.
> >However, the main problem I see is that it is run as one big monolithic
> >test. I think each test (group at least) should be scheduled individually
> >through Make, i.e. fate-checkasm-vp8dsp, fate-checkasm-audiodsp, .. instead
> >of just fate-checkasm. So instead of picking out certain tests to skip in
> >the big block of tests, we should split the big block into individual tests.
> >Once they are split, we can ignore individual checkasm tests just like any
> >other fate test with no special code in checkasm required.
> 
> I can certainly see the value in that.
> 
> How much I'd like the actual patches doing that is a different story - I
> kinda like checkasm as it is right now. What I think about it I guess
> remains to be seen if such patches are posted.
> 
> I can hold off of this one for now, in case you want to pursue that
> direction; getting the generic test ignoring mechanism will/would be a great
> first step at least.

Pester me in a week or two if I did not get to it until then.

Diego

Patch

diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
index 040c4eb..da3e5e2 100644
--- a/tests/checkasm/checkasm.c
+++ b/tests/checkasm/checkasm.c
@@ -179,6 +179,7 @@  static struct {
     int nop_time;
     int cpu_flag;
     const char *cpu_flag_name;
+    const char *ignore_pattern;
 } state;
 
 /* PRNG state */
@@ -455,6 +456,21 @@  static CheckasmFunc *get_func(CheckasmFunc **root, const char *name)
     return f;
 }
 
+static int test_ignored(const char *name)
+{
+    const char *start = state.ignore_pattern, *match;
+    int len = strlen(name);
+    if (!start)
+        return 0;
+    while ((match = strstr(start, name)) != NULL) {
+        if ((match == state.ignore_pattern || match[-1] == ' ') &&
+            (match[len] == '\0' || match[len] == ' '))
+            return 1;
+        start = match + 1;
+    }
+    return 0;
+}
+
 /* Perform tests and benchmarks for the specified cpu flag if supported by the host */
 static void check_cpu_flag(const char *name, int flag)
 {
@@ -469,6 +485,8 @@  static void check_cpu_flag(const char *name, int flag)
 
         state.cpu_flag_name = name;
         for (i = 0; tests[i].func; i++) {
+            if (test_ignored(tests[i].name))
+                continue;
             state.current_test_name = tests[i].name;
             tests[i].func();
         }
@@ -486,7 +504,7 @@  static void print_cpu_name(void)
 
 int main(int argc, char *argv[])
 {
-    unsigned int seed;
+    unsigned int seed = av_get_random_seed();
     int i, ret = 0;
 
 #if ARCH_ARM && HAVE_ARMV5TE_EXTERNAL
@@ -499,22 +517,27 @@  int main(int argc, char *argv[])
         return 0;
     }
 
-    if (argc > 1 && !strncmp(argv[1], "--bench", 7)) {
+    while (argc > 1) {
+        if (!strncmp(argv[1], "--bench", 7)) {
 #ifndef AV_READ_TIME
-        fprintf(stderr, "checkasm: --bench is not supported on your system\n");
-        return 1;
+            fprintf(stderr, "checkasm: --bench is not supported on your system\n");
+            return 1;
 #endif
-        if (argv[1][7] == '=') {
-            state.bench_pattern = argv[1] + 8;
-            state.bench_pattern_len = strlen(state.bench_pattern);
-        } else
-            state.bench_pattern = "";
+            if (argv[1][7] == '=') {
+                state.bench_pattern = argv[1] + 8;
+                state.bench_pattern_len = strlen(state.bench_pattern);
+            } else
+                state.bench_pattern = "";
+        } else if (!strncmp(argv[1], "--ignore=", 9)) {
+            state.ignore_pattern = argv[1] + 9;
+        } else {
+            seed = strtoul(argv[1], NULL, 10);
+        }
 
         argc--;
         argv++;
     }
 
-    seed = (argc > 1) ? strtoul(argv[1], NULL, 10) : av_get_random_seed();
     fprintf(stderr, "checkasm: using random seed %u\n", seed);
     av_lfg_init(&checkasm_lfg, seed);
 
diff --git a/tests/fate/checkasm.mak b/tests/fate/checkasm.mak
index daefe69..1ce3b11 100644
--- a/tests/fate/checkasm.mak
+++ b/tests/fate/checkasm.mak
@@ -1,5 +1,5 @@ 
 fate-checkasm: tests/checkasm/checkasm$(EXESUF)
-fate-checkasm: CMD = run tests/checkasm/checkasm
+fate-checkasm: CMD = run tests/checkasm/checkasm --ignore="$(subst checkasm.,,$(filter checkasm.%,$(IGNORE_TESTS)))"
 fate-checkasm: REF = /dev/null
 
 FATE += fate-checkasm