search

serenity-bdd / serenity-cucumber

Cucumber integration for the Serenity BDD Reporting library
Other
78 stars 74 forks source link

Fix serenity report generation when run by multiple tags #115

Closed Ram-Raghu closed 6 years ago

Ram-Raghu commented 6 years ago

Before this change when cucumber features or scenarios are run based on logical ORs and ANDs of two or more tags, serenity report fails to create tests report. This change would fix this issue and as a result when a user runs cucumber with tags as mentioned in this link (https://github.com/cucumber/cucumber/wiki/Tags) serenity report with tests on it would be generated.

Unit Tests in groovy are added and integratoin tests are added to the maven project in the path: serenity-cucumber\src\smoketests. The feature file name is serenity_report_when_using_tags_at_all_levels.feature and Junit test name is WhenUsingScenarioOutlineAndTagsAtAllLevels.java.

wakaleo commented 6 years ago

Yep, this is much nicer. Would it make sense to extract that entire logic in lines 403-462 into a separate class?

Ram-Raghu commented 6 years ago

Thanks for a swift response. of course. Isnt that following the "encapsulate what varies" and "open-closed" design principle? You know it better. Anyway, I am working on encapsulating the code block between those lines. Thanks. Ram.

Ram-Raghu commented 6 years ago

Hi @wakaleo

There is already a TaggedScenario class. From a quick look at the TaggedScenario class, It appears to me that it is meant for the serenity tags but not cucumber tags.

Could you advise whether creating a new class for the cucumber tags or updating the existing TaggedScenario be best?

Thanks Ram

wakaleo commented 6 years ago

This is more specific to Cucumber example tables, so I’d go for a new class unless you just need to add a few methods to TaggedScenario and reuse the existing logic (I don’t have the code in front of me so I’m not sure what TaggedScenario does)

Ram-Raghu commented 6 years ago

This pull request no longer required as the underlying issue has been fixed as part of #118