[Piglit] [PATCH] framework/backends/json: support non-piglit junit files

[Mark Janes]

Dylan Baker <dylan at pnwbakers.com> writes:

> Quoting Mark Janes (2016-10-26 16:30:12)
>> The junit loader is unnecessarily strict with the input that it
>> accepts.  It expects input generated by piglit, but can be made to
>> handle junit from other test suites like crucible.
>> ---
>>  framework/backends/junit.py | 28 ++++++++++++++++++++--------
>>  1 file changed, 20 insertions(+), 8 deletions(-)
>> 
>> diff --git a/framework/backends/junit.py b/framework/backends/junit.py
>> index 4c9b7af..ab14074 100644
>> --- a/framework/backends/junit.py
>> +++ b/framework/backends/junit.py
>> @@ -361,14 +361,17 @@ def _load(results_file):
>>      else:
>>          run_result.name = 'junit result'
>>  
>> -    tree = etree.parse(results_file).getroot().find('.//testsuite[@name="piglit"]')
>> +    tree = etree.parse(results_file).getroot().find('.//testsuite')
>>      for test in tree.iterfind('testcase'):
>>          result = results.TestResult()
>>          # Take the class name minus the 'piglit.' element, replace junit's '.'
>>          # separator with piglit's separator, and join the group and test names
>> -        name = test.attrib['classname'].split('.', 1)[1]
>> +        name = test.attrib['name']
>> +        if 'classname' in test.attrib:
>> +            name = grouptools.join(test.attrib['classname'], name)
>>          name = name.replace('.', grouptools.SEPARATOR)
>> -        name = grouptools.join(name, test.attrib['name'])
>> +        if name.startswith("piglit"):
>> +            name = name.split(grouptools.SEPARATOR, 1)[1]
>
> I assumed that grouptools.split could do this, but apparently not.
>
>>  
>>          # Remove the trailing _ if they were added (such as to api and search)
>>          if name.endswith('_'):
>> @@ -378,14 +381,23 @@ def _load(results_file):
>>  
>>          # This is the fallback path, we'll try to overwrite this with the value
>>          # in stderr
>> -        result.time = results.TimeAttribute(end=float(test.attrib['time']))
>> -        result.err = test.find('system-err').text
>> +        result.time = results.TimeAttribute()
>> +        if 'time' in test.attrib:
>> +            result.time = results.TimeAttribute(end=float(test.attrib['time']))
>> +        result.err = ""
>
> result.err is initialized to "", so no need to set it.
>
>> +        syserr = test.find('system-err')
>> +        if syserr is not None:
>> +            result.err = syserr.text
>     
>>  
>>          # The command is prepended to system-out, so we need to separate those
>>          # into two separate elements
>> -        out = test.find('system-out').text.split('\n')
>> -        result.command = out[0]
>> -        result.out = '\n'.join(out[1:])
>> +        out_tag = test.find('system-out')
>
>> +        result.out = ""
>> +        result.command = ""
>
> These are initialized to "" as well.
>
>> +        if out_tag is not None:
>> +            out = out_tag.text.split('\n')
>> +            result.command = out[0]
>
> This isn't right, if the suite generates stdout it is assumed that the command
> will be the first line, but that isn't true unless piglit generates it. We
> probably need to do like we do we do with stderr and have a "command: ..." and
> search for that.

You are right.  We could use the earlier check for piglit as the first
component in the name to conditionally take the first line as the
command.  Is that acceptable?

Other suites will have an empty command in the field, which is still
usable.

>> +            result.out = '\n'.join(out[1:])
>>  
>>          # Try to get the values in stderr for time and pid
>>          for line in result.err.split('\n'):
>> -- 
>> 2.9.3
>> 
>
> Dylan