Adding Gherkin-like feature set, see #50 #51

ashleyfrieze · 2016-11-25T21:52:07Z

WIP at the moment - relates to #50

ashleyfrieze · 2016-11-25T21:53:41Z

src/main/java/com/greghaskins/spectrum/Spectrum.java

   */
+  @Deprecated


Could we possibly even remove this type altogether?

Possibly in the future. The public API was originally all under the Spectrum type, basically all users have references to Spectrum.Block. I had to split the interface out to a separate file to resolve some internal circular dependencies. IIRC, I was also considering adding some internal methods to Block that would not be required/exposed on the public API, but didn't follow through with them.

So, I'd keep Spectrum.Block in place for backward/semver compatibility. Spectrum 1.0.0 is out there now, and we shouldn't remove/break any parts of its public API until 2.x. We can deprecate it now, and that would be a semver-minor bump (1.1.x).

Then it stays - it's essentially just a rename of the public Block now. It kinda messes up the rest of the usage of Block in Spectrum, but better that "core" Block be the way forward.

greghaskins · 2016-11-25T22:11:06Z

src/main/java/com/greghaskins/spectrum/Box.java

+/**
+ * For use with nested steps inside a test, this box allows data passing between tests
+ */
+public class Box<T> {


Spectrum used to have a similar structure (Value, see example), but I removed that in favor of let() to match the RSpec API. See #43. We should consider adding that back if you think it would be necessary, or at least use a consistent name/API.

I could rename it Value if you like. I see it used to be hiding inside the Spectrum class and used to have public field access :(

The let() construct is a nice idea, but is intended to stop leaking between specs. In a BDD context, the leaf nodes aren't specs, they're steps, and as such should be able to communicate. Boxed values would like us do that.

Tell me what to change to make you approve (other than the failing javadoc :))

ashleyfrieze · 2016-11-25T22:57:01Z

src/main/java/com/greghaskins/spectrum/BDDSyntax.java

+	 * @param block the contents of the scenario - given/when/then steps
+	 */
+	public static void scenario(final String scenarioName, final Block block) {
+		describeChain("Scenario: " + scenarioName, block);


Now uses a causal link within the steps to stop step running when there's a failure earlier in the list.

ashleyfrieze · 2016-11-25T22:57:36Z

src/main/java/com/greghaskins/spectrum/Block.java

 @FunctionalInterface
-interface Block {
+public interface Block {


Made public as this should be the entry point for users in future.

ashleyfrieze · 2016-11-25T23:00:03Z

src/main/java/com/greghaskins/spectrum/Spectrum.java

+   *    {@link #it(String, com.greghaskins.spectrum.Block) it} that define each expected behavior
+   *
+   */
+  public static void describeChain(final String context, final com.greghaskins.spectrum.Block block) {


This is where the requirements of BDD differ to the describe/it syntax. A BDD test should probably stop when a step fails. This allows anyone to say that their Specs within a suite are causally linked.

ashleyfrieze · 2016-11-25T23:00:45Z

src/main/java/com/greghaskins/spectrum/Suite.java

+  @FunctionalInterface
+  interface ChildRunner {
+    void runChildren(final Suite suite, final RunNotifier notifier);
+  }


This allows the running of children to be a strategy which can vary. A sort of micro subclassing.

coveralls · 2016-11-25T23:22:12Z

Coverage decreased (-3.5%) to 96.538% when pulling 40e3989 on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

ashleyfrieze · 2016-11-25T23:40:28Z

src/main/java/com/greghaskins/spectrum/NotifyingBlock.java

@@ -13,6 +14,8 @@ static NotifyingBlock wrap(final Block block) {
    return (description, notifier) -> {
      try {
        block.run();
+      } catch (final AssumptionViolatedException assumptionViolation) {
+        notifier.fireTestAssumptionFailed(new Failure(description, assumptionViolation));


Technically an assumption failure should be reported differently. The coverage tool made me notice this.

ashleyfrieze · 2016-11-26T00:04:41Z

src/test/java/specs/BddExampleSpecs.java

+      });
+
+      scenarioOutline("rerun template scenario for", Stream.of("A", "B", "C"),
+          letter -> {


Another idea stolen from Cucumber - the scenario outline works on any input object and allows the whole internal definition to be reused. This is parameterised testing done easily!

ashleyfrieze · 2016-11-26T00:12:18Z

src/test/java/specs/BddExampleSpecs.java

+      scenarioOutline("outer parameterised scenario with", Stream.of("A", "B", "C"),
+          letter1 -> {
+	  	scenarioOutline("inner parameterised scenario with", Stream.of("Z", "X", "Y"),
+			  letter2 -> {


Nesting scenario outlines with data passing in seems nice too.

ashleyfrieze · 2016-11-26T00:45:37Z

Odd build problem:

"test -z \"$(git status --porcelain)\""

is failing, but the code MUST all be checked in, or how would it have gotten there! Unless there's something funny in your gradle that's rewriting files.

greghaskins · 2016-11-26T04:00:16Z

Instead of failing on style rules, I try to have the build fix things itself where possible (indentation, etc). This error means you have not checked in the "final" version of those files (unstaged local changes). ./gradlew clean build is the source of truth; you'll be good on CI if you commit after that runs green.

ashleyfrieze · 2016-11-26T09:58:28Z

As I feared, running gradle under Windows has caused the files to have their CRLFs mangled. Yuck.

Any idea how to tell gradle not to be so annoying? Or do I have to do my final commits on a Linux command line...?

ashleyfrieze · 2016-11-26T10:13:08Z

Ahaha! I added a .gitattributes file to control EOL - this means that dumb collaborators like me have less chance of breaking the merges when Spotless rewrites the files.

coveralls · 2016-11-26T16:29:50Z

Coverage decreased (-0.4%) to 99.623% when pulling 900e7b9 on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

ashleyfrieze · 2016-11-26T16:32:01Z

Getting that build to pass was extremely time consuming... We're there now! Yay. Coverage is basically 100%.

Not ready for check-in yet. Any feedback gratefully received. Going to look at more stuff relating to ignoring and tagging, since my aim for this is to be able to substitute this for Cucumber in terms of BDD expressiveness and usage in a complex test suite.

ashleyfrieze · 2016-11-26T16:32:40Z

BTW style rules that fail builds because of periods in JavaDoc... :(

coveralls · 2016-11-26T20:25:21Z

Coverage decreased (-0.7%) to 99.281% when pulling 73184fb on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

ashleyfrieze · 2016-11-26T20:25:26Z

I think there's a risk of dissonance between my just-submitted ignore mechanism vs the xit/xdescribe technique already present. Given the requirement for both backwards compatibility and similarity to other frameworks, I think the x/f class of functions should be a thing in their own right. The more JUnit-like ignore function is itself a warm-up for the tagging capability I'm about to add.

coveralls · 2016-11-26T20:37:25Z

Coverage decreased (-0.4%) to 99.635% when pulling c80bfc1 on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

coveralls · 2016-11-26T20:44:43Z

Coverage decreased (-0.4%) to 99.636% when pulling 09910db on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

coveralls · 2016-11-26T22:25:13Z

Coverage decreased (-0.3%) to 99.698% when pulling f5d2af8 on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

ashleyfrieze · 2016-11-26T22:25:51Z

Tagging functionality now provided. This adds onto the general purpose ignore feature. It allows suites and specs to be given a series of tags and for them to be selected for running based on those tags. Example usage would be to mark a specific suite as WIP and add something to the system properties to either not run it or ONLY run it. This is a bit like the focus feature or the temp ignore feature. It's stronger because it might allow you to configure CI never to run WIP tests, or configure separate categories of tests to run in different builds.

coveralls · 2016-11-26T23:17:38Z

Coverage decreased (-0.3%) to 99.708% when pulling ecff7d2 on ashleyfrieze:af-bdd-1 into 782fd7a on greghaskins:master.

ashleyfrieze · 2016-11-26T23:20:57Z

@greghaskins I'm basically done with the Gherkin/Cucumber type features. I have already prototyped getting JUnit rules into the code, so would like to have a crack at doing that properly. It makes sense for me to branch off from this code to do it, but that's going to be self-defeating if, in code review, you come up with some major changes.

Do you think you'll be able to give this PR a quick review so I can see whether I should delay the next step?

greghaskins · 2016-11-27T01:42:05Z

Sure. I'll check it out this evening.

I haven't seen everything yet, but it does sound like this is quite a lot of functionality for one PR. Tagging & alternate ignore syntax sound like separate features, for example.

PRs also accepted on the build script and CONTRIBUTING.md instructions. I use the included config files with my IDE, and run gradle on the command-line often in my TDD cycle. But I'm just one dev, so there are probably things that work better across people and platforms.

greghaskins · 2016-11-27T06:01:20Z

@ashleyfrieze Really nice work. This is looking like a super easy-to-use way to declare Gherkin-style tests. In my experience, Cucumber-JVM has been a pain because of all the step definitions required behind the scenes. This makes tests readable for non-programmers, but simple to implement/edit/refactor for developers. Win!

Some quick points/questions:

The “BDD” term is ambiguous, since for example both RSpec and Cucumber both claim to be “BDD” tools. I think the “Gherkin” term is pretty well-aligned with the given-when-then style, so we should consider using GherkinSyntax instead of BddSyntax, unless you have an even better name. In my own usage, I tend to call the describe/it style test runners as “spec-style” runners. Perhaps what will evolve later on is a separate SpecSyntax to compliment GherkinSyntax.
Spectrum is ready to start having some better package organization. Maybe the Gherkin-specific syntax features could be moved to a sub-package like com.greghaskins.spectrum.gherkin or even com.greghaskins.spectrum.syntax.gherkin (perhaps looking forward to com.greghaskins.spectrum.syntax.spec for the describe/it API down the line).
This is some major waffling on my part: we call the wrapper class Value in there now (as before), and you originally suggested Box. I’m wondering if a better term is something like Variable because that’s really what’s going on: declaring a mutable variable whose value can be retrieved and changed. Not sure if any of them will make sense to newcomers right away. Opinions?
Let’s get the coverage to 100%; Jacoco is sometimes stupid about that, but having a threshold helps keep us honest. Otherwise we’ll have to keep looking at coverage decreases case-by-case. (That said, I’d like to get rid of the annoying coveralls bot, I think there are better alternatives.) In the real world, this will really only be a pain for utility classes like BddSyntax where we don’t call the constructor, and those are rare. I can live with one ugly test case in exchange for being able to show off the “100% coverage” badge. If you like, you can chalk this one up to me just being too dogmatic about TDD.

Now, a few bigger points.

For starters, let’s focus this PR down on just the Gherkin-style test declarations, so we can get it merged quickly and move on to additional features. Specifically, we should break out separate PRs for:

Tagging. This is bigger than just a Gherkin thing, and warrants a separate discussion. We should consider existing designs from RSpec (https://www.relishapp.com/rspec/rspec-core/v/3-5/docs/command-line/tag-option) and Mocha (https://github.com/mochajs/mocha/wiki/Tagging). I also see tagging as a more generic version of the existing focus/ignore functionality (only run a subset of the specs)—it would be nice if those implementations could align somehow.
Spectrum configuration. @SpectrumOptions is one idea, but again this warrants a larger conversation and consideration of prior art. I know we already touched on this over in Support running tests in a random order? #47.
Parameterized scenarios. I really like parameterized tests, but it feels like a separate feature that would be generally useful even for non-Gherkin tests. It also doesn’t quite work (at least in Eclipse) since it marks tests as started and never marks them as finished. In the mean time without scenario outlines, we can accomplish the same effect with a simple forEach loop that has spec-style or Gherkin-style blocks inside it.

I think the commits you made are pretty atomic, so we should be able to port these features over to separate PRs fairly easily (I can help too). I don’t want the back-and-forth around these related but separate ideas to stand in the way of getting a slick new Gherkin syntax in place.

For the Gherkin implementation itself, we’ve introduced some fairly tight coupling between Suite “children” to achieve the given/when/then workflow (which is required, you can't run then if the given fails). This adds a lot of complexity to the Suite class to track which children are related, etc. I think a cleaner approach would create the appropriate extension points or abstractions so that multiple declaration styles can work independently without stepping on each others’ toes. You’re partly there with the ChildRunner concept, which is essentially the Strategy design pattern. I wonder if we could split out those different run strategies to handle two unique styles and even simplify the core Suite class in the process.

Come to think of it, what about going one step further? Gherkin-style tests really operate in a different way and with different rules than spec-style tests. It might be cleaner if there was a proper object model for Feature, Scenario and so on, rather than trying to shoehorn those concepts into Suite and Spec. I’m not sure, but it could be simpler to create a some new Child implementations that are an alternative to Suite/Spec. That might make it easier to extend and maintain the Gherkin syntax down the road, like validating that all the correct pieces are in place (no missing thens) or adding real semantic meaning to given/when/then beyond just reporting them as separate steps.

I'm interested to hear your take on that.

Wow, lots of words! Overall, this work represents a really solid upgrade for Spectrum. Please let me know how I can help further, including collaborating on the code itself. I also set up a chat room over at https://gitter.im/greghaskins/spectrum if you want an easier way to communicate directly.

coveralls · 2016-11-27T08:31:08Z

Coverage decreased (-0.4%) to 99.623% when pulling a58cb16 on ashleyfrieze:af-bdd-1 into bd63595 on greghaskins:master.

coveralls · 2016-11-27T08:31:08Z

Coverage decreased (-0.4%) to 99.623% when pulling a58cb16 on ashleyfrieze:af-bdd-1 into bd63595 on greghaskins:master.

coveralls · 2016-11-27T08:45:45Z

Coverage remained the same at 100.0% when pulling bed4efa on ashleyfrieze:af-bdd-1 into bd63595 on greghaskins:master.

ashleyfrieze · 2016-11-27T08:52:31Z

@greghaskins - wise feedback. A lot of it. I have responded with a push.

BDD now replaced with Gherkin - I was describing it that way in my attempt at changing the Readme
Syntax package created
Wrapper now called Variable
Coverage = 100%
Tagging/ignoring stored for a future PR - we can discuss the options there
Paramaterized scenarios dropped for now

I would like to leave the ChildRunner and describeChain features alone for now. I had considered subclassing Suite and realised it did everything for step relationship except for its child running strategy. That's why I just turned that into a lambda. It also follows that someone using it could reasonably wish any failure in a describe stopped the suite. So they can do that with describeChain.

I'm really aiming to get to JUnit rules, and we're getting caught up at the starting blocks. I'd like to use Spectrum with this feature set in the very near future.

Let's focus just on this PR - what else should change to get this syntax in and merged? Then we can re-open the subject of ignoring/tagging. Then we can do rules.

greghaskins · 2016-11-27T21:44:11Z

src/main/java/com/greghaskins/spectrum/syntax/Gherkin.java

+import com.greghaskins.spectrum.Block;
+
+import java.util.function.Consumer;
+import java.util.stream.Stream;


Consumer and Stream seem to be unused imports in this file, FWIW.

coveralls · 2016-11-27T22:23:21Z

Coverage remained the same at 100.0% when pulling 8647e11 on ashleyfrieze:af-bdd-1 into bd63595 on greghaskins:master.

…hen/Then/And steps to be used. The Scenarios are treated as a block and failures within the block stop the scenario. The class has been added to allow state to be shared by steps or specs. A few minor Java 8 refactorings included also.

coveralls · 2016-11-27T22:27:16Z

Coverage remained the same at 100.0% when pulling 629c7fb on ashleyfrieze:af-bdd-1 into bd63595 on greghaskins:master.

greghaskins · 2016-11-28T03:21:32Z

Nice work; merging now. I have a couple minor fixups, mostly for consistency in the JavaDocs and README which I'll submit as a follow-up PR.

I realized with our latest change, the public API for the Gherkin syntax will be under the root package, in com.greghaskins.spectrum.GherkinSyntax. If we move it later, that will be a semver-major change. I'm okay with that, but wanted to raise the point.

ashleyfrieze commented Nov 25, 2016

View reviewed changes

greghaskins reviewed Nov 25, 2016

View reviewed changes

greghaskins added the semver-minor label Nov 25, 2016

ashleyfrieze commented Nov 25, 2016

View reviewed changes

ashleyfrieze commented Nov 26, 2016

View reviewed changes

ashleyfrieze force-pushed the af-bdd-1 branch 2 times, most recently from b043a1d to 525fdb6 Compare November 26, 2016 00:41

ashleyfrieze changed the title ~~Added BDD basics to Spectrum~~ Adding Gherkin-like feature set, see #50 Nov 26, 2016

ashleyfrieze mentioned this pull request Nov 27, 2016

Support running tests in a random order? #47

Open

ashleyfrieze force-pushed the af-bdd-1 branch 3 times, most recently from 392d746 to a58cb16 Compare November 27, 2016 08:29

greghaskins reviewed Nov 27, 2016

View reviewed changes

ashleyfrieze force-pushed the af-bdd-1 branch from 8647e11 to 629c7fb Compare November 27, 2016 22:25

greghaskins merged commit 0064181 into greghaskins:master Nov 28, 2016

ashleyfrieze mentioned this pull request Nov 28, 2016

Support a Given/When/Then declaration syntax #50

Closed

Adding Gherkin-like feature set, see #50 #51

Adding Gherkin-like feature set, see #50 #51

Conversation

ashleyfrieze commented Nov 25, 2016

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

coveralls commented Nov 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleyfrieze commented Nov 26, 2016

greghaskins commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016

coveralls commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016 • edited Loading

coveralls commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016

coveralls commented Nov 26, 2016

coveralls commented Nov 26, 2016

coveralls commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016

coveralls commented Nov 26, 2016

ashleyfrieze commented Nov 26, 2016

greghaskins commented Nov 27, 2016

greghaskins commented Nov 27, 2016

coveralls commented Nov 27, 2016

coveralls commented Nov 27, 2016

coveralls commented Nov 27, 2016

ashleyfrieze commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 27, 2016

coveralls commented Nov 27, 2016

greghaskins commented Nov 28, 2016

ashleyfrieze commented Nov 26, 2016 •

edited

Loading