Bug 59269 - JSONTestResult needs to handle multiple results
Summary: JSONTestResult needs to handle multiple results
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 59272
  Show dependency tree / graph
 
Reported: 2011-04-22 19:49 PDT by Adam Barth
Modified: 2011-05-01 23:15 PDT (History)
4 users (show)

See Also:

Attachments
Patch (3.57 KB, patch)
2011-04-22 19:50 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (3.57 KB, patch)
2011-04-22 20:24 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch (3.93 KB, patch)
2011-05-01 22:42 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.