[Piglit] [PATCH v2] framework: Add mustpass option for the Vulkan CTS test runner

[Alejandro Piñeiro]

On 10/11/16 19:05, Dylan Baker wrote:
> Quoting Alejandro Piñeiro (2016-11-10 07:01:03)
>> This option was added mostly to being able to run the mustpass
>> case list included on Vulkan CTS repository. As that file is in
>> txt format, we assume that the case list will be in txt format.
>>
>> That is not the case for the other deqp-xxx profiles, that uses
>> a xml format. In any case, it is properly documented on the
>> piglit.conf.example
>>
>> v2: allow to receive a txt file with just the name of the test,
>>     without any formatting
>> ---
>>
>> This new version is somewhat more intrusive. It allows to receive
>> a txt file with just the name of the tests. But doing that
>> it removed the ill-format-line check on deqp.make_profile.
>>
>> The advantage of this is that we don't need to transform the
>> mustpass file that cames with the vulkan cts repository.
>>
>> FWIW, deqp-vk also accepts that format. So with this change,
>> piglit just allows to receive the testcase list in both txt formats.
>>
>>  framework/test/deqp.py | 14 ++++++++++----
>>  piglit.conf.example    |  7 +++++++
>>  tests/deqp_vk.py       | 10 +++++++---
>>  3 files changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/framework/test/deqp.py b/framework/test/deqp.py
>> index 5b53efd..a63bea7 100644
>> --- a/framework/test/deqp.py
>> +++ b/framework/test/deqp.py
>> @@ -78,6 +78,14 @@ def select_source(bin_, filename, mustpass, extra_args):
>>          return iter_deqp_test_cases(
>>              gen_caselist_txt(bin_, filename, extra_args))
>>  
> You're missing a newline, it should be two for top level functions.

Ok.

>> +def select_source_txt(bin_, filename, mustpass, extra_args):
>> +    """Return either the mustpass list in txt format or the generated list."""
>> +    if options.OPTIONS.deqp_mustpass:
>> +        return iter_deqp_test_cases(mustpass)
>> +    else:
>> +        return iter_deqp_test_cases(
>> +            gen_caselist_txt(bin_, filename, extra_args))
>> +
> I'm not sure how to do it, but I'm also not thrilled with the near duplication
> of select_source function. I think I'd rather see either a keyword argument that
> passes the function to use for deqp_mustpass or do file extension detection or
> something that reduces the need for another function.

Taking into account that at this moment, this method is only used at
deqp_vk, other option is implement this logic there, without adding a
new method on the framework.

In any case, I'm ok with trying the argument option. What option do you
prefer?

>>  
>>  def make_profile(test_list, test_class):
>>      """Create a TestProfile instance."""
>> @@ -146,10 +154,8 @@ def iter_deqp_test_cases(case_file):
>>                  continue
>>              elif line.startswith('TEST:'):
>>                  yield line[len('TEST:'):].strip()
>> -            else:
>> -                raise exceptions.PiglitFatalError(
>> -                    'deqp: {}:{}: ill-formed line'.format(case_file, i))
>> -
> Don't remove this newline, it was correct.

Ok.

>> +            else: # We assume that the line is just the name of the test
>> +                yield line
> I'm not sure I like this approach, it conflates two different files formats, and
> it creates two functions that are nearly identical. I'll give you that this is
> all kind of a mess, but I think you should create a new generator function for
> the vulkan mustpass list, it's a different format.

Well, the issue here is not about vulkan cts using a different format.
As far as I see all deqp-xxx works in a similar, and somewhat odd way.

The issue is that you can ask deqp to create a test list in a text
format using the option "--deqp-runmode=txt-caselist". Right now, piglit
can parse that text file to execute individually the tests. But funnily
enough deqp can't. So if you try to use the txt file that deqp use on
deqp you get this:

  $ ./deqp-xxx --deqp-runmode
    Writing test cases from 'dEQP-GLES3' to file 'dEQP-XXX-cases.txt'..
  $ ./deqp-xxx --deqp-caselist-file=dEQP-XXX-cases.txt
    ERROR: Failed to parse test case list: Illegal character in test
case name
    FATAL ERROR: Failed to parse command line
    Killed

So, I assume that this is the reason that the vulkan cts repository
saves the mustpass list in a different format.

In any case, going back to this, what I could do is support only the
format Im interested in, and create a new generator as you mention.
Although that would make deqp_vk behaving somewhat different to other
deqp_xxx.

BR