Improve plugin and formatter structure

[mpkorstanje]

Summary

The following improvements have been made

1. All plugin interfaces (Formatter, StepDefinitionReporter,
   SummaryPrinter) now extend the Plugin interface.

2. Updated documentation to make it clear what each plugin does.

3. All plugins have been made final. They are not designed for
   extension.

4. Moved android formatters into formatter package to limit visibility
   of TestSourcesModel.

5. Classes in runtime/android have been made final and have had their
   visibility reduced. They are not designed for extension.

Motivation and Context

Discussion on Slack showed that TestSourcesModel was still available for use. TestSourcesModel is not part of the API so its use should be discouraged. While doing so the fact that none of the plugins actually implemented the plugin interface bugged me to no end.

How Has This Been Tested?

Ran the tests, fixed the tests.

Types of changes

The changes to the api should be backwards compatible as an interface has been added to the interfaces that were already used. It is possible that plugin used none of these interfaces, but such a plugin would have no effect.

• Bug fix (non-breaking change which fixes an issue).
• New feature (non-breaking change which adds functionality).
• Breaking change (fix or feature that would cause existing functionality to not work as expected).

[coveralls]

Coverage increased (+0.006%) to 81.233% when pulling d32558a on fix-plugin-isssues into ca3c2e7 on master.

[coveralls]

Coverage increased (+0.006%) to 81.247% when pulling d115e13 on fix-plugin-isssues into 2dc8bb8 on master.

[aukevanleeuwen]

Hello @mpkorstanje ,

this PR breaks a whole lot of things with the Extend report plugin (https://github.com/sitture/cucumber-jvm-extentreport). I haven't taken an in depth look, since this is a library that I've only noticed yesterday, but it breaks here:

https://github.com/sitture/cucumber-jvm-extentreport/blob/c215583b161a9056e4460e69fd946f04b39eb643/src/main/java/com/sitture/ExtentFormatter.java#L119-L142

because it's using the TestSourcesModel.isBackgroundStep(), which is no longer visible.

I've had a look at the current Formatters in this cucumber-jvm project (such as JSONFormatter.java, but they all seem to be using this TestSourcesModel.

At first sight it seems reasonable that one should be able to create a Formatter in a similar way that the 'standard' formatters are programmed? Any thoughts? Related issue on the cucumber-jvm-extentreport side: sitture/cucumber-jvm-extentreport#15.

Note: I'm not the developer of the mentioned formatter, so I don't know all the ins-and-outs.

[mpkorstanje]

Hey Auke, it is generally speaking a bad practice to depend on the implementation details of another project. Anything outside of the api packages may change between versions. If it didn't break now it would have permanently broken when using java 9. So for a short term fix I'd recommend copying the TestSourcesModel.

For a longer term fix it might be a good strategy to add the AST-like features to Gherkin.

[aukevanleeuwen]

Of course I understand that you should depend on api only, but I was confused by this TestSourcesModel which every formatter seem to use. Again, I'm not very familiar with the complete project, but it seemed sort of logical to me that knowing whether or not a step was a background step was included as part of the API. I.e. event.testStep.isBackgroundStep().

So my question was more or less in that direction, or thinking that TestSourcesModel should have been part of the API. Since most 'standard' formatters (those inside cucumber-jvm) seem to use the TestSourcesModel as well I thought it was maybe 'missed' in terms of being exported as part of the API.

[mpkorstanje]

Cucumber has been abstracted away from from gherkin. It operates on pickles rather then steps. Background steps and other steps are all identical in cucumber. As a result any formatter that wants to work with gherkin has to match line numbers back to the AST.

If you need this it might be useful to create a PR against the gherkin repository and add it in the line number lookup there.

[brasmusson]

Another way to put it is that testStep.isBackgroundStep() is semantically wrong, a test step never "is" a background step, it may "come from" a background step, but it does not need to know that and it should not know that. Pickles and event more so Test Cases/Steps (Pickles + mapped step definitions + hooks) are focused on executing the Test Cases/Steps. If the successful execution Test Step depended on knowing whether it came from a Gherkin Background Step or not, something would be very wrong.

Things that are exported as part of the API are in Martin Fowler's terms "Published Interfaces". One of Martin Fowler's guidelines for Published Interfaces is "Publish as little as you can as late as you can". "As late as you can" so that you have had the time to refine the interface to be published. TestSourcesModel is far from being as refined to be part of a "Published Interface". Secondly as mentioned in previous comment, the proper place for a support library for lookup of Gherkin AST data, is most likely a sub-repo of https://github.com/cucumber/cucumber so it can be used by all Cucumber implementations and not only Cucumber-JVM (maybe rather the event-protocol sub-repo rather than the gherkin sub-repo).

[aukevanleeuwen]

First off: just wanted to let you guys know that I appreciate the work you're doing here. This is not meant as pure criticism, but simply trying to (1) understand it, (2) give some (hopefully) constructive feedback.

---

Another way to put it is that testStep.isBackgroundStep() is semantically wrong, a test step never "is" a background step, it may "come from" a background step, but it does not need to know that and it should not know that. Pickles and event more so Test Cases/Steps (Pickles + mapped step definitions + hooks) are focused on executing the Test Cases/Steps. If the successful execution Test Step depended on knowing whether it came from a Gherkin Background Step or not, something would be very wrong.

It seems to me that this is valid mainly from the point of view of executing the steps. I understand that you don't really care about where it was coming from. However, judging from the fact that the formatters used in the creation of reports (HTMLFormatter, PrettyFormatter, JSONFormatter) all seem to want to dig down into the Gherkin step information this abstraction is less reasonable from a reporting point of view.

So I've read up a bit on the Pickles abstraction a bit.

The rationale is to decouple Gherkin from Cucumber so that Cucumber is open to support alternative formats to Gherkin (for example Markdown).

Which makes perfect sense, but - assuming that you want to leverage this abstraction for your Formatters as well - I'm wondering if the abstraction isn't a little too high for both these purposes.
Suppose there would be a Markdown implementation (maybe there is?) how do you do your reporting on that? Would you want to write a MarkdownHTMLFormatter for this purpose because you need to dig into that AST to find out if it's a background step or not? What is somebody decides to write half of it's steps in Gherkin and half of them in Markdown? I would like to have a consistent HTML report over that I suppose and this seems non-trivial to achieve.

Basically I'm wondering

1. if the Pickles abstraction should include the information needed for the reporting as well (so basically having the same abstraction for execution and reporting);
2. or have two separate abstractions one of them being 'fed' to the execution engine, one of them for reporting? This should still be an abstraction over Gherkin to support Markdown in the future.

In general it would be nice for the 'native' formatters (HTMLFormatter, PrettyFormatter, JSONFormatter) to 'eat your own dogfood' and be able to be implemented by only using the API you provide yourselves without having to dig into the underlying AST of whatever 'language' your using to describe your tests.

[aslakhellesoy]

The pickles abstraction is designed for execution only. It is suitable for simple formatting where the format doesn't attempt to replicate the input format. For example, progress bar, counting success/failure etc.

In the past the same data structure was used to represent the AST and the results. This led to very complicated internals, so we decided to split it.

If you want to generate a report that reflects the input, you have to consume source and/or gherkin-document events as well. These events contain the raw source and AST respectively.

The pickle events have soft references to the source and gherkin-document via uri and location fields.

The design is based on event sourcing, and by consuming relevant events you can construct a variety of output formats.

The long term goal is to expose all of these events as a stream of ndjson and delegate formatting to a separate process. For example, there would be a cucumber-html-formatter executable that reads events from STDIN and generates a rich report on disk. This formatter would then work for any Cucumber implementation, and we won't have to maintain formatters for several languages. These reusable formatters could be written in any language, preferrably something that is easy to cross-compile, easy to maintain for contributors, starts up fast and is easy to install for end users.

We've yet to write any of those formatters.

[aukevanleeuwen]

Thanks for taking the time to explain, this makes sense.

I'm slightly sceptical about the out of process external reporting. I might very well misunderstand the exact details, but I do understand how this would lighten the burden over several languages. However I also expect Maven or Gradle users to just be able to have a single plugin that does everything. Having an out of process extra process during a Maven build seems difficult to achieve, but maybe I'm totally wrong there :-)

Thanks.

[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.