Bug 88709 – nrwt outputs empty files for wdiff output if wdiff is not installed

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Balazs Kelemen]

It would be nice to avoid generating these useless files. Furthermore, I think we should print a warning message if wdiff is not installed, otherwise folks will likely not notice this useful feature at all (on Linux it's usually not installed by default).

[Dirk Pranke]

It's not supposed to generate the files if wdiff isn't installed, so you've found a bug :).

[Dirk Pranke]

Created attachment 148500 [details]
Patch

[Tony Chang]

Comment on attachment 148500 [details]
Patch

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

Is there a test for this?

Do we need to update the code that generates results.html to make sure it doesn't include links to the non-existent diffs? We should have a test for that.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:239
> +    def _log_wdiff_missing(self):
> +        _log.warning("wdiff is not installed; please install it to generate word-by-word diffs.")
> +        _log.warning('')

I would change this method to just return the string to output.  Then you don't have to duplicate the calls to _log.warning and the calls to _log.warning are next to the 'if logging' check.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 148500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148500&action=review
> 
> Is there a test for this?
> 

No ... I realized after I uploaded this last night that I needed to add some tests. Will do so.

> Do we need to update the code that generates results.html to make sure it doesn't include links to the non-existent diffs? We should have a test for that.
>

No, the code already handles that, but I'll add tests.
 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:239
> > +    def _log_wdiff_missing(self):
> > +        _log.warning("wdiff is not installed; please install it to generate word-by-word diffs.")
> > +        _log.warning('')
> 
> I would change this method to just return the string to output.  Then you don't have to duplicate the calls to _log.warning and the calls to _log.warning are next to the 'if logging' check.

Ok.

[Dirk Pranke]

Created attachment 148652 [details]
update w/ review feedback

[Dirk Pranke]

Committed r120865: <http://trac.webkit.org/changeset/120865>