Bug 59269

Summary: JSONTestResult needs to handle multiple results
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dpranke, eric, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 59272    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Adam Barth 2011-04-22 19:49:20 PDT
JSONTestResult needs to handle multiple results
Comment 1 Adam Barth 2011-04-22 19:50:23 PDT
Created attachment 90832 [details]
Patch
Comment 2 Adam Barth 2011-04-22 20:24:45 PDT
Created attachment 90837 [details]
Patch
Comment 3 Eric Seidel (no email) 2011-04-22 20:36:07 PDT
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". :(
Comment 4 Eric Seidel (no email) 2011-04-22 20:37:34 PDT
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?
Comment 5 Ojan Vafai 2011-04-22 20:45:36 PDT
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.
Comment 6 Eric Seidel (no email) 2011-04-22 20:46:43 PDT
Adam must be seeing these results or he wouldn't have added the support, or?
Comment 7 Eric Seidel (no email) 2011-04-22 20:48:01 PDT
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.
Comment 8 Adam Barth 2011-04-22 20:51:06 PDT
(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.
Comment 9 Ojan Vafai 2011-04-22 20:53:40 PDT
(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!
Comment 10 Dirk Pranke 2011-04-26 15:49:35 PDT
patch LGTM.
Comment 11 Eric Seidel (no email) 2011-04-26 18:27:04 PDT
Comment on attachment 90837 [details]
Patch

r- per my comments above.  This just needs one round of cleanup.
Comment 12 Adam Barth 2011-05-01 22:42:04 PDT
Created attachment 91884 [details]
Patch
Comment 13 Eric Seidel (no email) 2011-05-01 22:52:43 PDT
Comment on attachment 91884 [details]
Patch

OK.
Comment 14 WebKit Commit Bot 2011-05-01 23:15:08 PDT
Comment on attachment 91884 [details]
Patch

Clearing flags on attachment: 91884

Committed r85459: <http://trac.webkit.org/changeset/85459>
Comment 15 WebKit Commit Bot 2011-05-01 23:15:13 PDT
All reviewed patches have been landed.  Closing bug.