Bug 59256 – only include failures in full_results.json

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

only include failures in full_results.json

[Ojan Vafai]

Created attachment 90801 [details]
Patch

[Ojan Vafai]

This patch depends on bug 59239.

[Tony Chang]

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?

[Dirk Pranke]

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.

[Ojan Vafai]

(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?

[Ojan Vafai]

Committed r85095: <http://trac.webkit.org/changeset/85095>

[Dirk Pranke]

(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.

[Dirk Pranke]

Nit, since the summarized results kept the unexpected passes, the original docstring was more accurate. I think you forgot to change it back.

[WebKit Review Bot]

http://trac.webkit.org/changeset/85095 might have broken Windows 7 Release (Tests)