JUnit rules - attempt to implement them in Spectrum #56

ashleyfrieze · 2016-11-29T01:58:46Z

This appears to work for Mockito and TemporaryFolder as evidenced. It should work for Class rules too and needs roadtesting with Spring.

It should ultimately solve #15 #49 and #34

coveralls · 2016-11-29T02:06:10Z

Coverage decreased (-1.2%) to 98.777% when pulling 22b8836 on ashleyfrieze:junit-rules into 01e0062 on greghaskins:master.

ashleyfrieze · 2016-11-29T02:09:41Z

I'll tackle the code coverage after initial feedback on how this goes. I think this solution is easily extended to a more JUnit native look too - for situations where you don't need a fresh instance of the test class, i think you could just do

applyRules(this)

or maybe have Spectrum do this for you as the first step, since if "this" doesn't have rules, nothing will be applied.

ashleyfrieze · 2016-11-29T02:25:25Z

So I got the coverage back to 100%... this PR needs more thought and more testing. Note: the mixin classes must be public or JUnit won't play with them.

Note: I stole a lot of the implementation from JUnit BlockJUnitRunner. Knowing where to put it is the hard bit :)

coveralls · 2016-11-29T02:26:17Z

Coverage decreased (-0.3%) to 99.695% when pulling 48369d9 on ashleyfrieze:junit-rules into 01e0062 on greghaskins:master.

coveralls · 2016-11-29T02:30:08Z

Coverage remained the same at 100.0% when pulling ea3148c on ashleyfrieze:junit-rules into 01e0062 on greghaskins:master.

coveralls · 2016-11-29T09:47:24Z

Coverage remained the same at 100.0% when pulling 00ba83b on ashleyfrieze:junit-rules into d6d5c6e on greghaskins:master.

coveralls · 2016-11-30T01:02:46Z

Coverage remained the same at 100.0% when pulling eb7f275 on ashleyfrieze:junit-rules into d6d5c6e on greghaskins:master.

coveralls · 2016-11-30T01:04:29Z

Coverage remained the same at 100.0% when pulling 7348f48 on ashleyfrieze:junit-rules into cdba0a2 on greghaskins:master.

ashleyfrieze · 2016-11-30T02:25:00Z

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

+ * Subclass of {@link Suite} that represent the fact that some tests are composed
+ * of interrelated steps which add up to a single test.
+ */
+class CompositeTest extends Suite implements Atomic {


Extracted to class as Suite was getting big!

ashleyfrieze · 2016-11-30T02:25:32Z

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

@@ -16,14 +18,19 @@ public void run() throws Throwable {
    try {
      final Constructor<?> constructor = this.klass.getDeclaredConstructor();
      constructor.setAccessible(true);
-      constructor.newInstance();
+      testObject = constructor.newInstance();


We'll need this soon... I'm planning a cheat version of the rule stuff where you can use the test class the old way if you like and it just kinda works...

Yeah - I made that. It would be terrible in a parallel running scenario... potentially!

coveralls · 2016-11-30T02:25:36Z

Coverage remained the same at 100.0% when pulling 437e68b on ashleyfrieze:junit-rules into cdba0a2 on greghaskins:master.

ashleyfrieze · 2016-11-30T02:26:49Z

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

+  }
+
+  static <T> RuleContextScope<T> ofNonRecursive(final RuleContext<T> context) {
+    return new RuleContextScope<>(context, -2);


yeah.. -2 - that means it's "my generation" - this needs woyk!

ashleyfrieze · 2016-11-30T02:27:15Z

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

@@ -3,7 +3,7 @@
 import org.junit.runner.Description;
 import org.junit.runner.notification.RunNotifier;

-final class Spec implements Child {
+final class Spec implements Child, Atomic {


A fine Blondie album

ashleyfrieze · 2016-11-30T02:27:30Z

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

@@ -55,7 +55,7 @@
   *
   */
  static void compositeSpec(final String context, final com.greghaskins.spectrum.Block block) {
-    final Suite suite = getCurrentSuiteBeingDeclared().addAbortingSuite(context);
+    final Suite suite = getCurrentSuiteBeingDeclared().addCompositeTest(context);


clearer neater name

ashleyfrieze · 2016-11-30T02:28:28Z

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

    suite.beforeAll.addBlock(this.beforeAll);
    suite.beforeEach.addBlock(this.beforeEach);
    suite.afterEach.addBlock(this.afterEach);
+    rulesToApply.stream()
+        .map(RuleContextScope::nextGeneration)
+        .filter(scope -> scope != null)


Optional won't work here - something to do with type erasure, I think.

ashleyfrieze · 2016-11-30T02:29:12Z

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

+              .map(RuleContextScope::get)
+              .collect(Collectors.toList()),
+              child, () -> runChildWithRules(child, notifier), getDescription()))
+          .run(getDescription(), notifier);


This duplication needs removing before completion.

ashleyfrieze · 2016-11-30T02:29:35Z

src/main/java/com/greghaskins/spectrum/internal/junit/StubJUnitFrameworkMethod.java

+ */
+public interface StubJUnitFrameworkMethod {
+  class Stub {
+    public void method() {}


This is the so-called test method JUnit thinks is under test!

ashleyfrieze · 2016-11-30T02:32:09Z

Still to do:

Reconsider how the rule generations stuff works
Make it possible for the test class/test object to run rules anyway!
Make it possible to run rules against a class, but specify which annotated test method within it should be the rules' target - to allow for

@DirtiesContext
@Test
public void testProcessWhichDirtiesAppCtx() {
    // some logic that results in the Spring container being dirtied
}

ashleyfrieze · 2016-11-30T02:52:38Z

The class blocks of the unique rule class blocks should all be run once before the first actual test.

coveralls · 2016-11-30T12:31:07Z

Coverage remained the same at 100.0% when pulling 4a75b3e on ashleyfrieze:junit-rules into 8cb1f5d on greghaskins:master.

ashleyfrieze · 2016-12-02T00:18:37Z

Class rules now run along with @BeforeClass & @AfterClass
The test object - i.e. the object that Spectrum created - can now host rules and things that rules govern.

coveralls · 2016-12-02T00:20:18Z

Coverage remained the same at 100.0% when pulling 362173f on ashleyfrieze:junit-rules into 8cb1f5d on greghaskins:master.

coveralls · 2016-12-02T00:52:02Z

Coverage remained the same at 100.0% when pulling aa3d3b5 on ashleyfrieze:junit-rules into 8cb1f5d on greghaskins:master.

coveralls · 2016-12-02T00:58:24Z

Coverage remained the same at 100.0% when pulling 57a79aa on ashleyfrieze:junit-rules into 8cb1f5d on greghaskins:master.

coveralls · 2016-12-12T12:50:12Z

Coverage remained the same at 100.0% when pulling f3c5a3d on ashleyfrieze:junit-rules into 20827ba on greghaskins:master.

coveralls · 2016-12-12T20:35:16Z

Coverage remained the same at 100.0% when pulling 9ed966f on ashleyfrieze:junit-rules into 20827ba on greghaskins:master.

ashleyfrieze · 2016-12-12T21:01:28Z

Type safety - resolved
All/Here/Each/Every... it's not great is it!

The junit stuff wants to work in a kind of "for each atomic" way. This means that, recursively down the tree, it will apply to its and things like them. However, there's another approach. It MIGHT be better to apply the rules at the level of a particular suite. Within that suite, the rules run one for each immediate child, but don't propagate down to each child of each child.

The automagic JUnit stuff, to work the way JUnit users might expect, should be each atomic as should the default applyRules or whatever it's called. However, consider a situation where the rules set up and tear down a large eco system. We want to do that as few times as possible, and only repeat it if perhaps it's become invalidated. With this in mind you'd declare it at the suite level, and then have subsuites to reuse it.

If I'm not mistaken, let works at the suite level. Children of the children of the suite get the same object within let.

I think the name applyRules is not helping. I'd love to find a fitting alternative.

Supplier<Mixin> = ...

junitRulesAroundEach(Mixin.class) / junitRulesAroundAll(Mixin.class)
junitRules(Mixin.class).forEachSpec() / junitRules(Mixin.class).forEachImmediateChild()
applyRulesRecursively(Mixin.class) / applyRulesLocally(Mixin.class)
junitRules(Mixin.class) & junitRules(Mixin.class, RECURSIVE) / junitRules(Mixin.class, IMMEDIATE_CHILDREN)
rulesMixin(Mixin.class) / localRulesMixin(Mixin.class)

I had a debate with myself about magic mode. I kind of expect it might end up being easier to agree on SpectrumWithRules but I'd like to make a case for signposting the JavaDoc and living with it being on by default. Opting out is very easy - namely just don't put any @Rule variants in your test class...

Here's what specifically went wrong for the DropWizard rules. I've never seen this happen before by the way, so I reckon this is a weak use case - i.e. <1%.

The DropWizard rule requires you to initialise the instance rule variable using the static rule variable. JUnit can guarantee that the instance variable won't be instantiated before the @ClassRule is run, because JUnit doesn't create any instance of the test object until a test starts. Spectrum MUST create an instance of the test object and only does that once.

This means there are two easily worked around limitations of Spectrum automagic;

You can't instantiate an instance rule object from the static one
You can't use rules which assume that there's a fresh instance of the test object every time

In both cases, you'd try, fail and drop to applyRules or whichever is the closest.

In 95%+ of cases, you can probably just use JUnit rules out of the box as JUnit was intended. It's a real shame to force people to type extra and people assume you can just use the rules.

While I appreciate the point about BlockJUnitRunner and Spectrum being different, the ONLY sticking point is actually the new object instance thing, and that's not actually such a big deal in practice.

Moving the interface - will do - let's just agree the names of things. Perhaps there's a JUnit class/interface in the main package we can put the rules functions into.
Use of experimental - we could do that.... I'm inclined to go for broke and just put this in the main, but why don't you reorganise it to suit yourself after the PR?

coveralls · 2016-12-12T21:10:45Z

Coverage remained the same at 100.0% when pulling 9ed966f on ashleyfrieze:junit-rules into 20827ba on greghaskins:master.

ashleyfrieze · 2016-12-16T10:59:49Z

A thought that's been on my mind is the way that EACH_CHILD and ATOMIC propagation of hooks works, and thus how the two junit rules variants work.

It's the difference between shallow and deep propagation. As far as I understand it, let uses shallow propagation - the children of children of where a let is created get the same object.

Conversely, beforeEach uses deep propagation. Each child of a child of a child gets beforeEach called.

Now, it's possible that you think let should also be deep.

…present, and supporting mix-in objects applied in the hierarchy as a special type of supplier. Updated the Hooks mechanism from greghaskins#77 to allow the passing of RunNotifier and Description around, and hooked in a private version of the rules handling code from JUnit which adapts from Hooks to Statement objects. Demonstrated the whole thing with Spring, Mockito and JUnit native rules, and managed to reverse some of the changes to exception handling brought by greghaskins#77

coveralls · 2017-01-16T23:29:25Z

Coverage remained the same at 100.0% when pulling 00ed8a6 on ashleyfrieze:junit-rules into 659a38c on greghaskins:master.

greghaskins · 2017-01-22T04:35:52Z

Looking at this again, I'm actually willing to merge it mostly unchanged.

We definitely still have some confusion about the difference between applyRules and applyRulesHere. To me, the meaning behind applyRules is clear, but becomes muddy when compared against the applyRulesHere variant. JUnit's class rules are meant to be re-used between atomic tests, while method rules are meant to be fresh for each atomic test. With applyRulesHere we get a kind of middle ground where method rules actually do get re-used between some tests, but not all tests. I may have even gotten that wrong; and the difficulty explaining what it does is leading to difficulty naming it.

After looking at it for a while, I think I do understand the difference in propagation you're describing. The trouble, however, is the rules already describe how propagation should work (class vs method), and we're layering another system on top of that.

I propose just removing applyRulesHere. There may be scenarios where you have deep nesting and want to get fancy with what Spring (or whatever) re-initializes, but I don't think that's a particularly common case. Further, that would be something well beyond what "normal" JUnit could even do, which would be cool but not a deal breaker if we don't have it.

If we take that out, I'm comfortable merging the rest as-is.

After this I do plan to re-organize some packages and such. There's probably a home for all the global JUnit functions in a common place, based on where that shakes out. I'll probably also touch up the documentation and maybe tweak a few tests to add clarity.

There will of course be more significant changes as I push to decouple from JUnit (longer term), but I can't say exactly what that would look like yet. I'm thinking either 2.0 or 3.0 will make a significant break from the current packaging and API.

ashleyfrieze · 2017-01-22T15:00:19Z

@greghaskins - the more I think about my reasoning for applyRulesHere the more I think it's based on instinct, not specific use case. I'll pull it out of the code. If we can't find a need for it, then it's just confusing. Can we rename applyRules? junitMixin instead - or mixInJunit - since that's really what we're doing.

If you have some minor clean-up glitches on your mind, I'm happy to tackle them a bit too. I appreciate you will want to do some general post-merge curation of your own. In my team, we use Pull Requests for Code Review and people give line-by-line feedback where it's needed, so I'm happy to receive per-line _could you play with that a bit_s if you want me to do the leg-work.

I'll ping this thread when the applyRulesHere is vanquished.

I'd really like to keep the JUnit automagic that's also in this MR, and I notice you're not asking me to remove it. From the current docs, is there anything you'd like me to add regarding the caveats. I'm thinking that writing a separate article - linked from the README.md might might sense in discussing the pros and cons, similarities and differences, of each variant of the JUnit rules implementation. What do you think?

greghaskins · 2017-01-22T15:36:54Z

I like the sound of junitMixin, let's go with that.

There were only a couple small things I saw, I can add those as line comments here in the PR. None of them were deal-breakers, IMHO. More along the lines of "maybe play with this a bit" as you said.

I'm game to keep the automagic in and on by default. I think as we split out the JUnit specific pieces, that will make obvious sense for anyone using the JUnit runner (which is today the only runner).

Splitting out the details about how this works definitely seems like a good move. That would be a great first step toward the broader goal of better, proper docs instead of just a giant README. I might flip the order of presentation - to have people try automagic first, and falling back to junitMixin if that doesn't work. No need to bombard them with complexity for the 95% case where it will probably just work out of the box.

greghaskins · 2017-01-22T15:44:14Z

src/main/java/com/greghaskins/spectrum/model/Hooks.java

 import java.util.function.Predicate;
-import java.util.stream.Collectors;

 /**
 * Collection of hooks. It is a linked list, but provides some helpers for


I get a compiler warning about serialVerisionUID on the Hooks class:

The serializable class Hooks does not declare a static final serialVersionUID field of type long Hooks.java /spectrum/src/main/java/com/greghaskins/spectrum/model line 20

Ok. I've put one in. I don't see that warning, but it's no big deal.

greghaskins · 2017-01-22T15:45:56Z

src/main/java/com/greghaskins/spectrum/internal/junit/Rules.java

+   * @param object the JUnit test object - this will never be recreated
+   */
+  static void applyRules(Object object) {
+    RuleContext context = new RuleContext<>(object);


I get a compiler warning about raw types here:

RuleContext is a raw type. References to generic type RuleContext should be parameterized Rules.java /spectrum/src/main/java/com/greghaskins/spectrum/internal/junit line 33

Maybe add @SuppressWarnings("rawtypes") to the local variable?

I went further and just propagated generic types down into the code - it turns out that the type things happen to be can be captured, making this notionally type safe.

I say notionally, since it kind of was, and it kind of doesn't matter, and it's kind of all just Object :)

greghaskins · 2017-01-22T15:51:52Z

src/test/java/specs/JUnitRuleExample.java

+
+  {
+    final Set<File> filesSeen = new HashSet<>();
+    describe("A parent spec", () -> {


This suite was a little hard to follow the first couple times through reading it. Maybe the assertions could be more direct, something like assertTrue(theTempFolder.exists()) and/or assertThat(filesSeen, not(hasItem(theTempFolder))).

With less to test, this was a bit clearer. However, I've done some renames of everything and I think it's probably better...

greghaskins · 2017-01-22T15:55:10Z

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

+   * @param <T> type of the object
+   * @return a supplier of the rules object
+   */
+  public static <T> Supplier<T> applyRules(final Class<T> rulesClass) {


As discussed, we'll rename this one for better clarity. I'm still not sure where the best place for this entry point is, though. Not the worst if it's in root Spectrum, but as this is something I'll be marking as both experimental and JUnit-specific, it might make sense just letting people use Rules.applyRules directly, or maybe something like JUnit.applyRules. I'm flexible though.

Renamed junitMixin - it shall stay here because the junit package is presently internal and should stay that way. I think Spectrum is quite a good Facade, but I appreciate you'll want to restructure and move packages about, so you'll have more freedom if I leave this alone.

ashleyfrieze · 2017-01-22T22:26:31Z

I've addressed the comments. Please read my comments and please re-review.

…ation. Renamed the mixin method. Simplfied some of the code and reduced compiler warnings.

coveralls · 2017-01-22T22:31:31Z

Coverage decreased (-0.2%) to 99.826% when pulling 0ddfa2e on ashleyfrieze:junit-rules into 659a38c on greghaskins:master.

coveralls · 2017-01-22T22:39:55Z

Coverage remained the same at 100.0% when pulling 0bee7d5 on ashleyfrieze:junit-rules into 659a38c on greghaskins:master.

coveralls · 2017-01-23T02:14:56Z

Coverage remained the same at 100.0% when pulling 2361f5b on ashleyfrieze:junit-rules into 659a38c on greghaskins:master.

coveralls · 2017-01-23T20:37:42Z

Coverage remained the same at 100.0% when pulling 9d700da on ashleyfrieze:junit-rules into 457bafc on greghaskins:master.

ashleyfrieze force-pushed the junit-rules branch from ea3148c to 00ba83b Compare November 29, 2016 09:45

ashleyfrieze force-pushed the junit-rules branch from eb7f275 to 7348f48 Compare November 30, 2016 01:02

ashleyfrieze commented Nov 30, 2016

View reviewed changes

greghaskins mentioned this pull request Nov 30, 2016

Added parameterized specs for Gherkin syntax (Scenario Outline) #58

Merged

ashleyfrieze force-pushed the junit-rules branch from 437e68b to 4a75b3e Compare November 30, 2016 12:28

ashleyfrieze mentioned this pull request Dec 2, 2016

@Rule and @ClassRule support #49

Closed

ashleyfrieze force-pushed the junit-rules branch from 57a79aa to 6c17f55 Compare December 2, 2016 11:46

ashleyfrieze closed this Dec 12, 2016

ashleyfrieze deleted the junit-rules branch December 12, 2016 21:05

ashleyfrieze restored the junit-rules branch December 12, 2016 21:07

ashleyfrieze reopened this Dec 12, 2016

ashleyfrieze force-pushed the junit-rules branch from 9ed966f to 00ed8a6 Compare January 16, 2017 23:26

greghaskins reviewed Jan 22, 2017

View reviewed changes

Implemented changes suggested by code review comments. Added document…

0ddfa2e

…ation. Renamed the mixin method. Simplfied some of the code and reduced compiler warnings.

Removed superfluous code.

0bee7d5

made test explain itself better

2361f5b

greghaskins mentioned this pull request Jan 23, 2017

features listed on README not released yet under tag v1.0.0 #84

Closed

Merge branch 'master' into junit-rules

9d700da

greghaskins merged commit 173713f into greghaskins:master Jan 23, 2017

JUnit rules - attempt to implement them in Spectrum #56

JUnit rules - attempt to implement them in Spectrum #56

Conversation

ashleyfrieze commented Nov 29, 2016

coveralls commented Nov 29, 2016

ashleyfrieze commented Nov 29, 2016

ashleyfrieze commented Nov 29, 2016

coveralls commented Nov 29, 2016

coveralls commented Nov 29, 2016

coveralls commented Nov 29, 2016

coveralls commented Nov 30, 2016

coveralls commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Nov 30, 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

ashleyfrieze commented Nov 30, 2016

ashleyfrieze commented Nov 30, 2016

coveralls commented Nov 30, 2016

ashleyfrieze commented Dec 2, 2016

coveralls commented Dec 2, 2016

coveralls commented Dec 2, 2016

coveralls commented Dec 2, 2016

coveralls commented Dec 12, 2016

coveralls commented Dec 12, 2016

ashleyfrieze commented Dec 12, 2016

coveralls commented Dec 12, 2016

ashleyfrieze commented Dec 16, 2016

coveralls commented Jan 16, 2017

greghaskins commented Jan 22, 2017

ashleyfrieze commented Jan 22, 2017

greghaskins commented Jan 22, 2017

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

ashleyfrieze commented Jan 22, 2017 • edited Loading

coveralls commented Jan 22, 2017

coveralls commented Jan 22, 2017

coveralls commented Jan 23, 2017

coveralls commented Jan 23, 2017

ashleyfrieze commented Jan 22, 2017 •

edited

Loading