Skip to content
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

cucumber-jvm 5.x compatibility: feature ordering via tags and DocString enrichment via tags or content type #133

Merged
merged 8 commits into from
Apr 20, 2020

Conversation

andrewesweet
Copy link
Contributor

@andrewesweet andrewesweet commented Apr 17, 2020

As discussed in issue #117, feature and step comments are not supported in the out-of-the-box JSON output in cucumber-jvm 2.x and later.

This PR adds:

@coveralls
Copy link

coveralls commented Apr 17, 2020

Coverage Status

Coverage increased (+0.3%) to 80.162% when pulling ca8c351 on andrewesweet:master into de43e60 on rmpestano:master.

@rmpestano
Copy link
Owner

Oww very nice!

Can we skip the order tags in generated docs? I think we should not have cukedoctor related things in generated documentation, WDYT?

Thank you!

@rmpestano
Copy link
Owner

rmpestano commented Apr 17, 2020

Also, I almost forgot, there is another alternative which is to add comments to cucumber json report via a formatter, see here.

I didn't test the approach but if it works then we can use Cukedoctor with comments instead of creating custom tags.

@andrewesweet
Copy link
Contributor Author

Oww very nice!

Can we skip the order tags in generated docs? I think we should not have cukedoctor related things in generated documentation, WDYT?

Thank you!

Since writing that note about the tags remaining, it has annoyed me enough that I'm already hacking away at it. It adds a bit more complexity to the change as we now need to distinguish between a feature having tags and having renderable tags.

Also, I almost forgot, there is another alternative to add comments to cucumber json report via a formate, see here.

I didn't test the approach but if it works then we can use Cukedoctor with comments instead of creating custom tags.

I tried that approach, but it did not work for feature comments, only step comments. Maybe that approach could be extended to work for feature comments, but in my cursory examination I couldn't spot how to do it. I was also motivated by cucumber/cucumber-jvm#1329 (comment), which states that in general you cannot reliably list comments to Gherkin AST nodes, so we'd have no guarantee of compatibility. That being said, some of the commentary on the Gherkin 8 support project (https://github.com/orgs/cucumber/projects/6 and in particular cucumber/common#764 (comment)) suggests that they want to make this possible in future, although we'd like end up writing our own formatter (or future equivalent thereof) that would have to ship with cukedoctor.

@rmpestano
Copy link
Owner

Got it, let's keep the custom tags then.

For a new release I think we'll also need the @Asciidoc tag to enable Asciidoc in docstrings, can you work on that as well? take your time, no rush.

@andrewesweet
Copy link
Contributor Author

andrewesweet commented Apr 17, 2020

I've pushed a second commit to remove '@order-' tags from the rendered document, including the case where such a tag is the only tag.

As part of this, I’ve found a bug caused by a change in behaviour under cucumber-jvm - feature tags are automatically cascaded to scenarios in the json, but we render tags for a scenario by iterating through feature tags then scenario tags, hence we get a doubling up in the final output. I've raised a separate issue for that (#134). The fix should be easy enough.

Got it, let's keep the custom tags then.

For a new release I think we'll also need the @Asciidoc tag to enable Asciidoc in docstrings, can you work on that as well? take your time, no rush.

I intend to, but I'm not sure over what time period I'll be able to do it. Therefore, I'd prefer to do a PR per feature/fix, rather than one big "2.x support" PR, if that's agreeable for you? If you agree, perhaps we can create a more granular "2.x support" list or "2.0.0 milestone" list?

@andrewesweet
Copy link
Contributor Author

I've spotted a way to make this change simpler - I'll push another commit later today, I hope

@rmpestano
Copy link
Owner

I'd prefer to do a PR per feature/fix

Sure, that's the Idea! Thank you very much for your contribution so far!

@andrewesweet
Copy link
Contributor Author

Pushed. By fixing #134 the implementation has become much more succinct. Ready for merge, subject to your review.

…tor-discrete' or via feature/scenario tag '@cukedoctor-discrete'. Also fixed some test assertions which failed on Windows.
@andrewesweet andrewesweet changed the title Added support for feature ordering via a feature tag cucumber-jvm 5.x compatibility: feature ordering via tags and DocString enrichment via tags or content type Apr 19, 2020
@andrewesweet
Copy link
Contributor Author

andrewesweet commented Apr 19, 2020

After saying I did not want one PR to rule them all, that's what I've generated. This PR now:

  1. Allows feature ordering via feature tags
  2. Enrichment of DocStrings via a custom content-type (as suggested by @msche in Hack for fact that comments are no longer included in cucumber json output #132) or via a feature or scenario tag (as suggested by @rmpestano in Cucumber-jvm 2.x support #117).
  3. Test assertion fixes to allow the full test suite to be executed under Windows (mostly about formatting line endings and one instance of formatting paths).

Given (3), I'd appreciate someone running the test suite under *nix and confirm it is still passing successfully.

(2) required a breaking change to the spi.StepsRenderer interface, so the renderer is aware of tags on the to-be-rendered Step's Scenario and Feature. I couldn't think a way of doing this without some form of break on the interface (even if "only" on the model types Step and DocString) and given the ScenarioRenderer is already passed the to-be-rendered scenario's Feature, my change follows the established pattern.

@rmpestano
Copy link
Owner

rmpestano commented Apr 19, 2020

Given (3), I'd appreciate someone running the test suite under *nix and confirm it is still passing successfully.

It passes on MacOS here, the travis build runs on top of ubuntu so I think if travici passes then we are ok.

(2) required a breaking change to the spi.StepsRenderer interface...

Don't worry about that, we are going to release Cukedoctor 2.x to support cucumber 2.x onwards so users will expect breaking changes.

As we are introducing the tags, can't we have @cukedoctor instead of @cukedoctor-discrete? I'm also inclined to use @asciidoc if possible. WDYT?

Also, thank you again for your awesome contribution!

@andrewesweet
Copy link
Contributor Author

andrewesweet commented Apr 19, 2020

As we are introducing the tags, can't we have @cukedoctor instead of @cukedoctor-discrete? I'm also inclined to use @asciidoc if possible. WDYT?

I'm happy to change this. Whilst cukedoctor-discrete is a bit verbose, it gives consistency with the step comment approach #cukedoctor-discrete comments. I've updated it to use @asciidoc for tags and asccidoc for content type.

@rmpestano rmpestano added this to the 2.0 milestone Apr 20, 2020
@rmpestano rmpestano merged commit 9c06785 into rmpestano:master Apr 20, 2020
@rmpestano
Copy link
Owner

I'll make a couple of tests during this week (jenkins plugin) and then release 2.0 on next weekend, ok?

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants