-
Notifications
You must be signed in to change notification settings - Fork 9
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 UniqueScenarioNamesLinter for enforcing unique scenario names #30
base: master
Are you sure you want to change the base?
Conversation
* Unique names linter * Refactoing test and fix feature name * Refactoring tests * Refactoring: spec test * Refactoring: linter rule * Refactoring: linter and tests * Fix test description * Fix problem detection within a rule * Fix: remove extra method * Refactoring by review * Fix: DRY spec test * Refactoring linter and more tests * Fixed linter * Refactoring linter and tests * Refactoring with rubocop
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'll fiddle with the feature file this weekend and, from that, see what shakes out as needing changing in the implementation and RSpec tests. Sorry about not getting to this sooner.
'TestWithTooManyStepsLinter' => TestWithTooManyStepsLinter.new, | ||
'UniqueScenarioNamesLinter' => UniqueScenarioNamesLinter.new } |
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.
New linters should not be added to the default list. The list of linters that are active by default can be updated on major releases. Users can activate them explicitly if they want them sooner.
@@ -33,7 +33,7 @@ Feature: Default Linters | |||
| TestWithSetupStepAfterVerificationStepLinter | | |||
| TestWithSetupStepAsFinalStepLinter | | |||
| TestWithTooManyStepsLinter | | |||
|
|||
| UniqueScenarioNamesLinter | |
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.
See above comment regarding default linters.
Additionally, this test would need to be updated when new default linters are added. The Cucumber features are executable documentation for users, so it is important that they are up to date too, but the RSpec tests should cover everything on their own and prove the functionality of the gem, including the parts that aren't worth mentioning at the higher level of documentation.
@@ -0,0 +1,189 @@ | |||
Feature: Unique scenario names |
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 feature file needs another pass. Several of the example features don't demonstrate the use case that their scenario names state.
Scenario: Linting (Bad: Duplicates of regular scenario name and one from a Rule) | ||
Given the following feature: | ||
""" | ||
Feature: Sample Feature | ||
|
||
Rule: Example Rule | ||
Scenario: Duplicate Scenario Name | ||
Given something | ||
|
||
Scenario: Duplicate Scenario Name | ||
Given something | ||
""" |
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 'regular' scenario here is not one. Once the Rule
keyword is used, all scenarios that come after it will be considered part of that rule or part of a later Rule
. Regular scenarios have to come before any usage of Rule
in a feature.
UniqueScenarioNamesLinter ensures that all scenario names within a feature file or logical block (e.g., Rule) are unique.