[JUnit] Use AssumptionFailed to mark scenarios/steps as skipped #1142

brasmusson · 2017-06-06T13:52:56Z

Summary

Send TestAssumptionFailed notifications instead of TestIgnored notifications to mark scenarios/steps as skipped.

Details

Since it is only after a step or scenario has been executed we can tell if it should be considered skipped in JUnit terms, TestAssumptionFailed notifications suits better than TestIgnored notifications.

By using TestAssumptionFailed to mark scenarios/steps as skipped, TestStarted can always be fired before the scenario/step is executed, therefore the option --allow-stared-ignored is not longer needed.

Motivation and Context

By using TestIgnored notifications, the passing of the TestStarted had to be delayed until after the step has executed, which is both unexpected and an unnecessary complex solution. It also caused the need for the --allow-stared-ignored option (which adds more complexity).

How Has This Been Tested?

In addition to the automatic tests of the junit module, I have experimented with the java-calculator example from within Eclipse and with maven.

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality). Since the --allow-started-ignored option is only deprecated it is a non-breaking change.
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly (the help for the junit options).

mpkorstanje

I don't quite follow all the changes to the JUnitReporter but it appears the tests are there and cover all cases.

#   | Pickle Event            | Junit Event                   | Test                                                                                               |
1   | TestCaseStarted         | runNotifier.fireTestStarted   | test_case_started_fires_test_started_for_pickle                                                    |
2a  | TestStepStarted (hook)  | ignored                       |                                                                                                    |
2a  | TestStepFinished (hook) | ignored                       |                                                                                                    |
3a  | TestStepStarted (step)  | stepNotifier.fireTestStarted  | test_step_started_fires_test_started_for_step                                                      |
3b  | TestStepFinished (step) | notifyResult(stepNotifier)    | test_step_finished_fires_only_test_finished_for_passed_step                                        |
    |                         | stepNotifier.fireTestFinished | test_step_finished_fires_assumption_failed_and_test_finished_for_skipped_step                      |
    |                         |                               | test_step_finished_fires_assumption_failed_and_test_finished_for_pending_step_in_non_strict_mode   |
    |                         |                               | test_step_finished_fires_assumption_failed_and_test_finished_for_pending_step_in_strict_mode       |
    |                         |                               | test_step_finished_fires_assumption_failed_and_test_finished_for_undefined_step_in_non_strict_mode |
    |                         |                               | test_step_finished_fires_failure_and_test_finished_for_undefined_step_in_strict_mode               |
    |                         |                               | test_step_finished_fires_failure_and_test_finished_for_failed_step                                 |
4a  | TestStepStarted (hook)  | ignored                       |                                                                                                    |
4b  | TestStepFinished (hook) | ignored                       |                                                                                                    |
5   | TestCaseFinished        | notifyResult(stepNotifier)    | test_case_finished_fires_only_test_finished_for_passed_step                                        |
    |                         | runNotifier.fireTestFinished  | test_case_finished_fires_assumption_failed_and_test_finished_for_skipped_step                      |
    |                         |                               | test_case_finished_fires_assumption_failed_and_test_finished_for_pending_step_in_non_strict_mode   |
    |                         |                               | test_case_finished_fires_assumption_failed_and_test_finished_for_pending_step_in_strict_mode       |
    |                         |                               | test_case_finished_fires_assumption_failed_and_test_finished_for_undefined_step_in_non_strict_mode |
    |                         |                               | test_case_finished_fires_failure_and_test_finished_for_undefined_step_in_strict_mode               |
    |                         |                               | test_case_finished_fires_failure_and_test_finished_for_failed_step                                 |

However I don't quite understand why exceptions from the before and after hooks are not used? Should they not fail the test?

mpkorstanje · 2017-06-08T21:26:40Z

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

+
+    public SkippedThrowable() {
+        this(NotificationLevel.STEP);
+    }


This constructor is not used. I guess it should be removed?

mpkorstanje · 2017-06-08T21:26:51Z

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

+
+    public UndefinedThrowable() {
+        this(NotificationLevel.STEP);
+    }


This constructor is not used. I guess it should be removed?

mpkorstanje · 2017-06-08T21:28:59Z

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

+    }
+
+    public SkippedThrowable(NotificationLevel scenarioOrStep) {
+        super(String.format("This %s is skipped", scenarioOrStep.lowerCaseName()), null, false, false);


Which version of java are we using? I'm seeing some java 7 stuff here and there but this is not defined present in the pom. Might be good to put this in there explicitly.

Since v1.1.6 "JDK7 is required to build everything. Built jars should still work on JDK6" (see History.md). I would be a good idea to make it explicit in the pom.

brasmusson · 2017-06-10T12:50:14Z

However I don't quite understand why exceptions from the before and after hooks are not used? Should they not fail the test?

The exceptions from before and after hooks will fail the test, through the result of the Test Case Event, which holds the worst result (created here) from all the steps and hooks (collected by ScenarioImpl)

However, different from today only the worst exception will be notified on the pickleRunnerNotifier - maybe that should be reconsidered.

mpkorstanje · 2017-06-10T14:08:15Z

Yes. I think we should reconsider that. Perhaps by wrapping all errors in something like junits MultipleFailureException construction. However looking at the scale of that change I think that is best done separately.

brasmusson · 2017-06-11T14:50:00Z

Now all step exceptions are collected and at the end of the test case notified on the pickleRunnerNotifier (similarly to using ErrorCollectors in JUnit). The type of notifications are controlled by the result of the test case, so if the first step failed and the second is undefined, both will be notified using fileTestFailure (also in non-strict mode) since the test case as a whole get the status failed. It is not obviously the best solution, but it seems reasonable.

In a way it is consistent to the behaviour of an ErrorCollector when I both had a failing assert and added a AssumptionViolatedException to it - both were notified using fileTestFailure, but it is unlikely that someone have expected using an ErrorCollector like that.

To get a clearer view of the end state of the JUnit module, I'm thinking of merging this after #1145 which also is affecting the notification logic.

Since it is only after a step or scenario has been executed we can tell if it should be considered skipped in JUnit terms, TestAssumptionFailed notifications suits better than TestIgnored notifications. By using TestAssumptionFailed to mark scenarios/steps as skipped, TestStarted can always be fired before the scenario/step is executed, therefore the option --allow-stared-ignored is not longer needed.

Also update History.md.

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

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.

mpkorstanje reviewed Jun 8, 2017

View reviewed changes

This was referenced Jun 10, 2017

AssumptionViolatedException not causing test to be skipped #1082

Closed

JUnit runner does not distinguish between pending and ignored via Assume #1007

Closed

brasmusson force-pushed the junit-assumption-failed-for-skipped branch from f09e109 to e52a9a6 Compare June 11, 2017 14:37

brasmusson force-pushed the junit-assumption-failed-for-skipped branch from e52a9a6 to 8512b10 Compare June 12, 2017 17:01

brasmusson merged commit 8512b10 into master Jun 12, 2017

brasmusson added a commit that referenced this pull request Jun 12, 2017

Merge #1142 '[JUnit] Use AssumptionFailed to mark as skipped'

73bc2da

Also update History.md.

brasmusson deleted the junit-assumption-failed-for-skipped branch June 12, 2017 17:22

mpkorstanje mentioned this pull request Jun 25, 2017

Strange behavior using JUnit options when scenario failed. #1015

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] Use AssumptionFailed to mark scenarios/steps as skipped #1142

[JUnit] Use AssumptionFailed to mark scenarios/steps as skipped #1142

brasmusson commented Jun 6, 2017

mpkorstanje left a comment •

edited

Loading

mpkorstanje Jun 8, 2017

mpkorstanje Jun 8, 2017

mpkorstanje Jun 8, 2017

brasmusson Jun 11, 2017

brasmusson commented Jun 10, 2017

mpkorstanje commented Jun 10, 2017

brasmusson commented Jun 11, 2017

lock bot commented Oct 25, 2018

[JUnit] Use AssumptionFailed to mark scenarios/steps as skipped #1142

[JUnit] Use AssumptionFailed to mark scenarios/steps as skipped #1142

Conversation

brasmusson commented Jun 6, 2017

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mpkorstanje left a comment • edited Loading

Choose a reason for hiding this comment

mpkorstanje Jun 8, 2017

Choose a reason for hiding this comment

mpkorstanje Jun 8, 2017

Choose a reason for hiding this comment

mpkorstanje Jun 8, 2017

Choose a reason for hiding this comment

brasmusson Jun 11, 2017

Choose a reason for hiding this comment

brasmusson commented Jun 10, 2017

mpkorstanje commented Jun 10, 2017

brasmusson commented Jun 11, 2017

lock bot commented Oct 25, 2018

mpkorstanje left a comment •

edited

Loading