Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Add Before and AfterStep hooks #1323

Merged
merged 33 commits into from
Apr 26, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
783828d
Adding AfterStep Feature
Feb 14, 2018
89a4c90
add tests for AfterStep
Feb 14, 2018
f870014
Adding some more tests and refactoring
Feb 15, 2018
442a036
fix JSON formatter and add test to validate afterstep
Feb 16, 2018
b98656b
Skipping AfterStep in case step fails or is skipped
Feb 17, 2018
5528374
Some code refactoring as per review comments
Feb 26, 2018
e0f3980
Updating json formatter to be in sync with ruby implementation for af…
Feb 26, 2018
f3c908c
Run after step hook until the next test step from a gherkin step
brasmusson Mar 4, 2018
3ae1cff
Represent AfterHookSteps as substeps of a PickleTestStep
mpkorstanje Mar 6, 2018
1da5039
Extract Step and HookSteps from TestStep
mpkorstanje Mar 7, 2018
f926057
Make UNDEFINED less severe then AMBIGUOUS
mpkorstanje Mar 8, 2018
3be655e
Treat before and after hooks like hooks rather then unskipable steps
mpkorstanje Mar 8, 2018
50ef0bf
Hide step implementation details from public api
mpkorstanje Mar 9, 2018
423b961
Hide cucumber.runtime.arguments from the public api
mpkorstanje Mar 9, 2018
75b59bc
Add before step hook
mpkorstanje Mar 9, 2018
c7b1cfd
Add lambda before and after step hooks
mpkorstanje Mar 9, 2018
44d4f4f
Use concrete types in runner implementation
mpkorstanje Mar 9, 2018
189aea4
Move getDefinitionArgument down to TestStep
mpkorstanje Mar 9, 2018
f6b96da
Update JSONFormatter to include BeforeStep
Mar 12, 2018
5fa4360
Add javadoc to steps and events
mpkorstanje Mar 12, 2018
5742014
Add javadoc to TestStep.get*Argument
mpkorstanje Mar 12, 2018
a2ffa0f
Nested If..else -> switch
Mar 13, 2018
34c11e9
AfterStep should be skipped if BeforeStep fails
Mar 13, 2018
c1ef976
Rename TestStep to PickleTestStep, HookStep to TestStep, Step to Test…
mpkorstanje Mar 14, 2018
e55c769
Rename TestSteps
mpkorstanje Mar 17, 2018
7e3d10d
Always invoke after step hooks when before step hooks have been invoked
mpkorstanje Mar 17, 2018
4cae4bf
Restore test step methods as deprecated methods
mpkorstanje Mar 22, 2018
5266c0e
Remove unused and unsupported methods from internal api
mpkorstanje Mar 22, 2018
6bce153
Use Step, PickleStep and Hook naming convention
mpkorstanje Mar 22, 2018
26a30d8
Tighten accessibility
mpkorstanje Mar 22, 2018
373f729
Merge branch 'master' of github.com:cucumber/cucumber-jvm into featur…
mpkorstanje Apr 1, 2018
e632250
Merge branch 'master' into feature_afterstep
mpkorstanje Apr 8, 2018
92e4fa6
[pom] Update the version of the cucumber-html dependency
brasmusson Apr 15, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/java/cucumber/api/HookType.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package cucumber.api;

public enum HookType {
Before, After;
Before, After, AfterStep;

@Override
public String toString() {
Expand Down
7 changes: 7 additions & 0 deletions core/src/main/java/cucumber/runner/Runner.java
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, Thanks

}
}

Expand All @@ -139,6 +142,10 @@ private void addTestStepsForAfterHooks(List<TestStep> testSteps, List<PickleTag>
addTestStepsForHooks(testSteps, tags, glue.getAfterHooks(), HookType.After);
}

private void addTestStepsForAfterStepHooks(List<TestStep> testSteps, List<PickleTag> tags) {
addTestStepsForHooks(testSteps, tags, glue.getAfterStepHooks(), HookType.AfterStep);
}

private void addTestStepsForHooks(List<TestStep> testSteps, List<PickleTag> tags, List<HookDefinition> hooks, HookType hookType) {
for (HookDefinition hook : hooks) {
if (hook.matches(tags)) {
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/cucumber/runtime/Glue.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,14 @@ public interface Glue {

void addAfterHook(HookDefinition hookDefinition);

void addAfterStepHook(HookDefinition hookDefinition);

List<HookDefinition> getBeforeHooks();

List<HookDefinition> getAfterHooks();

List<HookDefinition> getAfterStepHooks();

StepDefinitionMatch stepDefinitionMatch(String featurePath, PickleStep step);

void reportStepDefinitions(StepDefinitionReporter stepDefinitionReporter);
Expand Down
12 changes: 12 additions & 0 deletions core/src/main/java/cucumber/runtime/RuntimeGlue.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public class RuntimeGlue implements Glue {
final Map<String, StepDefinition> stepDefinitionsByPattern = new TreeMap<String, StepDefinition>();
final List<HookDefinition> beforeHooks = new ArrayList<HookDefinition>();
final List<HookDefinition> afterHooks = new ArrayList<HookDefinition>();
final List<HookDefinition> afterStepHooks = new ArrayList<HookDefinition>();
final Map<String, CacheEntry> matchedStepDefinitionsCache = new HashMap<String, CacheEntry>();
private final LocalizedXStreams localizedXStreams;

Expand Down Expand Up @@ -49,6 +50,12 @@ public void addAfterHook(HookDefinition hookDefinition) {
Collections.sort(afterHooks, new HookComparator(false));
}

@Override
public void addAfterStepHook(HookDefinition hookDefinition) {
afterStepHooks.add(hookDefinition);
Collections.sort(afterStepHooks, new HookComparator(false));
}

@Override
public List<HookDefinition> getBeforeHooks() {
return beforeHooks;
Expand All @@ -59,6 +66,11 @@ public List<HookDefinition> getAfterHooks() {
return afterHooks;
}

@Override
public List<HookDefinition> getAfterStepHooks() {
return afterStepHooks;
}

@Override
public StepDefinitionMatch stepDefinitionMatch(String featurePath, PickleStep step) {
String stepText = step.getText();
Expand Down
19 changes: 19 additions & 0 deletions core/src/test/java/cucumber/runtime/HookOrderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,25 @@ public void after_hooks_execute_in_reverse_order() throws Throwable {
inOrder.verify(hooks.get(0)).execute(Matchers.<Scenario>any());
}

@Test
public void after_step_hooks_execute_in_reverse_order() throws Throwable {
List<HookDefinition> hooks = mockHooks(Integer.MIN_VALUE, 2, Integer.MAX_VALUE, 4, -1, 0, 10000);
for (HookDefinition hook : hooks) {
glue.addAfterStepHook(hook);
}

runner.runPickle(pickleEvent);

InOrder inOrder = inOrder(hooks.toArray());
inOrder.verify(hooks.get(2)).execute(Matchers.<Scenario>any());
inOrder.verify(hooks.get(6)).execute(Matchers.<Scenario>any());
inOrder.verify(hooks.get(3)).execute(Matchers.<Scenario>any());
inOrder.verify(hooks.get(1)).execute(Matchers.<Scenario>any());
inOrder.verify(hooks.get(5)).execute(Matchers.<Scenario>any());
inOrder.verify(hooks.get(4)).execute(Matchers.<Scenario>any());
inOrder.verify(hooks.get(0)).execute(Matchers.<Scenario>any());
}

@Test
public void hooks_order_across_many_backends() throws Throwable {
List<HookDefinition> backend1Hooks = mockHooks(3, Integer.MAX_VALUE, 1);
Expand Down
26 changes: 26 additions & 0 deletions java/src/main/java/cucumber/api/java/AfterStep.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package cucumber.api.java;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface AfterStep {
/**
* @return a tag expression
*/
String[] value() default {};

/**
* @return max amount of milliseconds this is allowed to run for. 0 (default) means no restriction.
*/
long timeout() default 0;

/**
* @return the order in which this hook should run. Higher numbers are run first.
* The default order is 10000.
*/
int order() default 10000;
}
12 changes: 11 additions & 1 deletion java/src/main/java/cucumber/runtime/java/JavaBackend.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static java.lang.Thread.currentThread;

import cucumber.api.java.After;
import cucumber.api.java.AfterStep;
import cucumber.api.java.Before;
import cucumber.api.java.ObjectFactory;
import cucumber.api.java8.GlueBase;
Expand Down Expand Up @@ -161,10 +162,14 @@ void addHook(Annotation annotation, Method method) {
String[] tagExpressions = ((Before) annotation).value();
long timeout = ((Before) annotation).timeout();
addBeforeHookDefinition(new JavaHookDefinition(method, tagExpressions, ((Before) annotation).order(), timeout, objectFactory));
} else {
} else if (annotation.annotationType().equals(After.class)) {
String[] tagExpressions = ((After) annotation).value();
long timeout = ((After) annotation).timeout();
addAfterHookDefinition(new JavaHookDefinition(method, tagExpressions, ((After) annotation).order(), timeout, objectFactory));
} else if (annotation.annotationType().equals(AfterStep.class)) {
String[] tagExpressions = ((AfterStep) annotation).value();
long timeout = ((AfterStep) annotation).timeout();
addAfterStepHookDefinition(new JavaHookDefinition(method, tagExpressions, ((AfterStep) annotation).order(), timeout, objectFactory));
}
}
}
Expand All @@ -179,6 +184,11 @@ public void addAfterHookDefinition(HookDefinition afterHook) {
glue.addAfterHook(afterHook);
}

@Override
public void addAfterStepHookDefinition(HookDefinition afterStepHook) {
glue.addAfterStepHook(afterStepHook);
}

private Pattern pattern(Annotation annotation) throws Throwable {
Method regexpMethod = annotation.getClass().getMethod("value");
String regexpString = (String) Utils.invoke(annotation, regexpMethod, 0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ public interface LambdaGlueRegistry {

void addStepDefinition(StepDefinition stepDefinition);

void addAfterStepHookDefinition(HookDefinition afterStepHook);

void addBeforeHookDefinition(HookDefinition beforeHook);

void addAfterHookDefinition(HookDefinition afterHook);
Expand Down
3 changes: 2 additions & 1 deletion java/src/main/java/cucumber/runtime/java/MethodScanner.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cucumber.runtime.java;

import cucumber.api.java.After;
import cucumber.api.java.AfterStep;
import cucumber.api.java.Before;
import cucumber.runtime.ClassFinder;
import cucumber.runtime.CucumberException;
Expand Down Expand Up @@ -76,7 +77,7 @@ private void validateMethod(Method method, Class<?> glueCodeClass) {

private boolean isHookAnnotation(Annotation annotation) {
Class<? extends Annotation> annotationClass = annotation.annotationType();
return annotationClass.equals(Before.class) || annotationClass.equals(After.class);
return annotationClass.equals(Before.class) || annotationClass.equals(After.class) || annotationClass.equals(AfterStep.class);
}

private boolean isStepdefAnnotation(Annotation annotation) {
Expand Down
10 changes: 10 additions & 0 deletions java/src/test/java/cucumber/runtime/java/JavaBackendTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ public void addBeforeHook(HookDefinition hookDefinition) {
throw new UnsupportedOperationException();
}

@Override
public void addAfterStepHook(HookDefinition hookDefinition) {
throw new UnsupportedOperationException();
}

@Override
public void addAfterHook(HookDefinition hookDefinition) {
throw new UnsupportedOperationException();
Expand All @@ -81,6 +86,11 @@ public List<HookDefinition> getBeforeHooks() {
throw new UnsupportedOperationException();
}

@Override
public List<HookDefinition> getAfterStepHooks() {
throw new UnsupportedOperationException();
}

@Override
public List<HookDefinition> getAfterHooks() {
throw new UnsupportedOperationException();
Expand Down
18 changes: 18 additions & 0 deletions java/src/test/java/cucumber/runtime/java/JavaHookTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import cucumber.api.Scenario;
import cucumber.api.java.After;
import cucumber.api.java.AfterStep;
import cucumber.api.java.Before;
import cucumber.runtime.ClassFinder;
import cucumber.runtime.CucumberException;
Expand Down Expand Up @@ -29,12 +30,14 @@
public class JavaHookTest {
private static final Method BEFORE;
private static final Method AFTER;
private static final Method AFTERSTEP;
private static final Method BAD_AFTER;

static {
try {
BEFORE = HasHooks.class.getMethod("before");
AFTER = HasHooks.class.getMethod("after");
AFTERSTEP = HasHooks.class.getMethod("afterStep");
BAD_AFTER = BadHook.class.getMethod("after", String.class);
} catch (NoSuchMethodException e) {
throw new InternalError("dang");
Expand Down Expand Up @@ -71,6 +74,16 @@ public void before_hooks_get_registered() throws Exception {
assertEquals(BEFORE, hookDef.getMethod());
}

@Test
public void after_step_hooks_get_registered() throws Exception {
objectFactory.setInstance(new HasHooks());
backend.buildWorld();
backend.addHook(AFTERSTEP.getAnnotation(AfterStep.class), AFTERSTEP);
JavaHookDefinition hookDef = (JavaHookDefinition) glue.getAfterStepHooks().get(0);
assertEquals(0, glue.getBeforeHooks().size());
assertEquals(AFTERSTEP, hookDef.getMethod());
}

@Test
public void after_hooks_get_registered() throws Exception {
objectFactory.setInstance(new HasHooks());
Expand Down Expand Up @@ -138,6 +151,11 @@ public void before() {

}

@AfterStep
public void afterStep() {

}

@After(order = 1)
public void after() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ public void addBeforeHookDefinition(HookDefinition beforeHook) {

}

@Override
public void addAfterStepHookDefinition(HookDefinition afterStepHook) {

}

@Override
public void addAfterHookDefinition(HookDefinition afterHook) {

Expand Down