Implement tag filtering with limits

[tom025]

Closes #50.

This adds a TagCounter that counts the tags associated with a test step and
the location of the test step.

TagLimits can be constructed from a list of filter expressions then used to
enforce the limits defined in the filter expressiont by passing a TagCounter
to TagLimits#enforce.

A TagExcess will be raised if any breaches of the limts are found.

[mattwynne]

limit_breaches.any? would read better here, IMO

[os97673]

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.

[tom025]

Thanks for the feedback @os97673!

I was unaware of the Gherkin::TagExpression class.

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 the Gherkin::TagExpression?

[os97673]

As far as I understand Gherkin::TagExpression is 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 :)

[mattwynne]

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::TagExpression class is useful for evaluating complex boolean expressions, but do we do that in tag filters? Seems like a not operator is about as complex as it gets. Might not be worth adding the dependency.

[os97673]

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.

[tom025]

I think that Gherkin::TagExpression has too many responsibilities. As far as I can tell it

1. Parses raw tag expressions into a collection representing a boolean expression

2. Evaluates the expression for tags passed in. (Used for deciding if a test case should be run or not)

3. 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::TagExpression all together as it is not used in cucumber anymore and only used in one place for cucumber-ruby-core.

[mattwynne]

On 19 Jan 2014, at 19:44, Thomas Brand notifications@github.com wrote:

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.

I agree.

[aslakhellesoy]

I don't think tag limits should be added to Cucumber core. I asked on the list a few months ago[1], and it seems very few people are using it. It makes much more sense to be a plugin.

I also think we should start using bool[2] to parse boolean expressions. That would allow us to get rid of the ridiculous mechanism we have now for logical and/or, which nobody seems to understand. It's released as a gem[3].

[1] https://groups.google.com/d/msg/cukes/9mRU6N88XUg/-cuI4i38qFYJ
[2] https://github.com/cucumber/bool
[3] http://rubygems.org/gems/bool

[tom025]

As we are trying to be compatible with cucumber 1.3 we need to support tag limits somewhere in the various codebases and the easiest place is in cucumber-core IMO. I think it makes sense to support tag limits in cucumber core as a stop gap then get rid of it for cucumber 3.0 and maybe develop a plugin for backwards compatibility if there is a pressing need for it.

[mattwynne]

Agreed Tom. Let's minimise the noise and disruption for 2.0, but let's keep the code clearly separated so we're ready to ditch tag limits and switch to bool ASAP.

[tom025]

BTW going through the various pieces of code that were historically involved in tag limits I think this PR is not 100% compatible with cucumber 1.3 as it doesn't take into account expressions like --tags '@one:1,@two:2' so I don't think this PR should be merged just yet.

There are also some names that I don't think make sense like TagCounter actually accumulates TestCases. I think this should be renamed to TestCases or TestCaseList.

I think I know enough about bool and test case matching to have a go at integrating bool into cucumber-core. I can do that as the next thing I work on.

[tom025]

I think that this now ready to merge if everyone else agrees?

[os97673]

Looks fine for me.

[aslakhellesoy]

FWIW, Bool will also have to be integrated into Gherkin itself, since Gherkin uses tags for filtering features/scenarios. (Cucumber uses tags to match hooks).

[tooky]

I think it would be better style to use update inside the reduce block, or each_with_object instead of reduce here, so we don't have the bare limit_list returned from the block.

e.g.

reduce({}) do |limit_list, matchdata|
  limit_list.update(matchdata[:tag_name] => Integer(matchdata[:limit]))
end

or

each_with_object({}) do |limit_list, matchdata|
  limit_list[matchdata[:tag_name]] = Integer(matchdata[:limit])
end

/picky

[mattwynne]

On 21 Jan 2014, at 05:20, Aslak Hellesøy notifications@github.com wrote:

FWIW, Bool will also have to be integrated into Gherkin itself, since Gherkin uses tags for filtering features/scenarios. (Cucumber uses tags to match hooks).

At the moment, we're no longer using Gherkin for that in 2.0. We're parsing everything, compiling it all into test-cases and then filtering the test-cases. This may turn out to be a mistake from a performance point of view, but I wanted to try and reduce the coupling to Gherkin. We can easily switch back to using parser-based filtering if it turns out to be slow.

[pdswan]

This will raise the TagExcess exception only after running all the scenarios. The current version of cucumber raises the exception before running any scenarios which I think is the desired behavior?

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.

[tom025]

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.

[tom025]

See #54.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.