-
Notifications
You must be signed in to change notification settings - Fork 69
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
Add a --audit flag for finding missing/unused steps #194
Conversation
@pedantic-git, thanks for your PR! By analyzing the history of the files in this pull request, we identified @aokolish, @ywen and @josepjaume to be potential reviewers. |
@@ -141,6 +142,11 @@ def after(&block) | |||
def feature(name) | |||
@feature_name = name | |||
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.
Trailing whitespace detected.
@@ -142,6 +143,16 @@ def failure_exceptions | |||
def fail_fast | |||
@fail_fast | |||
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.
Trailing whitespace detected.
@@ -132,6 +134,11 @@ def parse_options | |||
'Terminate the suite run on the first failure') do |class_name| | |||
config[:fail_fast] = true | |||
end | |||
|
|||
opts.on('-a', '--audit', | |||
'Audit steps instead of running them, outputting missing and obsolete steps') 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.
Line is too long. [98/80]
@@ -132,6 +134,11 @@ def parse_options | |||
'Terminate the suite run on the first failure') do |class_name| | |||
config[:fail_fast] = true | |||
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.
Trailing whitespace detected.
|
||
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.
Final newline missing.
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.
Trailing whitespace detected.
puts "\n" + "Unused step: #{location} ".colorize(:yellow) + "'#{name}'".colorize(:light_yellow) | ||
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.
Trailing whitespace detected.
# Remove any unused_steps that were in common modules and used in another feature | ||
used_steps.each {|location| unused_steps.delete location} | ||
unused_steps.each do |location, name| | ||
puts "\n" + "Unused step: #{location} ".colorize(:yellow) + "'#{name}'".colorize(:light_yellow) |
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.
Line is too long. [103/80]
|
||
def report_unused_steps | ||
# Remove any unused_steps that were in common modules and used in another feature | ||
used_steps.each {|location| unused_steps.delete location} |
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.
Space between { and | missing.
Space missing inside }.
end | ||
|
||
def report_unused_steps | ||
# Remove any unused_steps that were in common modules and used in another feature |
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.
Line is too long. [87/80]
end | ||
return true | ||
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.
Trailing whitespace detected.
end | ||
return false | ||
end | ||
return true |
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.
Redundant return detected.
if missing_steps.length > 0 | ||
puts "\nMissing steps:".colorize(:light_cyan) | ||
missing_steps.each do |step| | ||
puts Generators::StepGenerator.new(step).generate.gsub(/^/, ' ').colorize(:cyan) |
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.
Line is too long. [91/80]
# And then generate a report of missing steps | ||
if missing_steps.length > 0 | ||
puts "\nMissing steps:".colorize(:light_cyan) | ||
missing_steps.each do |step| |
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.
Trailing whitespace detected.
end | ||
|
||
# And then generate a report of missing steps | ||
if missing_steps.length > 0 |
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.
Use missing_steps.length.positive? instead of missing_steps.length > 0.
Use !empty? instead of length > 0.
location = step_defs.step_location_for(name).join(':') | ||
unused_steps[location] = name | ||
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.
Trailing whitespace detected.
Looks like CI is passing but I have some Hound issues - let me take care of those. |
5 similar comments
@stdout.scan('I should still exist').count.must_equal 1 | ||
end | ||
|
||
step 'I have step files for both with common steps, but one common step is not used by either' 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.
Block has too many lines. [45/25]
Line is too long. [99/80]
@stdout.scan('I exist').count.must_equal 1 | ||
@stdout.scan('I should still exist').count.must_equal 1 | ||
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.
Trailing whitespace detected.
pending 'step not implemented' | ||
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.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
end | ||
end | ||
""") | ||
write_file('features/steps/awesome_new_feature.rb', """ |
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.
Combine "" and "\nclass Spinach::Features::AwesomeNewFeature < Spinach::FeatureSteps\n include Awesome\n step 'I am awesome' do\n pending 'step not implemented'\n end\n step 'people will cheer' do\n pending 'step not implemented';\n end\nend\n" into a single string literal, rather than using implicit string concatenation. Or, if they were intended to be separate method arguments, separate them with a comma.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
Combine "\nclass Spinach::Features::AwesomeNewFeature < Spinach::FeatureSteps\n include Awesome\n step 'I am awesome' do\n pending 'step not implemented'\n end\n step 'people will cheer' do\n pending 'step not implemented';\n end\nend\n" and "" into a single string literal, rather than using implicit string concatenation. Or, if they were intended to be separate method arguments, separate them with a comma.
pending 'step not implemented' | ||
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.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
end | ||
|
||
step 'I have created a step file without those reused steps' do | ||
write_file('features/steps/exciting_feature.rb', """ |
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.
Combine "" and "\nclass Spinach::Features::ExcitingFeature < Spinach::FeatureSteps\n step 'I do nothing' do\n pending 'step not implemented'\n end\nend\n" into a single string literal, rather than using implicit string concatenation. Or, if they were intended to be separate method arguments, separate them with a comma.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
Combine "\nclass Spinach::Features::ExcitingFeature < Spinach::FeatureSteps\n step 'I do nothing' do\n pending 'step not implemented'\n end\nend\n" and "" into a single string literal, rather than using implicit string concatenation. Or, if they were intended to be separate method arguments, separate them with a comma.
Given I exist | ||
When I jump up and down | ||
Then I should still exist | ||
""") |
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.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
I've taken care of the hound issues in most of the I've ignored the issues reported on the step files because I'm assuming you don't have the same code review standards on those? |
1 similar comment
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.
Hi, thanks so much for your Pull Request! This is a really, really nice feature and you certainly put a lot of effort on it.
I'm 👍 on this - do you think you could update the Readme.md explaining how to use the feature?
We'll release a new version after merging. Thanks ❤️
Oh, and sorry for all Hound CI's trolling :(. It looks like we mistakenly activated it for this repo and, without further configuration, it's quite annoying - we usually don't follow some of his hints. Sorry you had to deal with it, but hopefully gave some useful advice! |
Hi @josepjaume - thanks for saying so! The feature has already saved me tons of time even though I only implemented it last week so I hope it will be useful for you and other users too! I've added those instructions to the README as requested. Let me know if I can help with anything else! |
spinach --generate
is awesome if you get your feature files right first time, but I often find that over time features change and I have to add to or edit old feature files.This tool helps with that. Running
spinach --audit
will do an audit of all the feature files and their shared modules.If steps are missing, it will generate all the pending step definitions that need adding somewhere and output them to stdout.
If steps are unused, it will print out the filename and line number of the unused step so it can be removed. [Note: In order to do this, a slight change has been made to Spinach::DSL to have it keep track of steps as they're defined. I hope this is OK/useful!]
(The audit can also be run on individual feature files, but of course it may detect shared steps as being unused when they are in fact being used by other features! It doesn't do this when run over the whole directory.)
I have tried to keep to the coding style as much as possible and I've added a lot of scenarios to defend all the possible quirks I could think of.
Please let me know if you'd like any changes making and I hope you find this useful!