Bug 59256 - only include failures in full_results.json
Summary: only include failures in full_results.json
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: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-22 16:38 PDT by Ojan Vafai
Modified: 2011-04-27 15:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (3.92 KB, patch)
2011-04-22 16:39 PDT, Ojan Vafai
tony: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-04-22 16:38:16 PDT
only include failures in full_results.json
Comment 1 Ojan Vafai 2011-04-22 16:39:05 PDT
Created attachment 90801 [details]
Patch
Comment 2 Ojan Vafai 2011-04-22 16:39:35 PDT
This patch depends on bug 59239.
Comment 3 Tony Chang 2011-04-22 19:17:24 PDT
Comment on attachment 90801 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:128
> +        test_dict = {}

It would be nice to just make these structs at some point.

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:130
> +        if False:
> +            test_dict['has_stderr'] = True

Debugging code?
Comment 4 Dirk Pranke 2011-04-26 16:11:05 PDT
Comment on attachment 90801 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:74
> +    """Summarize failing results as a dict.

Unless I'm misunderstanding this, it looks like this change removes unexpected passes. That's bad - it'll break the bot output reporting unexpected passes and the log output for the same. It seems to me that if you want to exclude the results from the JSON file you should do it where you build the JSON file, not by changing this function.
Comment 5 Ojan Vafai 2011-04-27 11:20:56 PDT
(In reply to comment #4)
> (From update of attachment 90801 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90801&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:74
> > +    """Summarize failing results as a dict.
> 
> Unless I'm misunderstanding this, it looks like this change removes unexpected passes. That's bad - it'll break the bot output reporting unexpected passes and the log output for the same. 

Good catch. Fixed.

> It seems to me that if you want to exclude the results from the JSON file you should do it where you build the JSON file, not by changing this function.

I don't see why. The only places the summarized results are used is to write out the json and print out unexpected failures. I don't really see that changing in the future, do you?
Comment 6 Ojan Vafai 2011-04-27 14:27:23 PDT
Committed r85095: <http://trac.webkit.org/changeset/85095>
Comment 7 Dirk Pranke 2011-04-27 14:57:19 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 90801 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=90801&action=review
> > 
> > > Tools/Scripts/webkitpy/layout_tests/layout_package/manager.py:74
> > > +    """Summarize failing results as a dict.
> > 
> > Unless I'm misunderstanding this, it looks like this change removes unexpected passes. That's bad - it'll break the bot output reporting unexpected passes and the log output for the same. 
> 
> Good catch. Fixed.
> 
> > It seems to me that if you want to exclude the results from the JSON file you should do it where you build the JSON file, not by changing this function.
> 
> I don't see why. The only places the summarized results are used is to write out the json and print out unexpected failures. I don't really see that changing in the future, do you?

At some point I'd like to change the Manager.run() routine to return something like the summarized results so that we can write better integration tests, but it's a pretty low-priority thing.
Comment 8 Dirk Pranke 2011-04-27 14:58:35 PDT
Nit, since the summarized results kept the unexpected passes, the original docstring was more accurate. I think you forgot to change it back.
Comment 9 WebKit Review Bot 2011-04-27 15:39:04 PDT
http://trac.webkit.org/changeset/85095 might have broken Windows 7 Release (Tests)