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

[Dylan Baker]

Quoting Alejandro Piñeiro (2016-11-10 12:08:23)
> 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.

I guess the other option, which I would be okay with, is to take the Vulkan
specific bits and put them in deqp_vk, that would at least allow us to hide the
different behavior since it's only used by Vulkan.

It is frustrating to me that we have half a dozen deqp suites, and they all
behave differently. I really wish they would have more similar behavior. Sigh.

Dylan

> 
> BR
> 
> 
> 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20161110/8a98b97f/attachment.sig>