-
-
Notifications
You must be signed in to change notification settings - Fork 1k
generator system spec: closing 1933 #2075
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.
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
generator system spec: closing 1933 #2075
Conversation
|
||
def generate_system_spec | ||
return unless options[:system_specs] | ||
raise "System Tests are available for Rails >= 5.1" if ::Rails::VERSION::STRING < '5.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it what we want? 🤔
I look at other parts of RSpec code and didn't see that kind of behavior implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The generator shouldn't be available on old rails...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we should have a higher condition if not on > Rails 5.1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should even define it we can't run it.
|
||
describe "system specs" do | ||
subject(:system_spec) { file("spec/system/posts_spec.rb") } | ||
if ::Rails::VERSION::STRING < '5.1' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this as a gate or do we want to tag the example to don't be run under Rails 5.1?
d01c40f
to
5a8928d
Compare
5a8928d
to
79316d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this works for you @benoittgt then it looks good to me!
Thanks @JonRowe for the review |
Merged via rspec#2075
Merged via rspec#2075
This PR is meant to close the #1933
CI will probably fail this is a WIP PR but open for review.
For exemple why do I get:
expected RuntimeError with "System Tests are available for Rails >= 5.1", got #<RuntimeError: System Tests are available for Rails >= 5.1> with backtrace: