[Core] Add Before and AfterStep hooks #1323

coding-yogi · 2018-02-14T15:00:44Z

This PR adds a new feature of AfterStep. Methods annotated with AfterStep will be executed after every step. For HTML Reports to work properly after this change , this PR requires to be merged

Motivation and Context

This change was requested by users of cucumber-jvm

Types of changes

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

Checklist:

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

coveralls · 2018-02-14T15:16:30Z

Coverage increased (+0.3%) to 81.939% when pulling 92e4fa6 on coding-yogi:feature_afterstep into f42079a on cucumber:master.

mpkorstanje · 2018-02-14T20:15:57Z

core/src/main/java/cucumber/runner/Runner.java

@@ -128,6 +128,9 @@ private void addTestStepsForPickleSteps(List<TestStep> testSteps, PickleEvent pi
                match = new FailedStepInstantiationMatch(pickleEvent.uri, step, t);
            }
            testSteps.add(new PickleTestStep(pickleEvent.uri, step, match));
+            if (!runtimeOptions.isDryRun()) {
+                addTestStepsForAfterStepHooks(testSteps, pickleEvent.pickle.getTags());
+            }


Could you provide a test that shows the after hook is executed after each step?

This also affects the JSONFormatter. Currently it assumes all hooks are either before or after hooks. Using after step hooks will have undefined results.

@brasmusson where do you think the TestStepStarted events for step hooks should go in the json? I am thinking they should go into the same list as the the test steps themselves.

@mpkorstanje , I can see afterstep being reported properly in json report. Could you please let me know what exactly is the issue?

I can not. It does indeed all seem to work out.

I will update the class to HookStep instead of Unskipable step and handle the logic of skipping AfterStep for a failed/skipped step. This will be in sync with ruby.

Let me know if that should be ok

I will have to do some changes for JSONFormatter as there is some incosistency when compared with Ruby Json report. Will add a test for same

@mpkorstanje , @brasmusson , I have implemented logic of skipping AfterStep in case of failed or skipped Step and also tests for JSON formatter showing AfterSteps JSON arry node should belong to it's respective step node.

Would you mind reviewing the changes?

I'll look into them, but it will probably take half a week or so before I have the time and energy to do it.

Sure, Thanks

brasmusson

I have (today) initiated a discussion in the #committers channel of the Cucumber Slack community, with respect to the two behavioral options for step hooks in case of failure, that is, the current Cucumber-Ruby behavior:

before hook - executed
step1 - executed and fails
after step hook - skipped
step2 - skipped
after step hook - skipped
after hook - executed

and the alternative:

before hook - executed
step1 - executed and fails
after step hook - skipped
step2 - skipped
after step hook - skipped
after hook - executed

We'll have to see were that discussion land before finish this PR.

brasmusson · 2018-02-25T18:22:58Z

core/src/main/java/cucumber/runner/HookStep.java

        super(definitionMatch);
        this.hookType = hookType;
    }

    protected Result.Type executeStep(String language, Scenario scenario, boolean skipSteps) throws Throwable {
-        definitionMatch.runStep(language, scenario);
-        return Result.Type.PASSED;
+        if(hookType == HookType.After || hookType == HookType.Before) {


The current convention in the code base is to use "if (" rather than "if(" (and "switch (" rather than "switch("), so for the sake of consistency this convention should be followed in this PR too.

Sure, I will fix these throughout PR

brasmusson · 2018-02-25T18:30:35Z

core/src/test/java/cucumber/runtime/RuntimeTest.java

-import cucumber.api.Scenario;
-import cucumber.api.StepDefinitionReporter;
-import cucumber.api.TestCase;
+import cucumber.api.*;


Do not use wildcard imports, see https://github.com/cucumber/cucumber-jvm/blob/master/CONTRIBUTING.md#contributing.

Yes, will fix this one. I think it happened due to Intellij formatting it to .* in case all the classes are being used and imported from package

brasmusson · 2018-02-25T18:36:57Z

core/src/test/java/cucumber/runtime/formatter/HTMLFormatterTest.java

+                "formatter.result({\n" +
+                "  \"status\": \"passed\"\n" +
+                "});\n", "" +
+                "formatter.afterstep({\n" +


As the intention is to move away from the Cucumber-HTML-project (see cucumber-attic/cucumber-html#44), I'm thinking about exploring the use of "formatter.after" also for after step hooks, instead of coupling this PR to a required change in the Cucumber-HTML (I'm not sure we want to open that Box of Pandora at this time).

I didn't exactly get what needs to be done for this, should I leave it as it is for now?

I'll try to take a look at what can best be done with respect to the HTML formatter.

brasmusson · 2018-02-25T18:39:05Z

core/src/test/java/cucumber/runtime/formatter/JSONFormatterTest.java

+            "              \"status\": \"passed\"\n" +
+            "            },\n" +
+            "            \"line\": 4,\n" +
+            "            \"afterstep\": [\n" +


The Json key should be "after" also for after step hooks, so that "before" and "after" are the Json key used for hooks both on the scenario level and the step level (see the Cucumber-Ruby spec https://github.com/cucumber/cucumber-ruby/blob/master/spec/cucumber/formatter/json_spec.rb#L677).

yes, I saw that in ruby but there is a small catch here. In the code every where I have used string representation of HookType enum , for eg mapToAddTo.put(hookType.toString(), new ArrayList<Map<String, Object>>());
So in this case I will have to explicitly handle the AfterStep case to report it as After

brasmusson · 2018-02-25T18:45:14Z

core/src/test/java/cucumber/runtime/HookTest.java

+        InOrder inOrder = inOrder(step1, step2, hook, backend);
+        inOrder.verify(step1).getText();
+        inOrder.verify(step2).getText();
+        inOrder.verify(hook, times(2)).execute(Matchers.<Scenario>any());


This test is confusing, it looks like it verifies that the after step hook is executed twice after step2 has been executed, but the expected behavior is that the hook is executed once in between step1 and step2 and once after step2. Can this test be change to better specify the expected behavior (and not look like it verifies the wrong behavior)?

This test was commented and was supposed to be removed as this behaviour is verified in other test. I will check once again and remove this test

…terStep hook

coding-yogi · 2018-02-28T10:22:34Z

@brasmusson , I have made the requested changes except the HTML formatter one for which I didn't exactly get what needs to be done

brasmusson · 2018-03-04T20:31:17Z

Even thought there has not as wide participation as I would have liked in the discussion in the #committers channel, the conclusion seems to be that it is reasonable to run the after step hook for a (gherkin) step that fails (but not the after step hooks for subsequent steps). Therefore I took a stab at implementing this (f3c908c). The TestCase abstraction is still a (flat) sequence of TestStep, but the skipStep state now has three states (RUN_ALL, RUN_HOOKS, SKIP_ALL_SKIPABLE) instead of two (true, false).

mpkorstanje · 2018-03-05T00:00:38Z

core/src/main/java/cucumber/api/TestCase.java

            Result stepResult = step.run(bus, pickleEvent.pickle.getLanguage(), scenarioResult, skipNextStep);
-            if (!stepResult.is(Result.Type.PASSED)) {
-                skipNextStep = true;
+            if (!stepResult.is(Result.Type.PASSED) && skipNextStep == SkipStatus.RUN_ALL) {


I find it a bit hard to qualify why but the concept underlying this solution feels wrong. TestStep is leaking information into TestCase as to how the test should proceed. It shouldn't have to.

I think this is caused by trying to represent the test execution as a list of steps rather then the tree that it is (it may help to imagine the existence of a before step hook).

I have one suggestion, why can't we have skipStep as binary bit flags, or bit array or a map for eg {"skipBeforeStep": false, "skipGherkingStep": false, "skipAfterStep": false}

when step fails set skipGherkingStep to true but skipAfterStep still remains false and executes the afterStep. After execution of afterStep, if skipGherkingStep is true , set skipAfterStep to true so that none of the next afterStep executes

mpkorstanje · 2018-03-07T00:02:05Z

I've moved the after step hooks into the PickleTestSteps. This reduces the complexity of the skip logic in TestCase significantly.

One thing we should still look at though are the UnsupportedOperationException in HookStep and PickleTestStep though. It smells like a refused bequest.

And perhaps also make sure we don't expose too much of the internal cucumber apis here.

mpkorstanje · 2018-03-07T08:56:50Z

I've extracted a Step and HookStep class from TestStep. Now it appears that Step doesn't contain any methods that are relevant to the event consumers of the TestStepFinishedEvent. They always have to cast to either TestStep or HookStep.

This raises the question, shouldn't hook events be in their own events?

brasmusson · 2018-03-07T20:07:52Z

core/src/main/java/cucumber/api/Result.java

        AMBIGUOUS,
+        UNDEFINED,


Ambiguous and Failed both unconditionally make the exit code non-zero, but Undefined does not, so Undefined must be less severe than Ambiguous.

You're right.

brasmusson · 2018-03-07T20:14:32Z

I've moved the after step hooks into the PickleTestSteps.

I'm not sure that is the right move. I have always envisioned that it is only the result of the test step from the PickleStep itself that are reported in the summary, even if an after step hook fails the summary should report "passed" of the test step from the PickleStep itself passed.

mpkorstanje · 2018-03-07T20:56:47Z

I am not sure I understand. The summary is created from events emitted through the bus not from the StepResult returned by the PickleStep. It should work as you've described.

The StepResult is only used to created the Result of the TestCase which is done by finding the most severe result and its error.

brasmusson · 2018-03-08T20:08:57Z

This raises the question, shouldn't hook events be in their own events?

The drive to unify the event model between the Cucumber-implementations (so that for instance common formatters processing the Json representation of the event stream), mean that we cannot do that in Cucumber-JVM without commitment that the other Cucumber-implementations also do it.

I think we in the community is very much into the view that everything that is executed during the execution of a "Test Case" is a "Test Step". A "Test Step" can be related to 0..1 "Pickle Step" (0 meaning that it is a "Test Step" created from a hook), and it can be related to 0..* code methods/functions (0 meaning that it is a "Test Step" created from a "Pickle Step" that could not be matched to any step definition - undefined, and * meaning that it is a "Test Step" created from a "Pickle Step" that could be matched to several step definitions - ambiguous). The "started" and "finished" event should therefore be of the same type for all "Test Step"s.

brasmusson · 2018-03-08T20:14:02Z

It should work as you've described.

You are right.

I think we can use a hierarchical model for steps in test cases internally in the runner. However, I think that if it is possible to get the steps of a test case through classes in the api packages, then it should be a flat list of test steps, so that the abstraction of a test case as a flat sequence of test steps is maintained there.

mpkorstanje · 2018-03-08T21:36:12Z

However, I think that if it is possible to get the steps of a test case through classes in the api packages, then it should be a flat list of test steps, so that the abstraction of a test case as a flat sequence of test steps is maintained there.

That would be the depth first iteration order over the step tree. Should be easy enough to change TestCase.getTestSTeps() to do that

mpkorstanje · 2018-03-08T21:39:58Z

I think we in the community is very much into the view that everything that is executed during the execution of a "Test Case" is a "Test Step".

We should conform to that then. How should we name set of proper TestSteps and HookSteps? I've currently named them Step. I reckon TestStep would be a better name but that leaves the proper TestSteps without a name.

mpkorstanje · 2018-03-08T22:11:41Z

What is the expected behavior of the TestStepStarted and TestStepFinished events for hooks on a dry run? Currently the AfterStepHook emits and event while the other hooks don't. They're not added to TestCase on a dryrun. Presumably because they're unskipable but hat seems to be rather inconsistent with the idea that they're regular test steps. I would expect them to emit a skipped event.

edit: And also quite easy to fix by making them skipable but only skipping them on a dry run.

To stay consistent with the TestStep, PickleStepTestStep and HookStep from `cucumber.api` we should use the same naming convention in `cucumber.runtime`.

…e_afterstep

coding-yogi · 2018-04-03T06:05:05Z

I have fixed the HTML reports including issue #1303, let me know if I should include those changes in this PR

brasmusson · 2018-04-08T19:06:24Z

@coding-yogi I have looked into the behavior of the HTML formatter of Cucumber-Ruby. Since the intention is to replace the HTML formatters for all Cucumber implementation with one binary that can process the event stream and produce a HTML report, we should keep the different HTML formatters as aligned as reasonable possible until then.

When using afterStep hooks and the HTML formatter in Cucumber-Ruby and all the step hooks pass, then nothing is added to the HTML DOM regarding the afterStep hooks. Therefore I do not think that the js functions for step hooks here should unconditional add anything to the HTML DOM either, that is not doing something like:

  this.afterstep = function(afterStep) {
    currentStep = $('.afterstep', $templates).clone();
    currentStep.appendTo(currentSteps);

The HTML formatter of Cucumber-Ruby will display output (in Cucumber-JVM Scenario.write) from:

the step definition (this is not possible in Cucumber-JVM)
the after step hook
the after hook (in case of the last step)

in exactly the same way (they will all appear below the text of the Gherkin step), therefore I do not think that there is any need to do anything new to handle output and embeddings with the introduction of step hooks (like making a change where handleTestStepStarted would just register what type of StepHook has started), the current behavior is sufficient.

In the end I think that the changes needed in Cucumber-HTML is cucumber-attic/cucumber-html@d757b90 (now when cucumber-attic/cucumber-html#49 has been fixed).

mpkorstanje · 2018-04-08T19:14:13Z

Since the intention is to replace the HTML formatters for all Cucumber implementation with one binary that can process the event stream and produce a HTML report, we should keep the different HTML formatters as aligned as reasonable possible until then.

Is that ideal still alive? From what I can see all the other implementations moved away from it.

brasmusson · 2018-04-08T19:21:18Z

The new initiative https://github.com/cucumber/cucumber-pickle-runner, why not shared binaries for formatters too?

aslakhellesoy · 2018-04-08T20:30:50Z

I think the single binary is a very exciting development, and it makes perfect sense to me to put formatters in there too.

mpkorstanje · 2018-04-08T21:55:57Z

I must be misunderstanding something somewhere. I am I right in assuming the go project is a second attempt at unifying the different formatters? In that case nevermind.

aslakhellesoy · 2018-04-08T23:35:04Z

@mpkorstanje: It’s much more:

cucumber/common#221

https://github.com/cucumber/cucumber-pickle-runner

coding-yogi · 2018-04-10T10:27:00Z

@brasmusson , thanks for the clarification, so I won't push any further changes for HTML formatters

naelabdeljawad · 2018-04-11T11:36:05Z

Hi,
Did you release new version which includes the new hooks (AfterStep/BeforeStep) for java?

aslakhellesoy · 2018-04-11T12:06:18Z

@naelabdeljawad if you're ever in doubt about what's been released, check out the CHANGELOG

aslakhellesoy · 2018-04-11T12:07:33Z

An open pull request (such as this one) means the code has not been merged, and therefore, not released.

mpkorstanje · 2018-04-21T12:39:31Z

Documentation still needs to be updated:

https://docs.cucumber.io/cucumber/#beforestep
https://github.com/cucumber/docs.cucumber.io/blob/master/content/cucumber.md

Now I think my issues have been resolved

aslakhellesoy · 2018-04-26T14:00:47Z

Hi @coding-yogi,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

lock · 2019-04-26T14:10:39Z

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.

aslakhellesoy · 2019-04-26T14:10:43Z

Hi @coding-yogi,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

Aniket added 2 commits February 14, 2018 19:45

Adding AfterStep Feature

783828d

add tests for AfterStep

89a4c90

coding-yogi mentioned this pull request Feb 14, 2018

required changes for Before and After Step Hooks cucumber-attic/cucumber-html#48

Closed

mpkorstanje reviewed Feb 14, 2018

View reviewed changes

Aniket added 3 commits February 15, 2018 15:14

Adding some more tests and refactoring

f870014

fix JSON formatter and add test to validate afterstep

442a036

Skipping AfterStep in case step fails or is skipped

b98656b

brasmusson previously requested changes Feb 25, 2018

View reviewed changes

Aniket added 2 commits February 26, 2018 17:09

Some code refactoring as per review comments

5528374

Updating json formatter to be in sync with ruby implementation for af…

e0f3980

…terStep hook

Run after step hook until the next test step from a gherkin step

f3c908c

mpkorstanje reviewed Mar 5, 2018

View reviewed changes

Represent AfterHookSteps as substeps of a PickleTestStep

3ae1cff

Extract Step and HookSteps from TestStep

1da5039

brasmusson reviewed Mar 7, 2018

View reviewed changes

Make UNDEFINED less severe then AMBIGUOUS

f926057

mpkorstanje added 2 commits March 9, 2018 00:37

Treat before and after hooks like hooks rather then unskipable steps

3be655e

Hide step implementation details from public api

50ef0bf

mpkorstanje added 3 commits March 22, 2018 19:34

Use Step, PickleStep and Hook naming convention

6bce153

To stay consistent with the TestStep, PickleStepTestStep and HookStep from `cucumber.api` we should use the same naming convention in `cucumber.runtime`.

Tighten accessibility

26a30d8

Merge branch 'master' of github.com:cucumber/cucumber-jvm into featur…

373f729

…e_afterstep

mpkorstanje mentioned this pull request Apr 7, 2018

[Core Bug Fix] Fixed runtime exit status for ambiguous scenarios #1342

Merged

6 tasks

Merge branch 'master' into feature_afterstep

e632250

brasmusson mentioned this pull request Apr 14, 2018

Handle before step and after step hooks cucumber-attic/cucumber-html#50

Merged

[pom] Update the version of the cucumber-html dependency

92e4fa6

mpkorstanje approved these changes Apr 21, 2018

View reviewed changes

mpkorstanje mentioned this pull request Apr 24, 2018

adding rerun functionality #1356

Closed

6 tasks

mpkorstanje merged commit fc6dabc into cucumber:master Apr 26, 2018

mpkorstanje added a commit that referenced this pull request Apr 26, 2018

Update CHANGELOG wrt to #1323

8c69975

mlvandijk mentioned this pull request Apr 28, 2018

Update Hooks with BeforeStep and AfterStep for jvm cucumber/docs#161

Closed

foursyth mentioned this pull request Nov 20, 2018

How is the EmbedEvent invoked in the reporters? #1504

Closed

lock bot locked as resolved and limited conversation to collaborators Apr 26, 2019

[Core] Add Before and AfterStep hooks #1323

[Core] Add Before and AfterStep hooks #1323

Conversation

coding-yogi commented Feb 14, 2018 • edited by mpkorstanje Loading

Motivation and Context

Types of changes

Checklist:

coveralls commented Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brasmusson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coding-yogi commented Feb 28, 2018

brasmusson commented Mar 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje commented Mar 7, 2018 • edited Loading

mpkorstanje commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brasmusson commented Mar 7, 2018

mpkorstanje commented Mar 7, 2018

brasmusson commented Mar 8, 2018

brasmusson commented Mar 8, 2018

mpkorstanje commented Mar 8, 2018 • edited Loading

mpkorstanje commented Mar 8, 2018

mpkorstanje commented Mar 8, 2018 • edited Loading

coding-yogi commented Apr 3, 2018

brasmusson commented Apr 8, 2018

mpkorstanje commented Apr 8, 2018

brasmusson commented Apr 8, 2018

aslakhellesoy commented Apr 8, 2018

mpkorstanje commented Apr 8, 2018

aslakhellesoy commented Apr 8, 2018

coding-yogi commented Apr 10, 2018

naelabdeljawad commented Apr 11, 2018

aslakhellesoy commented Apr 11, 2018

aslakhellesoy commented Apr 11, 2018

mpkorstanje commented Apr 21, 2018

aslakhellesoy commented Apr 26, 2018

lock bot commented Apr 26, 2019

aslakhellesoy commented Apr 26, 2019

coding-yogi commented Feb 14, 2018 •

edited by mpkorstanje

Loading

coveralls commented Feb 14, 2018 •

edited

Loading

mpkorstanje commented Mar 7, 2018 •

edited

Loading

mpkorstanje commented Mar 8, 2018 •

edited

Loading

mpkorstanje commented Mar 8, 2018 •

edited

Loading