Bug 59168 – LayoutTestResults should know how to handle NRWT json files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

LayoutTestResults should know how to handle NRWT json files

[Eric Seidel (no email)]

Created attachment 90657 [details]
Patch

[Adam Barth]

Comment on attachment 90657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90657&action=review

> Tools/Scripts/webkitpy/common/net/layouttestresults.py:32
> +from webkitpy.common.net.resultsjsonparser import ResultsJSONParser

This doesn't really belong in net, right?

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:39
> +# FIXME: common should never import from new-run-webkit-tests, one of these files needs to move.
> +from webkitpy.layout_tests.layout_package import json_results_generator, test_expectations, test_results, test_failures

Right.  This file belongs in layout_package.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:35
> +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup

I don't think you need this.

[Adam Barth]

Comment on attachment 90657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90657&action=review

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:82
> +        elif actual == test_expectations.MISSING:
> +            return [test_failures.FailureMissingResult(), test_failures.FailureMissingImageHash(), test_failures.FailureMissingImage()]

This is an AND or an OR?

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:107
> +        return itertools.ifilter(lambda a: a, non_passing_results)

I don't understand what ifilter does.  Can we just use normal filter?

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 90657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90657&action=review
> 
> > Tools/Scripts/webkitpy/common/net/layouttestresults.py:32
> > +from webkitpy.common.net.resultsjsonparser import ResultsJSONParser
> 
> This doesn't really belong in net, right?

Nope.  But we'd have to move LayoutTestResults to layout_tests and although that makes sense on the surface, layout_tests is currently the NRWT tool.  I think we should do that in a separate patch.

> > Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:35
> > +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup
> 
> I don't think you need this.

Good catch.

[Eric Seidel (no email)]

(In reply to comment #3)
> (From update of attachment 90657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90657&action=review
> 
> > Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:82
> > +        elif actual == test_expectations.MISSING:
> > +            return [test_failures.FailureMissingResult(), test_failures.FailureMissingImageHash(), test_failures.FailureMissingImage()]
> 
> This is an AND or an OR?

AND.  the current unexpected_results.json format is "lossy" with regards to the TestFailures data type.

> > Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:107
> > +        return itertools.ifilter(lambda a: a, non_passing_results)
> 
> I don't understand what ifilter does.  Can we just use normal filter?

Yup, will change.

[Eric Seidel (no email)]

Committed r84633: <http://trac.webkit.org/changeset/84633>

[Eric Seidel (no email)]

Ojan, Dirk:   I landed this just now to unblock Adam's work on making the EWS/commit-queue work with NRWT.  But I would still very much like both of your comments on this patch.

[Ojan Vafai]

Comment on attachment 90657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90657&action=review

Looks good to me.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:51
> +        return self._result_dict.get('time_ms')

FWIW, I shortly plan to remove time_ms from full_results.json and put it in a times_ms.json file instead. We can discuss whether that makes sense on that bug though.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:74
> +            return [test_failures.FailureImageHashMismatch(), test_failures.FailureTextMismatch()]

This should have a FIXME to make the test_expectations types 1:1 with TestFailures data type and to make the JSON non-lossy.

>>> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:82
>>> +            return [test_failures.FailureMissingResult(), test_failures.FailureMissingImageHash(), test_failures.FailureMissingImage()]
>> 
>> This is an AND or an OR?
> 
> AND.  the current unexpected_results.json format is "lossy" with regards to the TestFailures data type.

Ditto re: FIXME