Bug 54699 – we should skip webkitpy.common.prettypatch_unittest if ruby isn't installed

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dirk Pranke]

since there's no point in running them.

[Dirk Pranke]

Created attachment 82901 [details]
Patch

[WebKit Review Bot]

Attachment 82901 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/Scripts/webkitpy/common/prettypatch_unittest.py:44:  trailing whitespace  [pep8/W291] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 82901 [details]
Patch

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

> Tools/Scripts/webkitpy/common/prettypatch_unittest.py:40
> +            result = executive.run_command(['ruby', '--version'])

It would be nice if we didn't have to actually try to execute ruby. But it's probably the most foolproof check.

> Tools/Scripts/webkitpy/common/prettypatch_unittest.py:70
>      def test_pretty_diff_encodings(self):
> +        if not self.check_ruby():
> +            return

Should we print a warning?

> Tools/Scripts/webkitpy/common/prettypatch_unittest.py:72
>          pretty_patch = PrettyPatch(Executive(), self._webkit_root())

Should the prettypatch module do something sensible when ruby isn't installed, too?

> Tools/Scripts/webkitpy/common/prettypatch_unittest.py:79
>      def test_pretty_print_empty_string(self):
> +        if not self.check_ruby():
> +            return

Same question.

[Dirk Pranke]

(In reply to comment #3)
> (From update of attachment 82901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82901&action=review
> 
> > Tools/Scripts/webkitpy/common/prettypatch_unittest.py:40
> > +            result = executive.run_command(['ruby', '--version'])
> 
> It would be nice if we didn't have to actually try to execute ruby. But it's probably the most foolproof check.
> 

Yeah, I don't know of a better way to do this.

> > Tools/Scripts/webkitpy/common/prettypatch_unittest.py:70
> >      def test_pretty_diff_encodings(self):
> > +        if not self.check_ruby():
> > +            return
> 
> Should we print a warning?
>

I don't think we generally want to print warnings when we skip over tests that we know we can't execute, just like we don't in run-webkit-tests. 

At some point when I can revamp test-webkitpy it'll have more useful logging for this sort of thing (bug 49299)
 
> > Tools/Scripts/webkitpy/common/prettypatch_unittest.py:72
> >          pretty_patch = PrettyPatch(Executive(), self._webkit_root())
> 
> Should the prettypatch module do something sensible when ruby isn't installed, too?
>

Maybe. I'll file a separate bug to look into that. Right now this is trapped in new-run-webkit-tests, but I think webkitpy.tool will probably get upset.
 
> > Tools/Scripts/webkitpy/common/prettypatch_unittest.py:79
> >      def test_pretty_print_empty_string(self):
> > +        if not self.check_ruby():
> > +            return
> 
> Same question.

[Dirk Pranke]

Committed r79046: <http://trac.webkit.org/changeset/79046>

[Dirk Pranke]

bug 54799 filed to consider making the prettypatch module the place to sensibly handle ruby not being installed.