Bug 59269 – JSONTestResult needs to handle multiple results

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

[Adam Barth]

JSONTestResult needs to handle multiple results

[Adam Barth]

Created attachment 90832 [details]
Patch

[Adam Barth]

Created attachment 90837 [details]
Patch

[Eric Seidel (no email)]

So this is unfortunate.  My first version of my patch had support like this... until I asked Ojan if "actual" would ever have multiple values and he said "no". :(

[Eric Seidel (no email)]

Comment on attachment 90837 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:54
>      def _actual_as_expectation(self):

Seems this should be actual_as_expectations now.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser.py:56
> +        return map(test_expectations.TestExpectations.expectation_from_string, actual_results.split(' '))

This will end up putting None in the array if the mapping fails.  Seems we should detect that case and warn.

> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:61
> +            "actual": "PASS TEXT",

What does this even mean?

[Ojan Vafai]

Comment on attachment 90837 [details]
Patch

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

AFAIK, we don't ever put two results for actual in right now. We should though. We just don't right now. It's an easy change to summarize_results to start doing so, so I think supporting this is a good idea.

>> Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py:61
>> +            "actual": "PASS TEXT",
> 
> What does this even mean?

This means it passed during one one and failed text during another.

[Eric Seidel (no email)]

Adam must be seeing these results or he wouldn't have added the support, or?

[Eric Seidel (no email)]

So if we run new-run-webkit-tests foo.html --iterations 100, we'll get 100 values in 'actual'?

Seems currently we'd never see "PASS" before a second result.

[Adam Barth]

(In reply to comment #6)
> Adam must be seeing these results or he wouldn't have added the support, or?

Yes.  I ran into this assert while testing the new hotness.

[Ojan Vafai]

(In reply to comment #8)
> (In reply to comment #6)
> > Adam must be seeing these results or he wouldn't have added the support, or?
> 
> Yes.  I ran into this assert while testing the new hotness.

Oh crap. I was just wrong. Sorry for the misinformation. I didn't believe it, but I just went and reread the code and we do in fact do this already. Whoops!

[Dirk Pranke]

patch LGTM.

[Eric Seidel (no email)]

Comment on attachment 90837 [details]
Patch

r- per my comments above.  This just needs one round of cleanup.

[Adam Barth]

Created attachment 91884 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 91884 [details]
Patch

OK.

[WebKit Commit Bot]

Comment on attachment 91884 [details]
Patch

Clearing flags on attachment: 91884

Committed r85459: <http://trac.webkit.org/changeset/85459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.