Add RubyFeatures.ripper_supported? #245

yujinakayama · 2015-10-10T00:25:44Z

For rspec/rspec-core#2083

myronmarston · 2015-10-10T15:31:49Z

lib/rspec/support/ruby_features.rb

+      def ripper_supported?
+        # JRuby supports Ripper but it seems to be unstable
+        # and also reports wrong line number on JRuby 9.0.0.0.
+        defined?(RUBY_ENGINE) && RUBY_ENGINE == 'ruby' && RUBY_VERSION >= '1.9.2'


This is too bad about JRuby's ripper support not being good enough for us to use. Has the issue been reported upstream? I imagine @headius and co would want to know and do something about this. Unfortunately, as this is currently written it'll lock JRuby users out from enjoying the benefits of the feature if/when JRuby's ripper is fixed.

Would it be possible to write some logic that detects if ripper is working on JRuby so that we allow it to be used if a user is running a future version of JRuby where the ripper issues have been fixed? Although, the obvious ways to do that would involve loading ripper and testing it out, which I don't think we want to do...

Just reported the wrong line number issue jruby/jruby#3386.

Would it be possible to write some logic that detects if ripper is working on JRuby so that we allow it to be used if a user is running a future version of JRuby where the ripper issues have been fixed? Although, the obvious ways to do that would involve loading ripper and testing it out, which I don't think we want to do...

I added examples to check the Ripper behavior in rspec-support's spec rather than user's environement so that we can immediately notice the change on Travis CI in the future. By the way I noticed that we do not have CI for JRuby 9.0.0.0.

Also, I confirmed specs in rspec/rspec-core#2083 properly passed on JRuby 1.7.14 - 1.7.22.

myronmarston · 2015-10-11T20:42:28Z

lib/rspec/support/ruby_features.rb

+      elsif Ruby.jruby? &&
+            defined?(Gem::Version) &&
+            Gem::Version.new(JRUBY_VERSION) >= Gem::Version.new('1.7.14') &&
+            !JRUBY_VERSION.start_with?('9.')


This conditional will return false if rubygems is not loaded. It should not be a requirement that rubygems is loaded to use this feature. Users may want to run rspec w/ the --disable=gems flag and that should work, IMO. I think you can use our version checker instead:

https://github.com/rspec/rspec-support/blob/master/lib/rspec/support/version_checker.rb

yujinakayama · 2015-10-12T02:13:30Z

Not sure why but the CI is failing now, even on master branch...

Running specs for rspec-core
~/build/rspec/rspec-core ~/build/rspec/rspec-support
`ruby_22` is not a valid platform. The available options are: [:ruby, :ruby_18,
:ruby_19, :ruby_20, :ruby_21, :mri, :mri_18, :mri_19, :mri_20, :mri_21, :rbx,
:jruby, :jruby_18, :jruby_19, :mswin, :mingw, :mingw_18, :mingw_19, :mingw_20,
:mingw_21, :x64_mingw, :x64_mingw_20, :x64_mingw_21]
The command "script/run_build" exited with 4.

myronmarston · 2015-10-12T02:15:33Z

Those are bundler platform values. Maybe a bundler regression? Did the bundler version change?

yujinakayama · 2015-10-12T02:22:47Z

Did the bundler version change?

No.

Passed job: https://travis-ci.org/rspec/rspec-support/jobs/83149928

$ ruby --version
ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-linux]
$ rvm --version
rvm 1.26.10 (latest-minor) by Wayne E. Seguin <[email protected]>, Michal Papis <[email protected]> [https://rvm.io/]
$ bundle --version
Bundler version 1.7.9
$ gem --version
2.4.5

Failed job: https://travis-ci.org/rspec/rspec-support/jobs/83956734

$ ruby --version
ruby 2.2.0p0 (2014-12-25 revision 49005) [x86_64-linux]
$ rvm --version
rvm 1.26.10 (latest-minor) by Wayne E. Seguin <[email protected]>, Michal Papis <[email protected]> [https://rvm.io/]
$ bundle --version
Bundler version 1.7.9
$ gem --version
2.4.5

myronmarston · 2015-10-12T03:21:59Z

Looking at the benchmark, I can see that ripper is certainly much slower on those JRuby 1.7.x releases but I'm not sure that it's so slow that it makes sense to disable the feature entirely. Consider that the benchmark shows 240-250 ripper parsing operations per second on those versions of JRuby. That's only 4 ms or so. I think the productivity boost from having the full code snippet is going to save the user much more than 4 ms of time. Besides, in the rspec-core PR you've implemented a source cache so the slow parsing cost will only be paid once per distinct file that has a failure. In my experience, it's a lot more common to have a few files, each with many failures, than to have many files, each with few failures. This slowness is also not a general slowdown we're adding for all rspec runs -- it just comes in to play when there's a failure and we're trying to provide maximally useful failure output.

myronmarston · 2015-10-12T03:25:38Z

lib/rspec/support/ruby_features.rb

+        def ripper_supported?
+          true
+        end
+      rescue LibraryVersionTooLowError, UnsupportedVersionError


Exceptions for control flow is always one of those things that feels smelly. Instead, WDYT of making VersionChecker#compare_version public and using that to find out how the version compares w/o needing to use exceptions?

Actually, it looks like nothing is using the version checker so you should feel free to change it's API to whatever you need :).

yujinakayama · 2015-10-12T12:48:40Z

Replaced VersionChecker with the new ComparableVersion.
Changed the Ripper support on JRuby to >= 1.7.5 && < 9.0.0.0.

myronmarston · 2015-10-12T15:02:38Z

lib/rspec/support/comparable_version.rb

+      end
+    end
+  end
+end


This class looks nice and simple, but will it handle version numbers with .pre, .rc1, .beta, etc in the name? The segments method uses to_i which will silently convert those to 0. Looking at the JRuby downloads page, I see version numbers like 9.0.0.0.pre1 and 9.0.0.0.rc1. Since we pass the JRUBY_VERSION to this I think we need to handle those versions properly.

myronmarston · 2015-10-12T15:09:34Z

This LGTM besides the concern about pre-release words in version numbers.

JonRowe · 2015-10-12T23:37:47Z

lib/rspec/support/version_checker.rb

-    LibraryVersionTooLowError = Class.new(StandardError)
-
-    # @private
-    class VersionChecker


I think this is used outside of rspec-support (I'm thinking rspec-rails at least) so we might need to leave this in until we've switched those instances over.

The current rspec-rails is not using VersionChecker and the rspec-support dependency is pinned to major and minor versions, so I think removing VersionChecker does not break anything.

I was wrong. It's still used here. The vendor directory was ignored by my ag...

I thought it was used somewhere! It was created for rspec-rails originally :P

myronmarston · 2015-10-13T06:18:01Z

This LGTM apart from the build failure.

JonRowe · 2015-10-14T05:22:39Z

Want to rebase this off master and see if it works now?

yujinakayama · 2015-10-14T05:23:51Z

Done

JonRowe · 2015-10-14T05:38:53Z

Sweet as, 🍏

Add RubyFeatures.ripper_supported?

As of rspec/rspec-support#245, `rspec/support/ruby_features` relies upon `rspec/support` being loaded first.

yujinakayama force-pushed the ripper-support-detection branch from 2ac9a47 to 9f28c03 Compare October 10, 2015 13:36

myronmarston reviewed Oct 10, 2015
View reviewed changes

yujinakayama force-pushed the ripper-support-detection branch 4 times, most recently from ee98c6d to 4f5d608 Compare October 11, 2015 14:10

myronmarston reviewed Oct 11, 2015
View reviewed changes

yujinakayama force-pushed the ripper-support-detection branch 4 times, most recently from efff0a3 to 992711d Compare October 12, 2015 02:04

yujinakayama force-pushed the ripper-support-detection branch 3 times, most recently from 345233f to 9ba6ac5 Compare October 12, 2015 02:45

myronmarston reviewed Oct 12, 2015
View reviewed changes

yujinakayama force-pushed the ripper-support-detection branch 2 times, most recently from 4dcc8dc to 5743811 Compare October 12, 2015 12:45

myronmarston reviewed Oct 12, 2015
View reviewed changes

JonRowe reviewed Oct 12, 2015
View reviewed changes

yujinakayama force-pushed the ripper-support-detection branch 2 times, most recently from c6cbd07 to d315fa1 Compare October 13, 2015 02:55

yujinakayama force-pushed the ripper-support-detection branch from d315fa1 to 61c960e Compare October 13, 2015 02:56

yujinakayama force-pushed the ripper-support-detection branch 4 times, most recently from 54e84fd to e6fc120 Compare October 14, 2015 01:40

yujinakayama added 3 commits October 14, 2015 14:23

Add ComparableVersion

01a65c2

Add RubyFeatures.ripper_supported?

6042ee1

Add spec to track Ripper support for each Ruby implementation

ea46730

yujinakayama force-pushed the ripper-support-detection branch from e6fc120 to ea46730 Compare October 14, 2015 05:23

JonRowe added a commit that referenced this pull request Oct 14, 2015

Merge pull request #245 from rspec/ripper-support-detection

81e33d4

Add RubyFeatures.ripper_supported?

JonRowe merged commit 81e33d4 into master Oct 14, 2015

JonRowe deleted the ripper-support-detection branch October 14, 2015 05:38

myronmarston added a commit to rspec/rspec-core that referenced this pull request Oct 14, 2015

Add missing require.

801a48c

As of rspec/rspec-support#245, `rspec/support/ruby_features` relies upon `rspec/support` being loaded first.

MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020

Add missing require.

20fd1e5

As of rspec/rspec-support#245, `rspec/support/ruby_features` relies upon `rspec/support` being loaded first.

Uh oh!

Add RubyFeatures.ripper_supported? #245

Add RubyFeatures.ripper_supported? #245

Uh oh!

Conversation

yujinakayama commented Oct 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yujinakayama commented Oct 12, 2015

Uh oh!

myronmarston commented Oct 12, 2015

Uh oh!

yujinakayama commented Oct 12, 2015

Uh oh!

myronmarston commented Oct 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yujinakayama commented Oct 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Oct 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Oct 13, 2015

Uh oh!

JonRowe commented Oct 14, 2015

Uh oh!

yujinakayama commented Oct 14, 2015

Uh oh!

JonRowe commented Oct 14, 2015

Uh oh!

Uh oh!