Update ripper_supported? for truffleruby and ripper specs #395

bjfish · 2020-01-10T16:50:22Z

and allow ripper specs to produce exceptions.

eregon · 2020-01-10T17:56:48Z

lib/rspec/support/ruby_features.rb

@@ -101,7 +105,7 @@ def supports_taint?
      end
      ripper_requirements = [ComparableVersion.new(RUBY_VERSION) >= '1.9.2']

-      ripper_requirements.push(false) if Ruby.rbx?
+      ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?


This is not ideal, because TruffleRuby might support Ripper at some point.
How about checking Ripper.respond_to?(:lex) which is what https://github.com/k0kubun/hamlit does for instance?

You could push that onto the stack for if run truffle ruby?

@eregon I think there would be an issue with this because part of the spec asserts Ripper would not be loaded by feature detection:

rspec-support/spec/rspec/support/ruby_features_spec.rb

Lines 151 to 153 in b83b4ce

it 'does not load Ripper' do

expect { RubyFeatures.ripper_supported? }.not_to change { defined?(::Ripper) }

end

I see, that is unfortunate. Probably fine to keep as-is then and change it again (e.g., to a version check) once TruffleRuby supports Ripper.

Suggested change

ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?

ripper_requirements.push(defined?(::Ripper) && Ripper.respond_to?(:lex)) if Ruby.truffleruby?

We require it if ripper is supported,

Right, so the whole point of this logic is then avoiding require 'ripper' if Ripper is unsupported.
Is that to avoid changing $LOADED_FEATURES or making sure code which uses Ripper still require 'ripper', but only for Rubies with unsupported Ripper?

I would suggest something a lot more straightforward by always requiring Ripper, then we wouldn't need to have so much Ruby-implementation-specific code here.

on recent MRI it's already loaded

I don't see that:

$ ruby -ve 'puts $".grep(/ripper/i)' ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-linux]

In practise it would mean that you'd have to have require it on truffle ruby if you wanted to use it and it would work.

I wouldn't want that, I'd want the same behavior as MRI.
Since anyway this does not seem a critical feature of RSpec, I propose to adapt the code here once TruffleRuby supports Ripper and merge the PR as it is.

Right, so the whole point of this logic is then avoiding require 'ripper' if Ripper is unsupported. Is that to avoid changing $LOADED_FEATURES or making sure code which uses Ripper still require 'ripper', but only for Rubies with unsupported Ripper?

It's to avoid requiring ripper if possible.

I would suggest something a lot more straightforward by always requiring Ripper, then we wouldn't need to have so much Ruby-implementation-specific code here.

We do the least requiring we can to avoid false positives due to poisoned environments.

I don't see that:

I stand corrected, irb requires it, ruby does not.

I wouldn't want that, I'd want the same behavior as MRI.

In my experience only MRI behaves like MRI, other Ruby engines always have quirks. 😢

In my experience only MRI behaves like MRI, other Ruby engines always have quirks.

FWIW, TruffleRuby tries hard to behave exactly like MRI. But implementing Ripper is a significant effort, so that's not done yet.

To be fair I should probably say that every engine has quirks, it's just the MRI ones are the reference quirks? 🤷‍♂

I just wanted to end on a positive note :)
This PR is not the most beautiful code but TruffleRuby otherwise runs RSpec (and Rails nowadays) unmodified with no TruffleRuby-specific code since many years now.
We try hard to avoid any TruffleRuby-specific code in gems.

spec/rspec/support/ruby_features_spec.rb

JonRowe · 2020-01-10T18:07:48Z

spec/rspec/support/ruby_features_spec.rb

+              !sexp.nil?
+            rescue
+              false
+            end


I'd rather we didn't attempt to run this if ripper isn't available.

See #395 (comment)

JonRowe · 2020-01-10T22:26:40Z

lib/rspec/support/ruby_features.rb

@@ -101,7 +105,7 @@ def supports_taint?
      end
      ripper_requirements = [ComparableVersion.new(RUBY_VERSION) >= '1.9.2']

-      ripper_requirements.push(false) if Ruby.rbx?
+      ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?


You could push that onto the stack for if run truffle ruby?

pirj · 2020-01-11T10:09:04Z

spec/rspec/support/ruby_features_spec.rb

@@ -145,7 +145,7 @@ def ripper_can_parse_source_including_keywordish_symbol?
        end

        it 'returns whether Ripper is correctly implemented in the current environment' do
-          expect(RubyFeatures.ripper_supported?).to eq(ripper_is_implemented? && ripper_works_correctly?)
+          expect(RubyFeatures.ripper_supported?).to eq(RubyFeatures.ripper_supported? && ripper_is_implemented? && ripper_works_correctly?)


expect(RubyFeatures.ripper_supported?).to eq(RubyFeatures.ripper_supported?

That looks odd quite frankly :D

This should be reverted, the check here is that ripper_supported? matches the two test definitions, are you saying the test definitions return true here? As that should be impossible if ripper is broken on truffleruby. Whats actually happening?

@JonRowe On truffleruby, a runtime error occurs when running ripper_is_implemented? && ripper_works_correctly? which is why I suggested the begin/rescue change before to assert that running these results in error when RubyFeatures.ripper_supported? is false.

Per your comment: #395 (comment). I updated to not run this spec if RubyFeatures.ripper_supported? is false.

@pirj I agree this looked odd. I've pushed a new version to be more explicit in excluding this spec from running using: , :if => ripper_supported?. Please review.

JonRowe · 2020-01-12T08:48:52Z

lib/rspec/support/ruby_features.rb

@@ -101,7 +105,7 @@ def supports_taint?
      end
      ripper_requirements = [ComparableVersion.new(RUBY_VERSION) >= '1.9.2']

-      ripper_requirements.push(false) if Ruby.rbx?
+      ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?


Suggested change

ripper_requirements.push(false) if Ruby.rbx? || Ruby.truffleruby?

ripper_requirements.push(defined?(::Ripper) && Ripper.respond_to?(:lex)) if Ruby.truffleruby?

JonRowe · 2020-01-12T08:49:14Z

spec/rspec/support/ruby_features_spec.rb

@@ -145,7 +145,7 @@ def ripper_can_parse_source_including_keywordish_symbol?
        end

        it 'returns whether Ripper is correctly implemented in the current environment' do
-          expect(RubyFeatures.ripper_supported?).to eq(ripper_is_implemented? && ripper_works_correctly?)
+          expect(RubyFeatures.ripper_supported?).to eq(RubyFeatures.ripper_supported? && ripper_is_implemented? && ripper_works_correctly?)


This should be reverted, the check here is that ripper_supported? matches the two test definitions, are you saying the test definitions return true here? As that should be impossible if ripper is broken on truffleruby. Whats actually happening?

JonRowe · 2020-01-13T16:01:16Z

spec/rspec/support/ruby_features_spec.rb

@@ -123,6 +123,10 @@ def ripper_works_correctly?
            ripper_can_parse_source_including_keywordish_symbol?
        end

+        def self.ripper_supported?
+          RubyFeatures.ripper_supported?


This is the subject under test, its not a valid way to see if ripper is supported..

JonRowe · 2020-01-13T16:03:44Z

This should be reverted, the check here is that ripper_supported? matches the two test definitions, are you saying the test definitions return true here? As that should be impossible if ripper is broken on truffleruby. Whats actually happening?

Github has lost the reply to this somewhere, whats the runtime error from those two detection methods? Is that what the rescue false was for originally? Out of those two solutions I'd prefer you rescue the specific exception with a note about truffle-ruby.

The main thing I wanted you to change was to stop the reliance on RubyFeatures.ripper_supported? to run the test for RubyFeatures.ripper_supported?. Its basically preventing the test from checking for false positives.

bjfish · 2020-01-13T16:48:30Z

@JonRowe I've updated to add the begin;rescue with a note for truffleruby and removed the dependency on RubyFeatures.ripper_supported? to run the test. Thanks.

JonRowe

I'm sorry for the constant back and forth, I think I've misunderstood your problem a couple of times...

spec/rspec/support/ruby_features_spec.rb

…ted?

bjfish · 2020-01-13T20:05:34Z

@JonRowe I've updated ripper_is_implemented? with your suggestion, thanks.

JonRowe

Thanks! Apologies again for the lack of clear direction!

eregon

Looks good to me too.

JonRowe

Whilst we don't officially support Truffle Ruby, if this makes it possible to run our specs with it I'm ok with it.

Update ripper_supported? for truffleruby and ripper specs

…te-ripper-helper-and-spec Update ripper_supported? for truffleruby and ripper specs --- This commit was imported from rspec/rspec-support@6c5d192.

This commit was imported from rspec/rspec-support@2c142a8.

eregon reviewed Jan 10, 2020

View reviewed changes

JonRowe requested changes Jan 10, 2020

View reviewed changes

bjfish force-pushed the update-ripper-helper-and-spec branch from 88aee8b to e15e4a8 Compare January 10, 2020 23:02

pirj reviewed Jan 11, 2020

View reviewed changes

JonRowe requested changes Jan 12, 2020

View reviewed changes

bjfish force-pushed the update-ripper-helper-and-spec branch from e15e4a8 to e183117 Compare January 13, 2020 15:17

JonRowe requested changes Jan 13, 2020

View reviewed changes

bjfish force-pushed the update-ripper-helper-and-spec branch 2 times, most recently from 41872c7 to efdce64 Compare January 13, 2020 16:42

JonRowe requested changes Jan 13, 2020

View reviewed changes

spec/rspec/support/ruby_features_spec.rb Outdated Show resolved Hide resolved

Update ripper_supported? for truffleruby and specs ripper_is_implemen…

6ae5540

…ted?

bjfish force-pushed the update-ripper-helper-and-spec branch from efdce64 to 6ae5540 Compare January 13, 2020 20:03

JonRowe approved these changes Jan 13, 2020

View reviewed changes

eregon approved these changes Jan 13, 2020

View reviewed changes

JonRowe approved these changes Jan 14, 2020

View reviewed changes

JonRowe merged commit 54f601f into rspec:master Jan 14, 2020

JonRowe added a commit that referenced this pull request Jan 14, 2020

Change log for #395

8c2ca4a

bjfish deleted the update-ripper-helper-and-spec branch January 14, 2020 15:26

JonRowe added a commit that referenced this pull request May 2, 2020

Merge pull request #395 from bjfish/update-ripper-helper-and-spec

6c5d192

Update ripper_supported? for truffleruby and ripper specs

JonRowe added a commit that referenced this pull request May 2, 2020

Change log for #395

2c142a8

yujinakayama pushed a commit to yujinakayama/rspec-monorepo that referenced this pull request Oct 6, 2021

[support] Change log for rspec/rspec-support#395

97abfb1

This commit was imported from rspec/rspec-support@2c142a8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update ripper_supported? for truffleruby and ripper specs #395

Update ripper_supported? for truffleruby and ripper specs #395

bjfish commented Jan 10, 2020

eregon Jan 10, 2020

JonRowe Jan 10, 2020

bjfish Jan 10, 2020

eregon Jan 10, 2020

JonRowe Jan 12, 2020

eregon Jan 14, 2020

JonRowe Jan 14, 2020

eregon Jan 14, 2020

JonRowe Jan 14, 2020

eregon Jan 14, 2020

JonRowe Jan 10, 2020

bjfish Jan 10, 2020

JonRowe Jan 10, 2020

pirj Jan 11, 2020

JonRowe Jan 12, 2020

bjfish Jan 13, 2020

JonRowe Jan 12, 2020

JonRowe Jan 12, 2020

JonRowe Jan 13, 2020 •

edited

JonRowe commented Jan 13, 2020 •

edited

bjfish commented Jan 13, 2020

JonRowe left a comment

bjfish commented Jan 13, 2020

JonRowe left a comment

eregon left a comment

JonRowe left a comment

	it 'does not load Ripper' do
	expect { RubyFeatures.ripper_supported? }.not_to change { defined?(::Ripper) }
	end

	ripper_requirements.push(false) if Ruby.rbx? \|\| Ruby.truffleruby?
	ripper_requirements.push(defined?(::Ripper) && Ripper.respond_to?(:lex)) if Ruby.truffleruby?

Update ripper_supported? for truffleruby and ripper specs #395

Update ripper_supported? for truffleruby and ripper specs #395

Conversation

bjfish commented Jan 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonRowe Jan 13, 2020 • edited

Choose a reason for hiding this comment

JonRowe commented Jan 13, 2020 • edited

bjfish commented Jan 13, 2020

JonRowe left a comment

Choose a reason for hiding this comment

bjfish commented Jan 13, 2020

JonRowe left a comment

Choose a reason for hiding this comment

eregon left a comment

Choose a reason for hiding this comment

JonRowe left a comment

Choose a reason for hiding this comment

JonRowe Jan 13, 2020 •

edited

JonRowe commented Jan 13, 2020 •

edited