[JUnit] Add --[no-]step-notifications options to JunitOptions #1135

brasmusson · 2017-05-27T14:35:40Z

Summary

Improve surefire integration.

Motivation and Context

Because cucumber presents the steps as tests to junit surefire provides
an incorrect test counts and reports strange names. By adding
--[no-]step-notifications to JunitOptions this can be avoided.

Related Issues:

How Has This Been Tested?

Added PickleRunnerWithNoStepDescriptionsTest and PickleRunnerWithStepDescriptionsTest to
ensure both work as expected.

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).

Checklist:

I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly.

brasmusson

@mpkorstanje I think the handling of the different versions of AssumptionViolatedException needs to be fixed. Apart from that I think the PR looks good.

brasmusson · 2017-05-27T18:02:55Z

junit/src/main/java/cucumber/runtime/junit/JUnitReporter.java

 import gherkin.pickles.PickleStep;
-import org.junit.internal.runners.model.EachTestNotifier;
+import org.junit.internal.AssumptionViolatedException;


AFAIK this is the JUnit <4.12 class for the assume feature, JUnit 4.12 use another class, as we handle both classes in other places we should do it here to (see comment on usage).

brasmusson · 2017-05-27T18:13:52Z

junit/src/main/java/cucumber/runtime/junit/JUnitReporter.java

+        public void addFailure(Throwable targetException) {
+            if (targetException instanceof MultipleFailureException) {
+                addMultipleFailureException((MultipleFailureException) targetException);
+            } else if (targetException instanceof AssumptionViolatedException) {


The different versions of AssumtionViolatedExceptions are kept in the array PENDING_EXCEPTIONS, isPending() searches the array, it could done here to (if also removing the cast of the argument to addFailedAssumption - as it uses its argument it could accept a Throwable)

As the PendingException now is identified with an annotation instead of as a member of PENDING_EXCEPTIONS, there is a case for renaming PENDING_EXCEPTIONS to something with ASSUMPTION_VIOLATED or something - but it does not directly has anything to do with this pull request.

Mmh. Good point. I'm not happy about using junit internal here either.

mpkorstanje · 2017-05-27T19:40:26Z

@brasmusson Fixed and I've added tests for good measure.

When interacting with cucumber in an IDE it is nice to see the step executions. However presenting step executions to surefire as tests results in strange test counts and weird reports. The --no-step-notifications options ensures steps are not included in descriptions and no events are fired for the execution of steps. Related issues: - #263 - #577

* Extract Runtime.isAssumptionViolated from Runtime.isPending. * Use Runtime.isAssumptionViolated as the condition for fireTestAssumptionFailed

mpkorstanje · 2017-05-28T13:17:03Z

junit/src/main/java/cucumber/runtime/junit/JUnitReporter.java

@@ -202,7 +203,7 @@ public void fireTestFinished() {
        public void addFailure(Throwable targetException) {
            if (targetException instanceof MultipleFailureException) {
                addMultipleFailureException((MultipleFailureException) targetException);
-            } else if (isPending(targetException)) {
+            } else if (isAssumptionViolated(targetException)) {


Do you intend to count the pending exception as a failure? I'm not entirely sure how pending items should be reported? I guess this should depend on the strict setting.

The Cucumber-JVM statuses passed, skipped, undefined, pending, failed are mapped to the JUnit statuses passed, skipped, failure, error, as:

passed -> passed

skipped -> skipped

failed -> failure

undefined, pending -> skipped unless in strict mode where they are mapped to failure
We do not end up here for a pending result unless in strict mode, if in non-strict mode we go straight to fireTestIgnored

With this PR there is a change in how AssumptionViolatedExceptions are notified, previously they would cause a fireTestIgnored in non-strict mode and fireTestFailure in strict mode, now they will cause a fireTestIgnored in non-strict mode and fileTestAssumptionFailed in strict mode. I intended to accept that and view it as a part of the fix to #1007.

Now that I know about the fileTestAssumptionFailed I think it should be used instead of fireTestIgnored to signal "skipped" for passed, skipped, undefined and pending statuses (the latter only in non-strict mode of course), but that is another PR.

Makese sense.

mpkorstanje · 2017-05-28T13:38:50Z

@brasmusson

~~I found a bug in FeatureRunner. It is not properly notifying signaling the start of a test when steps are suppressed. I'll fix it later today.~~

That made not sense. Surefire is using a test name as the name of scenario. I think it's cause by misfiring a notifier but I can't say for sure.

<testsuite xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="https://maven.apache.org/surefire/maven-failsafe-plugin/xsd/failsafe-test-report.xsd" name="Say hello" time="0.367" tests="8" errors="0" skipped="0" failures="1">

mpkorstanje · 2017-05-28T15:08:01Z

@brasmusson

It appears there are lots of implicit assumptions in junit4/surefire about the name of tests. Somebody already reported it as #865.

So I think we're good to go here. This problem needs its own PR.

Also update History.md.

longtimeago · 2017-06-30T09:19:16Z

@mpkorstanje Could we release a new version with this fix? Rather annoying bug :(

mpkorstanje · 2017-06-30T09:30:20Z

Hey @longtimeago we're currently working on releasing 2.0.0. There are a few issues we have to clean up first and then we'll be good to go. This may take some time as we're working with 3 people and spare time at the moment.

You can however get our snapshot builds by using:

<repository>
    <id>sonatype-snapshots</id>
    <url>https://oss.sonatype.org/content/repositories/snapshots</url>
    <snapshots>
        <enabled>true</enabled>
    </snapshots>
</repository>

<dependency>
    <groupId>io.cucumber</groupId>
    <artifactId>cucumber-java</artifactId>
    <version>2.0.0-SNAPSHOT</version>
    <scope>test</scope>
</dependency>

longtimeago · 2017-06-30T11:05:56Z

@mpkorstanje Sure, no problem :) Just wanted to know some if there is a movement towards that :)
And it's not SO annoying to use snapshots :D

lock · 2018-10-25T01:13:28Z

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.

brasmusson force-pushed the junit-no-step-notification-option branch from 3affec7 to a9605e9 Compare May 27, 2017 14:50

brasmusson commented May 27, 2017

View reviewed changes

mpkorstanje force-pushed the junit-no-step-notification-option branch 2 times, most recently from d46384f to a2f06e0 Compare May 27, 2017 19:22

mpkorstanje force-pushed the junit-no-step-notification-option branch from f2888f0 to a2f06e0 Compare May 27, 2017 19:45

mpkorstanje force-pushed the junit-no-step-notification-option branch from a2f06e0 to 6beaa7e Compare May 27, 2017 20:24

This was referenced May 28, 2017

Proposal to fix Issue577 -- improved junit runner #588

Closed

Eclipse JUnit reports inaccurate run count #263

Closed

[JUnit] fireTestAssumptionFailed only for violated assumptions.

b0b26a3

* Extract Runtime.isAssumptionViolated from Runtime.isPending. * Use Runtime.isAssumptionViolated as the condition for fireTestAssumptionFailed

mpkorstanje mentioned this pull request May 28, 2017

maven surefire test results are wrong (cucumber-java) #577

Closed

mpkorstanje reviewed May 28, 2017

View reviewed changes

brasmusson merged commit b0b26a3 into master May 28, 2017

brasmusson added a commit that referenced this pull request May 28, 2017

Merge #1135 'Add --[no-]step-notifications JUnit option'

a38911e

Also update History.md.

brasmusson deleted the junit-no-step-notification-option branch May 28, 2017 16:53

mpkorstanje mentioned this pull request Jun 11, 2017

Restructured JUnit class/method name reporting #626

Closed

mpkorstanje mentioned this pull request Jun 18, 2017

A single failure is duplicated in the failsafe output #935

Closed

mpkorstanje mentioned this pull request Mar 23, 2018

SUREFIRE-1372 Filter tests to be rerun by description apache/maven-surefire#150

Closed

mpkorstanje mentioned this pull request Jul 26, 2018

JUnit Run Viewer does not show Given When Then hierarchy #1425

Closed

lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JUnit] Add --[no-]step-notifications options to JunitOptions #1135

[JUnit] Add --[no-]step-notifications options to JunitOptions #1135

brasmusson commented May 27, 2017

brasmusson left a comment

brasmusson May 27, 2017

brasmusson May 27, 2017

mpkorstanje May 27, 2017

mpkorstanje commented May 27, 2017

mpkorstanje May 28, 2017

brasmusson May 28, 2017

mpkorstanje May 28, 2017

mpkorstanje commented May 28, 2017 •

edited

Loading

mpkorstanje commented May 28, 2017

longtimeago commented Jun 30, 2017

mpkorstanje commented Jun 30, 2017

longtimeago commented Jun 30, 2017

lock bot commented Oct 25, 2018

[JUnit] Add --[no-]step-notifications options to JunitOptions #1135

[JUnit] Add --[no-]step-notifications options to JunitOptions #1135

Conversation

brasmusson commented May 27, 2017

Summary

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

brasmusson left a comment

Choose a reason for hiding this comment

brasmusson May 27, 2017

Choose a reason for hiding this comment

brasmusson May 27, 2017

Choose a reason for hiding this comment

mpkorstanje May 27, 2017

Choose a reason for hiding this comment

mpkorstanje commented May 27, 2017

mpkorstanje May 28, 2017

Choose a reason for hiding this comment

brasmusson May 28, 2017

Choose a reason for hiding this comment

mpkorstanje May 28, 2017

Choose a reason for hiding this comment

mpkorstanje commented May 28, 2017 • edited Loading

mpkorstanje commented May 28, 2017

longtimeago commented Jun 30, 2017

mpkorstanje commented Jun 30, 2017

longtimeago commented Jun 30, 2017

lock bot commented Oct 25, 2018

mpkorstanje commented May 28, 2017 •

edited

Loading