-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fail the build if a specified role is missing specs #40
Conversation
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.
That makes sense to me
lib/ami_spec/server_spec.rb
Outdated
@@ -40,6 +47,7 @@ def run | |||
|
|||
RSpec.configuration.fail_fast = true if @debug | |||
|
|||
|
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.
whitespace?
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.
ooops. I've spent too long in javascript land, where prettier cleans up my mess for me ;)
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.
This is before RSpec gets loaded anyway, so 👍 to printing an error and returning
lib/ami_spec/server_spec.rb
Outdated
puts "skip the role '#{@role}', create the directory but leave it empty" | ||
puts "(except for a .gitkeep file)." | ||
return false | ||
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.
Should this be done during initialization? Doing it during a run
will spin up other AMIs to test still, despite an imminent failure. Otherwise a verify
method might be useful?
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.
Yeah, I've just moved that in the latest commit.
6b3a883
to
7daf1ed
Compare
unless Dir.exist?("#{options[:specs]}/#{role}") | ||
fail "Role directory #{options[:specs]}/#{role} does not exist. If you'd like to skip the role '#{role}', create the directory but leave it empty (except for a .gitignore file)." | ||
end | ||
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.
This only tests if we are using the file. We should also test when options[:ami]
and options[:role]
are supplied
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.
Further up in the file options[:role] && options[:ami]
are co-erced into options[:amis]
Not sure on implementation here - I feel like there should be a more rspeccy way of doing this, preferably with a nice red rspec failure.
Anyway, this not failing fast caused lots of confusion as our passing build was not actually running tests against some of the roles it had been told to.