Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only set $SAFE for MRI in spec helper #2684

Merged
merged 1 commit into from
Jan 9, 2020
Merged

Only set $SAFE for MRI in spec helper #2684

merged 1 commit into from
Jan 9, 2020

Conversation

bjfish
Copy link
Contributor

@bjfish bjfish commented Jan 7, 2020 •
edited

This PR is to set$SAFE only for MRI as I think other engines do not support $SAFE.

@bjfish
Copy link
Contributor Author

bjfish commented Jan 7, 2020

Should this be updated as well? and possibly used here?
https://github.com/rspec/rspec-support/blob/9d0482b501b1dfb197bd57a809ab2e921fde11be/lib/rspec/support/ruby_features.rb#L93-L101

pirj
pirj approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For CI failure: !defined?(RUBY_ENGINE) || RUBY_ENGINE == 'ruby' as per rspec-support's implementation should work.

Regarding rspec-support - I could only find supports_taint? to be used once across RSpec projects, in rspec-core. I would expect that example to fail on JRuby, but it doesn't for some reason.

@bjfish
Copy link
Contributor Author

bjfish commented Jan 7, 2020

Thanks @pirj , I updated to (defined?(RUBY_ENGINE) && RUBY_ENGINE != "ruby"). There is now another failure which I don't quite understand yet which may be unrelated.

@pirj
Copy link
Member

pirj commented Jan 8, 2020

Thanks! I'll take another look later today.

@pirj
Copy link
Member

pirj commented Jan 8, 2020

Actually, I've re-started that failed job and it's all green now.

@pirj
Copy link
Member

pirj commented Jan 9, 2020

@JonRowe Please take a look.

@JonRowe
Copy link
Member

JonRowe commented Jan 9, 2020

LGTM I trust you two on this

@pirj pirj merged commit 1174cc5 into rspec:master Jan 9, 2020
@bjfish bjfish deleted the ignore-safe-when-not-mri branch January 9, 2020 22:17
MatheusRich pushed a commit to MatheusRich/rspec-core that referenced this pull request Oct 30, 2020
@pirj
Merge pull request rspec#2684 from bjfish/ignore-safe-when-not-mri
4c6cb8c 
Only set $SAFE for MRI in spec helper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirj pirj pirj approved these changes

@benoittgt benoittgt benoittgt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bjfish @pirj @JonRowe @benoittgt