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

Adam Barth
Reported 2011-04-22 19:49:20 PDT
JSONTestResult needs to handle multiple results
Attachments
Patch (3.57 KB, patch)
2011-04-22 19:50 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (3.57 KB, patch)
2011-04-22 20:24 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch (3.93 KB, patch)
2011-05-01 22:42 PDT, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2011-04-22 19:50:23 PDT
Created attachment 90832 [details] Patch
Adam Barth
Comment 2 2011-04-22 20:24:45 PDT
Created attachment 90837 [details] Patch
Eric Seidel (no email)
Comment 3 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". :(
Eric Seidel (no email)
Comment 4 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?
Ojan Vafai
Comment 5 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.
Eric Seidel (no email)
Comment 6 2011-04-22 20:46:43 PDT
Adam must be seeing these results or he wouldn't have added the support, or?
Eric Seidel (no email)
Comment 7 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.
Adam Barth
Comment 8 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.
Ojan Vafai
Comment 9 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!
Dirk Pranke
Comment 10 2011-04-26 15:49:35 PDT
patch LGTM.
Eric Seidel (no email)
Comment 11 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.
Adam Barth
Comment 12 2011-05-01 22:42:04 PDT
Created attachment 91884 [details] Patch
Eric Seidel (no email)
Comment 13 2011-05-01 22:52:43 PDT
Comment on attachment 91884 [details] Patch OK.
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2011-05-01 23:15:13 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.