cucumber-attic / gherkin2

A fast Gherkin parser in Ragel (The parser behind Cucumber)
MIT License
381 stars 220 forks source link

Fixed corruption of internal Examples tags on FilterFormatter replay #286

Closed sirchia closed 10 years ago

sirchia commented 10 years ago

Replay method cleared examplesTags list, which is a direct reference to the Examples private administration of its Tags. I saw no reason why this would be desired/necessary for correct functioning of the FilterFormatter or other components.

I ran into this bug when I tried to implement some functionality in Cucumber that would take effect on absence/presence of tags on a(n outline) scenario (discussion and/or pull request on that part will follow...). The outline scenarios would not retain the tags on the example table until execution of the test, thus failing to trigger the tag-related functionality.

os97673 commented 10 years ago

please provide test for the problem you are fixing.

sirchia commented 10 years ago

Refer to the last commit for a test showing the problem

os97673 commented 10 years ago

I'm not sure but after your fix as far as I can see examplesTags updated only in examples() but replay() which uses them is called in several places which do not re-initialize examplesTags. Imho it looks suspicious. Most likely the fix introduces some problem (e.g. if we call examples() first and than scenario() will be called several time). Am I missing something? Also I wonder is the real problem is not in the formatter but in the API which returns internal modifiable data structure to user. @aslakhellesoy do you think it is ok to fix just the formatter?

brasmusson commented 10 years ago

Currently the replay() method fulfills the post condition that all 4 examplesX fields of the FilterFormatter has been reset and contains no data. Rather than changing this post condition by not calling examples.clear(), but still not changing the Examples internal Tag data structure, I would suggest to instead changing the assignment of exampleTags in the examples method to:

examplesTags.addAll(examples.getTags());

Then the call to examplesTags.clear() would not corrupt the Examples internal Tag data structure.

os97673 commented 10 years ago

yes, it would more accurate fix.

sirchia commented 10 years ago

I changed the pull request branch to reflect the desired approach. Reason I chose just to remove the clear was that the featureTags and featureElementTags fields were also set to the internal state of the model objects and FilterFormatter never appeared to clear those lists.

Do you think a clear of those would also be needed at the end of a replay as well to adhere to the same post condition on those levels? Of course they would also need to be modified to use addAll() instead of an assignment.

os97673 commented 10 years ago

No, we do not need to clean these collections they are reset in correct places: featureTags are set in feature(), and featureElementTags in scenario() or scenarioOutline() and they should be kept until next similar element.

jfstephe commented 9 years ago

Hi guys, Please can you let me know when this will be released? And the associated CucumberJVM release too? Thanks, John