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

[Chad Versace]

On 10/07/2013 04:19 PM, Ian Romanick wrote:
> 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. :)

Damn... you caught me...


>> 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.

Introducing an accessor for the config makes sense to me.

>> 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.

Argh, that's not valid syntax anywhere. You can only initialize an array
like above at the point of variable declaration. However, such initialization
is legal in C89.