[01/15] tests/util: Add config value to enumerate subtests

Submitted by Dylan Baker on Jan. 23, 2018, 1:22 a.m.

Details

Message ID 6ebe8d2f6215ce9c7522c0683a95de5f6aef21fe.1516670408.git-series.dylan@pnwbakers.com
State New
Headers show
Series "Series without cover letter" ( rev: 3 ) in Piglit

Not browsing as part of any series.

Commit Message

Dylan Baker Jan. 23, 2018, 1:22 a.m.
This adds a new member to the GL config struct for informing the python
framework the number and order of subtests that will be run (if any). If
this value is unset then no subtests are expected.
---
 tests/util/piglit-framework-gl.c | 13 +++++++++++++
 tests/util/piglit-framework-gl.h | 11 +++++++++++
 2 files changed, 24 insertions(+)


base-commit: 736496667329bf73a706aebec6f8287078df79ae

Patch hide | download patch | download mbox

diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
index 1b2078d..1ce5474 100644
--- a/tests/util/piglit-framework-gl.c
+++ b/tests/util/piglit-framework-gl.c
@@ -318,3 +318,16 @@  piglit_get_selected_tests(const char ***selected_subtests)
 	*selected_subtests = gl_fw->test_config->selected_subtests;
 	return gl_fw->test_config->num_selected_subtests;
 }
+
+void
+piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config)
+{
+	if (config->all_subtests) {
+		printf("PIGLIT: {\"enumerate subtests\": [\"%s\"", config->all_subtests[0]);
+		for (int i = 1; config->all_subtests[i]; i++) {
+			printf(", \"%s\"", config->all_subtests[i]);
+		}
+		printf("]}\n");
+		fflush(stdout);
+	}
+}
diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
index 970fd55..d36c4f0 100644
--- a/tests/util/piglit-framework-gl.h
+++ b/tests/util/piglit-framework-gl.h
@@ -213,6 +213,13 @@  struct piglit_gl_test_config {
 	 * enum used for markin test as supporting KHR_no_error or not.
 	 */
 	enum piglit_khr_no_error_support khr_no_error_support;
+
+	/**
+	 * Null terminated list of subtests to be enumerated.
+	 *
+	 * Each subtest *must* be run in the order reported by this list.
+	 */
+	const char **all_subtests;
 };
 
 /**
@@ -246,6 +253,9 @@  void
 piglit_gl_test_run(int argc, char *argv[],
 		   const struct piglit_gl_test_config *config);
 
+void
+piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config);
+
 #ifdef __cplusplus
 #  define PIGLIT_EXTERN_C_BEGIN extern "C" {
 #  define PIGLIT_EXTERN_C_END   }
@@ -287,6 +297,7 @@  piglit_gl_test_run(int argc, char *argv[],
                 }                                                            \
                                                                              \
                 piglit_gl_process_args(&argc, argv, &config);                \
+                piglit_gl_enumerate_subtests(&config);                       \
                 piglit_gl_test_run(argc, argv, &config);                     \
                                                                              \
                 assert(false);                                               \

Comments

Hello!

So after some consideration I think I prefer your first approach
(explicitly calling the enumeration function) after all.

First of all: Sorry for the back and forth on my part and the excess
work you put into this series because of it. Please don't hate me! 🙇

However, I think this approach will be more DRY and less WET.
In particular when used with the existing struct piglit_subtest and
piglit_run_selected_subtests.

I updated the series accordingly (and fixed a type-o in a docstring in
5/15 and the commit message of 6/15 and a small arithmetical error in
11/15).

The result can be found here:
https://github.com/fabianbieler/piglit/tree/enumerate_subtests
If this isn't acceptable for intermediate review, I'll post to the list,
too.

Note that I leveraged piglit_run_selected_subtests for gl-2.1-pbo,
fbo-incomplete, degenerate-prims and linestipple and used
piglit_register_subtests in fbo-storage-formats.

Regards (and sorry again)
Fabian

On 2018-01-23 02:22, Dylan Baker wrote:
> This adds a new member to the GL config struct for informing the python
> framework the number and order of subtests that will be run (if any). If
> this value is unset then no subtests are expected.
> ---
>  tests/util/piglit-framework-gl.c | 13 +++++++++++++
>  tests/util/piglit-framework-gl.h | 11 +++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
> index 1b2078d..1ce5474 100644
> --- a/tests/util/piglit-framework-gl.c
> +++ b/tests/util/piglit-framework-gl.c
> @@ -318,3 +318,16 @@ piglit_get_selected_tests(const char ***selected_subtests)
>  	*selected_subtests = gl_fw->test_config->selected_subtests;
>  	return gl_fw->test_config->num_selected_subtests;
>  }
> +
> +void
> +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config)
> +{
> +	if (config->all_subtests) {
> +		printf("PIGLIT: {\"enumerate subtests\": [\"%s\"", config->all_subtests[0]);
> +		for (int i = 1; config->all_subtests[i]; i++) {
> +			printf(", \"%s\"", config->all_subtests[i]);
> +		}
> +		printf("]}\n");
> +		fflush(stdout);
> +	}
> +}
> diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
> index 970fd55..d36c4f0 100644
> --- a/tests/util/piglit-framework-gl.h
> +++ b/tests/util/piglit-framework-gl.h
> @@ -213,6 +213,13 @@ struct piglit_gl_test_config {
>  	 * enum used for markin test as supporting KHR_no_error or not.
>  	 */
>  	enum piglit_khr_no_error_support khr_no_error_support;
> +
> +	/**
> +	 * Null terminated list of subtests to be enumerated.
> +	 *
> +	 * Each subtest *must* be run in the order reported by this list.
> +	 */
> +	const char **all_subtests;
>  };
>  
>  /**
> @@ -246,6 +253,9 @@ void
>  piglit_gl_test_run(int argc, char *argv[],
>  		   const struct piglit_gl_test_config *config);
>  
> +void
> +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config);
> +
>  #ifdef __cplusplus
>  #  define PIGLIT_EXTERN_C_BEGIN extern "C" {
>  #  define PIGLIT_EXTERN_C_END   }
> @@ -287,6 +297,7 @@ piglit_gl_test_run(int argc, char *argv[],
>                  }                                                            \
>                                                                               \
>                  piglit_gl_process_args(&argc, argv, &config);                \
> +                piglit_gl_enumerate_subtests(&config);                       \
>                  piglit_gl_test_run(argc, argv, &config);                     \
>                                                                               \
>                  assert(false);                                               \
> 
> base-commit: 736496667329bf73a706aebec6f8287078df79ae
>
No worries. I'd rather get this right the first time before we convert 100+
tests than afterwards :)

I also appreciate that you've converted some of the tests to use
piglit_run_selected_subtests. I've lamented with others that it's unfortunate
that we've never really used that feature, since it would be nice for debugging.

I've left you a few comments on github, but we should send it to the list before
we land it so that others have a chance to look at it.

Dylan

Quoting Fabian Bieler (2018-01-23 15:35:32)
> Hello!
> 
> So after some consideration I think I prefer your first approach
> (explicitly calling the enumeration function) after all.
> 
> First of all: Sorry for the back and forth on my part and the excess
> work you put into this series because of it. Please don't hate me! 🙇
> 
> However, I think this approach will be more DRY and less WET.
> In particular when used with the existing struct piglit_subtest and
> piglit_run_selected_subtests.
> 
> I updated the series accordingly (and fixed a type-o in a docstring in
> 5/15 and the commit message of 6/15 and a small arithmetical error in
> 11/15).
> 
> The result can be found here:
> https://github.com/fabianbieler/piglit/tree/enumerate_subtests
> If this isn't acceptable for intermediate review, I'll post to the list,
> too.
> 
> Note that I leveraged piglit_run_selected_subtests for gl-2.1-pbo,
> fbo-incomplete, degenerate-prims and linestipple and used
> piglit_register_subtests in fbo-storage-formats.
> 
> Regards (and sorry again)
> Fabian
>
Fabian,

When you get a chance can you send your updated version of this series to the
list? I think this is probably a good candidate for piecemeal fixes, we can
land the framework bits and the initial tests that are ported, then land the
changes to the rest of the tests in manageable chunks.

Dylan

Quoting Fabian Bieler (2018-01-23 15:35:32)
> Hello!
> 
> So after some consideration I think I prefer your first approach
> (explicitly calling the enumeration function) after all.
> 
> First of all: Sorry for the back and forth on my part and the excess
> work you put into this series because of it. Please don't hate me! 🙇
> 
> However, I think this approach will be more DRY and less WET.
> In particular when used with the existing struct piglit_subtest and
> piglit_run_selected_subtests.
> 
> I updated the series accordingly (and fixed a type-o in a docstring in
> 5/15 and the commit message of 6/15 and a small arithmetical error in
> 11/15).
> 
> The result can be found here:
> https://github.com/fabianbieler/piglit/tree/enumerate_subtests
> If this isn't acceptable for intermediate review, I'll post to the list,
> too.
> 
> Note that I leveraged piglit_run_selected_subtests for gl-2.1-pbo,
> fbo-incomplete, degenerate-prims and linestipple and used
> piglit_register_subtests in fbo-storage-formats.
> 
> Regards (and sorry again)
> Fabian
> 
> On 2018-01-23 02:22, Dylan Baker wrote:
> > This adds a new member to the GL config struct for informing the python
> > framework the number and order of subtests that will be run (if any). If
> > this value is unset then no subtests are expected.
> > ---
> >  tests/util/piglit-framework-gl.c | 13 +++++++++++++
> >  tests/util/piglit-framework-gl.h | 11 +++++++++++
> >  2 files changed, 24 insertions(+)
> > 
> > diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
> > index 1b2078d..1ce5474 100644
> > --- a/tests/util/piglit-framework-gl.c
> > +++ b/tests/util/piglit-framework-gl.c
> > @@ -318,3 +318,16 @@ piglit_get_selected_tests(const char ***selected_subtests)
> >       *selected_subtests = gl_fw->test_config->selected_subtests;
> >       return gl_fw->test_config->num_selected_subtests;
> >  }
> > +
> > +void
> > +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config)
> > +{
> > +     if (config->all_subtests) {
> > +             printf("PIGLIT: {\"enumerate subtests\": [\"%s\"", config->all_subtests[0]);
> > +             for (int i = 1; config->all_subtests[i]; i++) {
> > +                     printf(", \"%s\"", config->all_subtests[i]);
> > +             }
> > +             printf("]}\n");
> > +             fflush(stdout);
> > +     }
> > +}
> > diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
> > index 970fd55..d36c4f0 100644
> > --- a/tests/util/piglit-framework-gl.h
> > +++ b/tests/util/piglit-framework-gl.h
> > @@ -213,6 +213,13 @@ struct piglit_gl_test_config {
> >        * enum used for markin test as supporting KHR_no_error or not.
> >        */
> >       enum piglit_khr_no_error_support khr_no_error_support;
> > +
> > +     /**
> > +      * Null terminated list of subtests to be enumerated.
> > +      *
> > +      * Each subtest *must* be run in the order reported by this list.
> > +      */
> > +     const char **all_subtests;
> >  };
> >  
> >  /**
> > @@ -246,6 +253,9 @@ void
> >  piglit_gl_test_run(int argc, char *argv[],
> >                  const struct piglit_gl_test_config *config);
> >  
> > +void
> > +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config);
> > +
> >  #ifdef __cplusplus
> >  #  define PIGLIT_EXTERN_C_BEGIN extern "C" {
> >  #  define PIGLIT_EXTERN_C_END   }
> > @@ -287,6 +297,7 @@ piglit_gl_test_run(int argc, char *argv[],
> >                  }                                                            \
> >                                                                               \
> >                  piglit_gl_process_args(&argc, argv, &config);                \
> > +                piglit_gl_enumerate_subtests(&config);                       \
> >                  piglit_gl_test_run(argc, argv, &config);                     \
> >                                                                               \
> >                  assert(false);                                               \
> > 
> > base-commit: 736496667329bf73a706aebec6f8287078df79ae
> >
I can resend to the list.
Did we reach an agreement on how piglit_run_selected_subtests should behave?

Should it
enumerate all tests and mark unselected tests PIGLIT_SKIP
or
enumerate only selected tests implicitly marking unselected tests as "not run" when compared to another test run?

I favor the second option as understand PIGLIT_SKIP means "tests ought to be run, but for some reason (missing extension, limited resources, ...) aren't" and not "test shouldn't be run to begin with (e.g. because it's not included in the current test suite)" for which I thought "not run" is the appropriate status.

Fabian

On Fri, Jan 26, 2018, at 6:53 PM, Dylan Baker wrote:
> Fabian,
> 
> When you get a chance can you send your updated version of this series to the
> list? I think this is probably a good candidate for piecemeal fixes, we can
> land the framework bits and the initial tests that are ported, then land the
> changes to the rest of the tests in manageable chunks.
> 
> Dylan
> 
> Quoting Fabian Bieler (2018-01-23 15:35:32)
> > Hello!
> > 
> > So after some consideration I think I prefer your first approach
> > (explicitly calling the enumeration function) after all.
> > 
> > First of all: Sorry for the back and forth on my part and the excess
> > work you put into this series because of it. Please don't hate me! 🙇
> > 
> > However, I think this approach will be more DRY and less WET.
> > In particular when used with the existing struct piglit_subtest and
> > piglit_run_selected_subtests.
> > 
> > I updated the series accordingly (and fixed a type-o in a docstring in
> > 5/15 and the commit message of 6/15 and a small arithmetical error in
> > 11/15).
> > 
> > The result can be found here:
> > https://github.com/fabianbieler/piglit/tree/enumerate_subtests
> > If this isn't acceptable for intermediate review, I'll post to the list,
> > too.
> > 
> > Note that I leveraged piglit_run_selected_subtests for gl-2.1-pbo,
> > fbo-incomplete, degenerate-prims and linestipple and used
> > piglit_register_subtests in fbo-storage-formats.
> > 
> > Regards (and sorry again)
> > Fabian
> > 
> > On 2018-01-23 02:22, Dylan Baker wrote:
> > > This adds a new member to the GL config struct for informing the python
> > > framework the number and order of subtests that will be run (if any). If
> > > this value is unset then no subtests are expected.
> > > ---
> > >  tests/util/piglit-framework-gl.c | 13 +++++++++++++
> > >  tests/util/piglit-framework-gl.h | 11 +++++++++++
> > >  2 files changed, 24 insertions(+)
> > > 
> > > diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
> > > index 1b2078d..1ce5474 100644
> > > --- a/tests/util/piglit-framework-gl.c
> > > +++ b/tests/util/piglit-framework-gl.c
> > > @@ -318,3 +318,16 @@ piglit_get_selected_tests(const char ***selected_subtests)
> > >       *selected_subtests = gl_fw->test_config->selected_subtests;
> > >       return gl_fw->test_config->num_selected_subtests;
> > >  }
> > > +
> > > +void
> > > +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config)
> > > +{
> > > +     if (config->all_subtests) {
> > > +             printf("PIGLIT: {\"enumerate subtests\": [\"%s\"", config->all_subtests[0]);
> > > +             for (int i = 1; config->all_subtests[i]; i++) {
> > > +                     printf(", \"%s\"", config->all_subtests[i]);
> > > +             }
> > > +             printf("]}\n");
> > > +             fflush(stdout);
> > > +     }
> > > +}
> > > diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
> > > index 970fd55..d36c4f0 100644
> > > --- a/tests/util/piglit-framework-gl.h
> > > +++ b/tests/util/piglit-framework-gl.h
> > > @@ -213,6 +213,13 @@ struct piglit_gl_test_config {
> > >        * enum used for markin test as supporting KHR_no_error or not.
> > >        */
> > >       enum piglit_khr_no_error_support khr_no_error_support;
> > > +
> > > +     /**
> > > +      * Null terminated list of subtests to be enumerated.
> > > +      *
> > > +      * Each subtest *must* be run in the order reported by this list.
> > > +      */
> > > +     const char **all_subtests;
> > >  };
> > >  
> > >  /**
> > > @@ -246,6 +253,9 @@ void
> > >  piglit_gl_test_run(int argc, char *argv[],
> > >                  const struct piglit_gl_test_config *config);
> > >  
> > > +void
> > > +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config);
> > > +
> > >  #ifdef __cplusplus
> > >  #  define PIGLIT_EXTERN_C_BEGIN extern "C" {
> > >  #  define PIGLIT_EXTERN_C_END   }
> > > @@ -287,6 +297,7 @@ piglit_gl_test_run(int argc, char *argv[],
> > >                  }                                                            \
> > >                                                                               \
> > >                  piglit_gl_process_args(&argc, argv, &config);                \
> > > +                piglit_gl_enumerate_subtests(&config);                       \
> > >                  piglit_gl_test_run(argc, argv, &config);                     \
> > >                                                                               \
> > >                  assert(false);                                               \
> > > 
> > > base-commit: 736496667329bf73a706aebec6f8287078df79ae
> > > 
> Email had 1 attachment:
> + signature.asc
>   1k (application/pgp-signature)
I can go either way, if you like 2 better then lets go with that.

Quoting Fabian Bieler (2018-01-26 11:33:04)
> I can resend to the list.
> Did we reach an agreement on how piglit_run_selected_subtests should behave?
> 
> Should it
> enumerate all tests and mark unselected tests PIGLIT_SKIP
> or
> enumerate only selected tests implicitly marking unselected tests as "not run" when compared to another test run?
> 
> I favor the second option as understand PIGLIT_SKIP means "tests ought to be run, but for some reason (missing extension, limited resources, ...) aren't" and not "test shouldn't be run to begin with (e.g. because it's not included in the current test suite)" for which I thought "not run" is the appropriate status.
> 
> Fabian
> 
> On Fri, Jan 26, 2018, at 6:53 PM, Dylan Baker wrote:
> > Fabian,
> > 
> > When you get a chance can you send your updated version of this series to the
> > list? I think this is probably a good candidate for piecemeal fixes, we can
> > land the framework bits and the initial tests that are ported, then land the
> > changes to the rest of the tests in manageable chunks.
> > 
> > Dylan
> > 
> > Quoting Fabian Bieler (2018-01-23 15:35:32)
> > > Hello!
> > > 
> > > So after some consideration I think I prefer your first approach
> > > (explicitly calling the enumeration function) after all.
> > > 
> > > First of all: Sorry for the back and forth on my part and the excess
> > > work you put into this series because of it. Please don't hate me! 🙇
> > > 
> > > However, I think this approach will be more DRY and less WET.
> > > In particular when used with the existing struct piglit_subtest and
> > > piglit_run_selected_subtests.
> > > 
> > > I updated the series accordingly (and fixed a type-o in a docstring in
> > > 5/15 and the commit message of 6/15 and a small arithmetical error in
> > > 11/15).
> > > 
> > > The result can be found here:
> > > https://github.com/fabianbieler/piglit/tree/enumerate_subtests
> > > If this isn't acceptable for intermediate review, I'll post to the list,
> > > too.
> > > 
> > > Note that I leveraged piglit_run_selected_subtests for gl-2.1-pbo,
> > > fbo-incomplete, degenerate-prims and linestipple and used
> > > piglit_register_subtests in fbo-storage-formats.
> > > 
> > > Regards (and sorry again)
> > > Fabian
> > > 
> > > On 2018-01-23 02:22, Dylan Baker wrote:
> > > > This adds a new member to the GL config struct for informing the python
> > > > framework the number and order of subtests that will be run (if any). If
> > > > this value is unset then no subtests are expected.
> > > > ---
> > > >  tests/util/piglit-framework-gl.c | 13 +++++++++++++
> > > >  tests/util/piglit-framework-gl.h | 11 +++++++++++
> > > >  2 files changed, 24 insertions(+)
> > > > 
> > > > diff --git a/tests/util/piglit-framework-gl.c b/tests/util/piglit-framework-gl.c
> > > > index 1b2078d..1ce5474 100644
> > > > --- a/tests/util/piglit-framework-gl.c
> > > > +++ b/tests/util/piglit-framework-gl.c
> > > > @@ -318,3 +318,16 @@ piglit_get_selected_tests(const char ***selected_subtests)
> > > >       *selected_subtests = gl_fw->test_config->selected_subtests;
> > > >       return gl_fw->test_config->num_selected_subtests;
> > > >  }
> > > > +
> > > > +void
> > > > +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config)
> > > > +{
> > > > +     if (config->all_subtests) {
> > > > +             printf("PIGLIT: {\"enumerate subtests\": [\"%s\"", config->all_subtests[0]);
> > > > +             for (int i = 1; config->all_subtests[i]; i++) {
> > > > +                     printf(", \"%s\"", config->all_subtests[i]);
> > > > +             }
> > > > +             printf("]}\n");
> > > > +             fflush(stdout);
> > > > +     }
> > > > +}
> > > > diff --git a/tests/util/piglit-framework-gl.h b/tests/util/piglit-framework-gl.h
> > > > index 970fd55..d36c4f0 100644
> > > > --- a/tests/util/piglit-framework-gl.h
> > > > +++ b/tests/util/piglit-framework-gl.h
> > > > @@ -213,6 +213,13 @@ struct piglit_gl_test_config {
> > > >        * enum used for markin test as supporting KHR_no_error or not.
> > > >        */
> > > >       enum piglit_khr_no_error_support khr_no_error_support;
> > > > +
> > > > +     /**
> > > > +      * Null terminated list of subtests to be enumerated.
> > > > +      *
> > > > +      * Each subtest *must* be run in the order reported by this list.
> > > > +      */
> > > > +     const char **all_subtests;
> > > >  };
> > > >  
> > > >  /**
> > > > @@ -246,6 +253,9 @@ void
> > > >  piglit_gl_test_run(int argc, char *argv[],
> > > >                  const struct piglit_gl_test_config *config);
> > > >  
> > > > +void
> > > > +piglit_gl_enumerate_subtests(const struct piglit_gl_test_config *config);
> > > > +
> > > >  #ifdef __cplusplus
> > > >  #  define PIGLIT_EXTERN_C_BEGIN extern "C" {
> > > >  #  define PIGLIT_EXTERN_C_END   }
> > > > @@ -287,6 +297,7 @@ piglit_gl_test_run(int argc, char *argv[],
> > > >                  }                                                            \
> > > >                                                                               \
> > > >                  piglit_gl_process_args(&argc, argv, &config);                \
> > > > +                piglit_gl_enumerate_subtests(&config);                       \
> > > >                  piglit_gl_test_run(argc, argv, &config);                     \
> > > >                                                                               \
> > > >                  assert(false);                                               \
> > > > 
> > > > base-commit: 736496667329bf73a706aebec6f8287078df79ae
> > > > 
> > Email had 1 attachment:
> > + signature.asc
> >   1k (application/pgp-signature)