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

Consider patch release branch: Gherkin glitches #127

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ashleyfrieze
Copy link
Contributor

Fixes for inconsistencies in hook and configuration behaviour with Gherkin Syntax

final Suite suite = DeclarationState.instance().getCurrentSuiteBeingDeclared()
.addCompositeSuite("Scenario: " + scenarioName);
DeclarationState.instance().beginDeclaration(suite, block);
addComposite("Scenario: " + scenarioName, block);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracted out to a helper as needed below.

@@ -155,14 +152,14 @@ static void and(final String behavior, final Block block) {
describe("Scenario outline: " + name, () -> {
describe("Examples:", () -> {
examples.rows().forEach(example -> {
describe(example.toString(), () -> example.runDeclaration(block));
addComposite(example.toString(), () -> example.runDeclaration(block));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

describe did not create a composite test as scenario did

.addSuite(context);
suite.applyPreconditions(block);
DeclarationState.instance().beginDeclaration(suite, block);
addSuiteInternal(parent -> parent.addSuite(context), block);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There some stuff about creating a suite that was common to more than one implementation - extracted out to a common helper.

.getCurrentSuiteBeingDeclared());
suite.applyPreconditions(block);
DeclarationState.instance().beginDeclaration(suite, block);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The applyPreconditions bit wasn't being applied to gherkin. Similarly, the boilerplate around getCurrentSuite... and beginDeclaration... felt like it should be written once and specialised every time it was used.

@@ -126,7 +126,8 @@ public void addHook(final HookContext hook) {
}

private Hooks getHooksFor(final Child child) {
Hooks allHooks = this.parent.getInheritableHooks().plus(this.hooks);
Hooks allHooks = isAtomic() ? new Hooks() : this.parent.getInheritableHooks();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the thing which stops an atomic parent conveying its own inherited hooks down to a child.

});

then("the data is still there", () -> {
assertThat(list.get().get(0), is("hello"));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the fix, this was failing as the let was being freshened for both given and then

});
then("the block configuration will have applied", () -> {
assertThat(result.get().getRunCount(), is(1));
assertThat(result.get().getIgnoreCount(), is(1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the fixes, the with(focus()... didn't work for gherkin

});
then("both are found in the set", () -> {
assertTrue(set.get().contains(first));
assertTrue(set.get().contains(second));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even after one of the fixes, this wasn't going to work because the scenarioOutline wasn't generating actual scenarios...

@greghaskins
Copy link
Owner

@ashleyfrieze I will merge this, but there's a small conflict with master now. Sorry I let this PR sit out here way too long!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants