-
-
Notifications
You must be signed in to change notification settings - Fork 51
Implement tag filtering with limits #51
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
Changes from all commits
9cdd5d8
5561d60
f7a01d9
ad5f266
6c195ab
e18f345
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| module Cucumber | ||
| module Core | ||
| module Test | ||
| class TagFilter | ||
| include Cucumber.initializer(:filter_expressions, :receiver) | ||
|
|
||
| def test_case(test_case) | ||
| test_cases << test_case | ||
| if test_case.match_tags?(filter_expressions) | ||
| test_case.describe_to(receiver) | ||
| end | ||
| self | ||
| end | ||
|
|
||
| def done | ||
| tag_limits.enforce(test_cases) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will raise the Since parsing, compilation, and execution are part of the same chain, replicating the existing behavior seems a bit complex at least to a cucumber codebase n00b. Also possible I'm completely missing something. When I was looking into it I contemplated an extra parsing pass outside of core but that seemed a bit gnarly.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for bringing this up @pdswan! You are correct that the current version of cucumber will raise an error before it starts running the scenarios. @mattwynne and I found this bug last night when pairing on this. A issue needs to be raised for this. I am not sure how to fix this issue at the moment as I don't have a complete understanding of the new execution model. Perhaps @mattwynne and @tooky can provide some guidance for this issue. It is also worth noting that @mattwynne and I have decided to move all of the code that parses to raw filter expressions to the main cucumber code base and pass in the tag limits and tag filters into cucumber-core at execution time. See #53.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #54. |
||
| receiver.done | ||
| self | ||
| end | ||
|
|
||
| private | ||
| def test_cases | ||
| @test_cases ||= TestCases.new | ||
| end | ||
|
|
||
| def tag_limits | ||
| @tag_limits ||= TagLimits.new(filter_expressions) | ||
| end | ||
|
|
||
| class TestCases | ||
| attr_reader :test_cases_by_tag_name | ||
| private :test_cases_by_tag_name | ||
| def initialize | ||
| @test_cases_by_tag_name = Hash.new { [] } | ||
| end | ||
|
|
||
| def <<(test_case) | ||
| test_case.tags.each do |tag| | ||
| test_cases_by_tag_name[tag.name] += [test_case] | ||
| end | ||
| self | ||
| end | ||
|
|
||
| def with_tag_name(tag_name) | ||
| test_cases_by_tag_name[tag_name] | ||
| end | ||
| end | ||
|
|
||
| class TagLimits | ||
| TAG_MATCHER = /^ | ||
| (?:~)? #The tag negation symbol "~". This is optional and not captured. | ||
| (?<tag_name>\@[\w\d]+) #Captures the tag name including the "@" symbol. | ||
| \: #The seperator, ":", between the tag name and the limit. | ||
| (?<limit>\d+) #Caputres the limit number. | ||
| $/x | ||
|
|
||
| attr_reader :limit_list | ||
| private :limit_list | ||
| def initialize(filter_expressions) | ||
| @limit_list = Array(filter_expressions).flat_map do |raw_expression| | ||
| raw_expression.split(/\s*,\s*/) | ||
| end.map do |filter_expression| | ||
| TAG_MATCHER.match(filter_expression) | ||
| end.compact.each_with_object({}) do |matchdata, limit_list| | ||
| limit_list[matchdata[:tag_name]] = Integer(matchdata[:limit]) | ||
| end | ||
| end | ||
|
|
||
| def enforce(test_cases) | ||
| limit_breaches = limit_list.reduce([]) do |breaches, (tag_name, limit)| | ||
| tag_count = test_cases.with_tag_name(tag_name).count | ||
| if tag_count > limit | ||
| tag_locations = test_cases.with_tag_name(tag_name).map(&:location) | ||
| breaches << TagLimitBreach.new( | ||
| tag_count, | ||
| limit, | ||
| tag_name, | ||
| tag_locations | ||
| ) | ||
| end | ||
| breaches | ||
| end | ||
| raise TagExcess.new(limit_breaches) if limit_breaches.any? | ||
| self | ||
| end | ||
| end | ||
|
|
||
| TagLimitBreach = Struct.new( | ||
| :tag_count, | ||
| :tag_limit, | ||
| :tag_name, | ||
| :tag_locations | ||
| ) do | ||
|
|
||
| def message | ||
| "#{tag_name} occurred #{tag_count} times, but the limit was set to #{tag_limit}\n " + | ||
| tag_locations.map(&:to_s).join("\n ") | ||
| end | ||
| alias :to_s :message | ||
| end | ||
|
|
||
| class TagExcess < StandardError | ||
| def initialize(limit_breaches) | ||
| super(limit_breaches.map(&:to_s).join("\n")) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| 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.
Is filter_expression we use in initializer is Gherkin teg expression? If so it aleady has both tags and limits and we do not have to re-perse it here.
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 the feedback @os97673!
I was unaware of the
Gherkin::TagExpressionclass.I will update this to use the
Gherkin::TagExpression. While I am at it, is it worthwhile putting the code for enforcing the tag limits into theGherkin::TagExpression?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.
As far as I understand
Gherkin::TagExpressionis not supposed to be used to keep runtime information :(So, it can not enforce tag limits itself.
Also it would be a pain to release one more version of gherkin :)
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.
Please bear in mind #20. I'd like to add as few couplings to Gherkin as possible from cucumber-core as it's likely to be changed significantly in the future. The
Gherkin::TagExpressionclass is useful for evaluating complex boolean expressions, but do we do that in tag filters? Seems like anotoperator is about as complex as it gets. Might not be worth adding the dependency.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 agree that we should avoid dependency on Gherkin, but either Gherkin or bool will always have something to represent tag expression and I se no point to duplicate parsing code here. Perhaps we should just pass all necessary information instead of parsing text again.
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 that
Gherkin::TagExpressionhas too many responsibilities. As far as I can tell itParses raw tag expressions into a collection representing a boolean expression
Evaluates the expression for tags passed in. (Used for deciding if a test case should be run or not)
Parses raw tag expressions into a list of tag names with limits. (Used for enforcing limits).
I think the tags with limits are a separate concern from evaluating a boolean expression and we should not mix those concepts, even if we are parsing the filter expressions twice.
It looks like we want to use bool for the evaluation of boolean expressions anyway so we may want to deprecate
Gherkin::TagExpressionall together as it is not used in cucumber anymore and only used in one place for cucumber-ruby-core.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.
On 19 Jan 2014, at 19:44, Thomas Brand notifications@github.com wrote: