AbstractTestNGCucumberTests runs each feature as separate test

[ushkinaz]

Previously AbstractTestNGCucumberTests reported all features as a single test, making it a little bit difficult to understand which feature have failed scenarios.
With this change AbstractTestNGCucumberTests uses TestNG data provider to generate separate tests for each feature.

Test output is much nicer, especially in IDE.
Compare old to new. Both are screenshots from IntelliJ Idea running java-calculator-testng example.

Exiting API of TestNGCucumberRunner is kept unchanged.

[ffbit]

Provided old and new screenshot images have just been attached to the PR to keep all things in one place and not depend on an exterior image hosting.
new

old

[ffbit]

Could you please avoid wildcard imports?

[ffbit]

@aslakhellesoy WDYT?

[aslakhellesoy]

I think all those protected fields should be private. I don't buy the argument that someone perhaps maybe might want to subclass this class in the future, and if they do I'm not convinced protected fields are the way to go. protected fields have a tendency to introduce bugs when they are altered in a way that wasn't expected by the class that declares them.

I'll take it for a spin when that's fixed.

[ffbit]

@ushkinaz Is it possible to squash last 4 commits? There are too many of them just for changes according to code review.
One of possible ways to do that is by running:

git checkout testNG-data-provider 
git branch testNG-data-provider-backup
git reset --soft HEAD~4
git commit

verify that there is nothing lost

git diff testNG-data-provider testNG-data-provider-backup

and then

git push -f origin testNG-data-provider

[ushkinaz]

Sure it's possible.
And rebasing is easier way to do that, and besides - it keeps commit comments, while soft reset doesn't.

[ffbit]

Thanks a lot.

[ushkinaz]

@ffbit, just in case it went unnoticed - commits are now squashed.

[ffbit]

Hi, @ushkinaz.
Thanks. I can't merge the PR myself because of [1], let's wait for someone to take a look at it and give us advice.

[1] I think that the story of Batman is quite interesting, but those extra files and counting number of them in the test are a bit out of place.

[ffbit]

@brasmusson,
Could you, please, take a look at this PR, if you've got some free time?
Thanks in advance.

[brasmusson]

@ffbit I actually did start to look at this PR a bit today, but so far not on the testing issues.

Since it is scenarios and not features that have passed/failed status in Cucumber, ideally both features and scenarios should appear in the TestNG tree. I do not know enough about TestNG to tell if that is possible or not, this PR is a step forward anyway.

The printing of snippets (and statistics) to help the user is removed. I think that the AbstractTestNGCucumberTest class should have a @afterClass method that prints the statistics and snippets through the TestNGCucumberRunner (using runtime.printSummary()).

I think the way to track if a feature pass or fail without creating a new runtime everytime, is to run the runtime with a custom Reporter (similar to the https://github.com/cucumber/cucumber-jvm/blob/master/junit/src/main/java/cucumber/runtime/junit/JUnitReporter.java), but only implementing the Reporter interface. In addition to the Reporter interface something like the following method would be needed by the TestNGCucumberRunner:

startFeature() // resets error state
featurePassed() // if no test failures or in strict mode also no underfined steps
getFirstFeatureError() // the error to wrap in a CucumberException and throw to notify TestNG about the failure

I'll will look at the testing in a day or two.

[brasmusson]

The content of this method should be extracted to TestNGCucumberRunner to simplify if anyone wants to both want to use composition (like in https://github.com/cucumber/cucumber-jvm/blob/master/examples/java-calculator-testng/src/test/java/cucumber/examples/java/calculator/RunCukesByCompositionTest.java) and get result for each feature separately.

[ushkinaz]

Most important part already extracted. See cucumber.api.testng.TestNGCucumberRunner#getFeatures.
There is no reason to extract the whole method, because it uses package local CucumberFeatureWrapper, which has very specific purpose. That class, CucumberFeatureWrapper, imho should be inner to AbstractTestNGCucumberTests, but it was extracted by request.

[brasmusson]

The Cucumber TestNG module has two API:s. First the AbstractTestNGCucumberTest - subclass it, annotate with @CucumberOptions, and go. Second it is the TestNGCucumberRunner which need to be used directly when having to let the runner class subclass some other class (for instance to inherit @BeforeClass/@AfterClass methods), this possibility was added in #622, and is exemplified by https://github.com/cucumber/cucumber-jvm/blob/master/examples/java-calculator-testng/src/test/java/cucumber/examples/java/calculator/RunCukesByCompositionTest.java. When they use the option to run each feature as a separate TestNG test, such runner classes are basically copies of AbstractTestNGCucumberTest. To support this kind of runner as good as possible, AbstractTestNGCucumberTest is best to contain only one line methods.

To really endorse the practice of running each feature as a separate TestNG test, this PR could rewrite https://github.com/cucumber/cucumber-jvm/blob/master/examples/java-calculator-testng/src/test/java/cucumber/examples/java/calculator/RunCukesByCompositionTest.java using the new methods in TestNGCucumberRunner.

[brasmusson]

@ffbit, @ushkinaz The test TestNGCucumberRunnerTest.getFeatures() is not dependent on the exact number of feature files in cucumber/api/testng/batman, just that all files with the extension .feature are feature files (an empty file with the extension .feature breaks the test). It seems to be a quite safe assumption so I'm pretty OK with the test. However, since there exist a runner (RunCukesTest.java) and feature files (cucumber/runtime/testng) in the module already, I would use them in the test instead of creating a new runner and new feature files.

The remaining actions on the PR that I see are:
[ ] Add and @AfterClass annotated method to AbstractTestNGCucumberRunner, which delegates to a new method in TestNGCucumberRunner that closes formatters and prints the summary. This need to be done explicitly when calling CucumberFeature.run to run features. Runtime.run shows what needs to be done after calling CucumberFeature.run for all features (if not done() and close() are called on formatters their result file will be empty or incomplete).
[ ] Extract the content of AbstractTestNGCucumberRunner.features to TestNGCucumberRunner (with only one liners in AbstractTestNGCucumberRunner, it will be easier to create runners that use composition like RunCukesByCompositionTest.java)
[ ] Revert TestNGCucumberRunner.runCukes to its old intended behaviour (run all features and then check for failues). Looking at the version of the method before this PR, there are two changes one would like to do; remove the printSummary call (it is already called by runtime.run(), and add the check for exit status to catch undefined/pending scenarios in strict mode (good catch @ushkinaz)
[ ] Change to not recreate the runtime for each feature, and instead use a custom reporter to determine the result of the feature (so that the snippets and summary can be printed correctly at the end)
[ ] Consider marking TestNGCucumberRunner.runCukes as deprecated (if in the future running one feature per TestNG test will be the only option supported by the TestNG module - also when for runner that uses composition)
[ ] If TestNGCucumberRunner.runCukes is marked as deprecated definitely, otherwise at least consider to rework the example runner that uses composition ,RunCukesByCompositionTest.java, to to run one feature per TestNG test.

[brasmusson]

@ushkinaz @ffbit I fixed my own comment on this PR in an own branch.
WDYT?

[ffbit]

@brasmusson could you please give me a couple of days to make review for you.

[brasmusson]

@ffbit Absolutely, take the time you need.

[brasmusson]

Sorry, clicked on the wrong button when trying to comment (had to use old browser)

[brasmusson]

Really sorry about duplicate comments and stuff, tried to comment form an environment where I was stuck on an outdated browser, that was a big mistake.

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