[Piglit] [PATCH 3/4] util: Add a -list-subtests option that will list all the subtests

[Ian Romanick]

On 10/07/2013 11:02 AM, Chad Versace wrote:
> On 10/04/2013 06:11 PM, Ian Romanick wrote:
>> From: Ian Romanick <ian.d.romanick at intel.com>
>>
>> This required some ugly hacking about to get the list of subtests to
>> process_args.  Suggestions?
> 
> Let's avoid these ugly hacks. Here's my suggestion, which covers patches
> 2 and 3.

My troll bait worked. :)

> Patch 2/4 moves `struct piglit_gl_test_config config` out of main() to
> file scope. Let's avoid making things global; that leads to a slippery
> slope. I fought hard to kill many of Piglit's globals, and I don't welcome
> the arrival of new ones. If at all possible, let's leave it in main()
> where it is.
> 
> To leave 'config' in main(), we need to restructure a few things. First,
> piglit_gl_test_config_init() should no longer parse args; instead, it
> should only
> memset 'config' to 0. That is, let's change its signature from
> 
>     void piglit_gl_test_config_init(int *argc, char *argv[], struct
> piglit_gl_test_config *config)
> 
> to
> 
>     void piglit_gl_test_config_init(struct piglit_gl_test_config *config)
> 
> Move the argparsing (that is, the call to process_args()) into
> piglit_gl_test_run(). That will ensure
> that args are parsed *after* the test author has set all config
> variables in the CONFIG block.
> 
> The CONFIG block in your patch 4/4 should now look like this:
> 
> PIGLIT_GL_TEST_CONFIG_BEGIN
> 
>     config.subtests = subtests;
>     config.supports_gl_compat_version = 10;
>     config.window_visual = PIGLIT_GL_VISUAL_RGB;
> 
> PIGLIT_GL_TEST_CONFIG_END

I like this.  This is the way I originally wanted to structure things,
but there were issues.

> The config is no longer defined with file-scope, so piglit_init() should
> execute subtests
> like this:
>     
>     result = piglit_run_subtests(PIGLIT_SKIP);
> 
> piglit_run_selected_subtests() should access `config->subtests` through
> the global
> "test context object", piglit-framework-gl.c:`struct piglit_gl_framework
> *gl_fw`,
> like this:
> 
>     enum piglit_result
>     piglit_run_selected_subtests(enum piglit_result previous_result)
>     {
>         enum piglit_result result = previous_result;
>         const struct piglit_gl_subtest *subtests gl_fw->config->subtests;
>         const char **selected_subtests = gl_fw->config->selected_subtests;
>        
>         ...
>     }

I have somewhat mixed feelings about this... I liked that the old
piglit_run_selected_subtests was both more primitive (flexible) and more
explicit.  This is the birth of this tool, and I'm not sure how the next
user will want to use it.  Hmm...

A third option is an accessor that lets the test get config or maybe
just the list of selected tests from the implicit test context object.

> At this point, it's no longer necessary for the subtests array to be
> global,
> so you could even do the below if you wanted, but that's really up to
> personal
> taste.
> 
> PIGLIT_GL_TEST_CONFIG_BEGIN
> 
>     config.supports_gl_compat_version = 10;
>     config.window_visual = PIGLIT_GL_VISUAL_RGB;
> 
>     config.subtests = {
>         {
>             "Cannot delete while active",
>             "delete-inactive-only",
>             delete_inactive_only
>         },
>         ...,
>     };
> 
> PIGLIT_GL_TEST_CONFIG_END

I think that's C99 syntax, so we probably can't use it in piglit.