-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
added generator spec generator and spec tests #2085
Conversation
attempts to resolve #2084 |
Hello, I'm not entirely sure why it failed in Ruby 1.9.2 after looking at the job logs. I'd love to fix this if you have any thoughts on what could have caused it! |
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.
Great start! Just a few questions / suggestions :)
@@ -0,0 +1,26 @@ | |||
require 'generators/rspec' | |||
|
|||
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.
Why only Rails 5.1 and above? All Rails versions have generators?
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.
Thanks for pointing this out! I was looking at the system generator to help with creating this generator and forgot to take that out. But it is now removed in the new commit.
RSpec.describe "<%= class_name.pluralize %>", <%= type_metatag(:generator) %> do | ||
before do | ||
driven_by(:rack_test) | ||
end |
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.
Generators don't have system test functionality, this should be able to go :)
@@ -0,0 +1,40 @@ | |||
# Generators are not automatically loaded by rails | |||
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.
If the other rails version if is removed so should this.
I reran the failing job for 1.9.2 |
Hello, @JonRowe I appreciate the review/feedback! I have made the changes in the new commit. Let me know if there is anything else that needs to be adjusted or changed! :) @benoittgt thank you for re-running the failing job, seems like that resolved it! |
It seems you have indentation issue https://travis-ci.org/rspec/rspec-rails/jobs/499390139 |
My mistake, I fixed indention in most recent commit! |
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.
Great! Will merge on green
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.
Actually sorry my bad, I've just realised the file is named wrong.
It needs to be:
lib/generators/rspec/generators/generator_generator.rb
and the template needs to be in:
lib/generators/rspec/generators/templates
No worries! I've got that changed now, before I commit do we want the |
Yes it needs to be with its cohorts |
I made those adjustments to the names, let me know if there is anything else! |
I'm seeing a bunch of errors after the most recent commit. I will work on resolving them |
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.
Almost there
require 'generators/rspec/generators/generator_generator' | ||
require 'support/generators' | ||
|
||
RSpec.describe Rspec::Generators::GeneratorsGenerator, :type => :generator do |
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.
Can you fix the indentation here? Its not quite right (all of this file needs 2 spaces removing)
module Generators | ||
# @private | ||
class GeneratorsGenerator < Base | ||
class_option :generator_specs, :type => :boolean, :default => true, :desc => "Generate generator specs" |
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 think :default
should be false
here
|
||
describe "generator specs" do | ||
subject(:generator_spec) { file("spec/generator/posts_generator_spec.rb") } | ||
describe "are generated independently from the command line" do |
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.
s/describe/context
and s/are generated independently/can be generated
before do | ||
run_generator %w(posts) | ||
end | ||
describe "the spec" do |
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.
You can collapse this into the docs strings below:
it "creates the spec file"
it "includes the rails helper in the spec file"
it "includes the generator type in the metadata"
end | ||
end | ||
|
||
describe "are not generated" do |
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.
s/describe/context
s/are not generated/are skipped by default
subject(:generator_spec) { file("spec/generator/posts_generator_spec.rb") } | ||
describe "are generated independently from the command line" do | ||
before do | ||
run_generator %w(posts) |
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.
Add --generator-specs
|
||
describe "are not generated" do | ||
before do | ||
run_generator %w(posts --no-generator-specs) |
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.
Can remove the --no-generator-specs
Hey @JonRowe, I made those changes. Let me know how it looks! |
Added generator spec generator
No description provided.