Grand unified theory of hooks #77

ashleyfrieze · 2016-12-04T00:14:20Z

Still WIP at the moment - demonstrated purely by a replacement of the let function.

I've reorganised the package structure, and I'm in the process of killing off all arounds/befores etc. First target - the humble let.

Code coverage will not be 100% yet as there are unused features.

Intended as a fix for #76 and #73. Eventually. Intended as a new platform on which to build #56

coveralls · 2016-12-04T00:16:38Z

Coverage decreased (-1.03%) to 98.971% when pulling d947193 on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

coveralls · 2016-12-04T02:43:45Z

Coverage decreased (-4.5%) to 95.503% when pulling 105a04b on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

ashleyfrieze · 2016-12-04T02:45:21Z

Highlights:

Large package structure change of the internal classes. There's now an internal and a model package into which detail has been stuck to hide
All the per-use-case implementation of befores, around, afters etc has been turned into a general purpose hook mechanism.
- to get the tests to pass, though, this mechanism has been twisted a bit
- hooks are now classified by where they run, rather than what they are
- helpers for before/after and around are there
- the way is paved for hooks that are suppliers, for which 'let' is an example
- JUnit rules would be an example and could now be implemented independently of Spectrum as just another plugin hook
The nature of notifyingblocks and other general purpose error handling has modified the error messages slightly
The way that hooks can stop execution means that some befores and afters don't happen the way they used to - what they do seems logical enough to me, though.

Todo - test coverage - some unused code no doubt - implementation not 100% complete.
Bonus feature - there's a thread-safe Singleton and this is used for let. What this means is that the following construct isn't a million miles away:

Supplier<Widget> widget = let(() -> WidgetFactory.make());
describe("Some specs to run in parallel", with(parallelRunner(30), () -> {
    it("will be run on one thread", () -> {
        widget.get().doSomething(); // uses one widget even though apparently sharing `widget`
    });
    it("will be run on another thread", () -> {
        widget.get().doSomething(); // uses another widget even though apparently sharing `widget` in parallel
    });
}));

ashleyfrieze · 2016-12-04T02:46:09Z

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

+ * This is not the only way to achieve that - you can also build from {@link SupplyingHook}
+ * but this captures the template for a complex hook.
+ */
+public class AbstractSupplyingHook<T> extends Singleton<T> implements SupplyingHook<T> {


This would do both let and junitTestClass

ashleyfrieze · 2016-12-04T02:46:26Z

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

+
+  @Override
+  public T get() {
+    assertSpectrumInTestMode();


Borrowed from let not a bad thing to have around.

And by borrowed, I mean stolen, given that let is now a hook!

ashleyfrieze · 2016-12-04T02:46:57Z

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.
+ */
+final class CompositeTest extends Suite implements Atomic {


Extracted from #56 - this is necessary for the Atomic to help us correctly.

ashleyfrieze · 2016-12-04T02:47:22Z

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

+/**
+ * Allows the injection of suite configuration during test definition. A wrapper for the
+ * suite which exposes configurables.
+ */


Checkstyle forced me to write Javadoc as items stopped being internal when they switched packages.

ashleyfrieze · 2016-12-04T02:48:18Z

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

+ * {@link SupplyingHook} or subclass {@link AbstractSupplyingHook}.
+ */
+public interface Hook extends ThrowingConsumer<Block> {
+}


I really want to modify aroundXXX to use this rather than ThrowingConsumer - we probably could, but for now I've left an adapter in place.

We haven't done a release with ThrowingConsumer, so I wouldn't be too worried about protecting that API.

Throwing Consumer can die then.

ThrowingSupplier on the other hand, was the item in question and I've removed it.. then restored it.

ashleyfrieze · 2016-12-04T02:48:41Z

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

@@ -1,26 +0,0 @@
-package com.greghaskins.spectrum;


not dead -> moved.

ashleyfrieze · 2016-12-04T02:50:06Z

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

@@ -303,7 +296,7 @@ default T get() {
   * @param consumer to run each spec block
   */
  public static void aroundEach(ThrowingConsumer<com.greghaskins.spectrum.Block> consumer) {
-    getCurrentSuiteBeingDeclared().aroundEach(consumer);
+    getCurrentSuiteBeingDeclared().aroundEach(consumer::acceptOrThrow);


ashleyfrieze · 2016-12-04T02:50:20Z

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

-        throw new RuntimeException(checked);
-      }
-    }
+  public interface ThrowingSupplier<T> extends com.greghaskins.spectrum.ThrowingSupplier<T> {


Kept for posterity...

ashleyfrieze · 2016-12-04T02:50:38Z

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

+  // the hooks - they will be turned into a chain of responsibility
+  // so the first one will be executed last as the chain is built up
+  // from first to last.
+  private Hooks hooks = new Hooks();


So much smaller.

ashleyfrieze · 2016-12-04T02:51:20Z

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

-    suite.beforeAll.addBlock(this.beforeAll);
-    suite.beforeEach.addBlock(this.beforeEach);
-    suite.afterEach.addBlock(this.afterEach);
-    suite.aroundEach(this.aroundEach);


All this stuff now works with the hooks. The cascade from outer to inner is also rather nice - stopping us having to push it down at definition time.

ashleyfrieze · 2016-12-04T02:52:45Z

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

-        }
-
-      }).run(description, notifier);
-    };


Lots of code goes bye bye...

ashleyfrieze · 2016-12-04T02:55:19Z

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

  }

  private void runChildren(final RunNotifier notifier) {
    this.childRunner.runChildren(this, notifier);
  }

-  private void runChild(final Child child, final RunNotifier notifier) {
+  protected void runChild(final Child child, final RunNotifier notifier) {


Visibility modified to enable the child to be run from the composite test subclass

coveralls · 2016-12-04T02:58:07Z

Coverage decreased (-4.5%) to 95.464% when pulling 0bf8a53 on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

coveralls · 2016-12-04T03:07:59Z

Coverage remained the same at 100.0% when pulling 6b75425 on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

ashleyfrieze · 2016-12-04T03:08:14Z

Booyah! Coverage 100%!

greghaskins · 2016-12-04T04:33:09Z

src/test/java/specs/FixturesSpec.java

@@ -483,11 +484,10 @@
              .map((failure) -> failure.getMessage())
              .collect(Collectors.toList()));

-          it("report each error", () -> {
-            assertThat(result.get().getFailureCount(), is(2));
+          it("report the error only once", () -> {


Note: Let's not change this until we come to a decision on the other "to discuss" items.

Totally - was a temp test change. Let's revert it.

greghaskins · 2016-12-04T04:34:00Z

.../given/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/WhenRunningTheSpec.java

@@ -21,16 +21,20 @@ public void before() throws Exception {
  }

  @Test
-  public void thereAreTwoFailuresForEachAffectedTest() throws Exception {
-    assertThat(this.result.getFailureCount(), is(4));
+  public void theActualTestsCanOnlyFailOnceEach() throws Exception {


Note: Let's not change this until we come to a decision on the other "to discuss" items.

greghaskins · 2016-12-04T04:36:45Z

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

@@ -10,17 +12,17 @@
 /**
 * Represents the state of tagging for Spectrum - what it presently means.
 */
-class TaggingState {
+public class TaggingState {


If this is getting promoted to the public API, then we might need to consider it's name. At first, I thought TaggingState captured the tags for a spec, not the filter of which tags to run and which to ignore. Perhaps FilterCriteria or something like that would be more appropriate.

TaggingFilterCriteria

greghaskins · 2016-12-04T04:40:54Z

src/test/java/specs/AroundSpecs.java

@@ -71,8 +71,7 @@

        assertThat(result.getFailureCount(), is(1));
        assertThat(result.getFailures().get(0), is(failure("first spec",
-            RuntimeException.class, "aroundEach did not run the block")));
-
+            RuntimeException.class, "At least one of the test hooks did not run the test block.")));


With the generic hooks implementation, we certainly can't give as specific a message. Maybe we can at least include the location/trace to the offending hook?

Can you suggest how?

This is a minor one, we could address it in a future PR after #77 merges. I'm thinking we could capture the stacktrace at hook declaration time, and use that as the cause when throwing an InvalidHookException if it never called run(). Also, I go back and forth on whether it's a good idea to allow hooks to call run() multiple times -- it seems like doing so would probably break things. But I also don't want to limit the possibilities if we don't have to.

Technically a hook could execute an it over multiple examples... so preventing multiple runs might be wrong...but this is not the easiest of frameworks to debug a failing test in. The simpler the hooks the better.

Let's defer all of this for post merge.

greghaskins · 2016-12-04T04:46:37Z

src/test/java/specs/TaggedSpecs.java

 import static com.greghaskins.spectrum.Spectrum.beforeEach;
 import static com.greghaskins.spectrum.Spectrum.configure;
 import static com.greghaskins.spectrum.Spectrum.describe;
 import static com.greghaskins.spectrum.Spectrum.it;
 import static com.greghaskins.spectrum.Spectrum.let;
+import static com.greghaskins.spectrum.internal.PreConditionBlock.with;


Let's discuss package structure and naming for PreConditions. If we are going to generalize the API to support things like parallelization, timeouts, etc. then we might want a different term. It feels kind of like some sort of Decorator maybe?

Decorator is not quite it. I'll try a rename to BlockConfiguration

greghaskins · 2016-12-04T05:31:40Z

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

+ * destroyed. This is a twist on the usual singleton pattern, in that normally the
+ * singleton stays around indefinitely. This version is also thread safe.
+ */
+public class Singleton<T> implements Supplier<T> {


@ashleyfrieze I think I understand how this and AbstractSupplyingHook work to provide better thread safety, and speed up parallelism. Let me share my understanding and you tell me if it's correct:

With different threads running the same hooks, it could lead to weird cross-thread behavior if one thread steps on the state of another thread.

A traditional approach might use synchronized blocks to mitigate that, but they are slow and limit parallelism since threads would have to wait for others to finish before entering. Also deadlocking is a thing that sucks.

In most cases, we actually don't want shared state across threads, so traditional synchronizing makes even less sense. It's much easier to reason about the code as if it was single-threaded.

A Singleton<T> is kind of like a Variable<T> that allows each thread to have its own, separate value of the variable.

This means that, for example, setup and teardown happening on one thread probably won't collide as much with tests that are running on another thread.

Specifically, this will help with variables and state created and managed by the specs themselves. It doesn't solve anything related to side-effects such as databases or other external resources.

The Singleton itself makes no attempt to limit changing, resetting, or re-creating its values, unlike a traditional singleton which is constructed once and shared.

Regardless, it took me a while to form this understanding, which might mean we need to revisit naming or structure of these classes. Maybe ThreadSingleton or ConcurrentVariable? Come to mention it, what is the real difference between this Singleton and the built-in ThreadLocal?

In doing some quick research, I found the commentary in this StackOverflow thread interesting. Oh, boy, threading is fun.

Oh, and another thought/idea. We might try to explore the possible thread-safety failure points of Spectrum by writing some tests. When I have to rely too much on keeping the right model in my head and thinking things through properly, I start getting into trouble. That's why I love my TDD. Of course, threading-related automated tests are notoriously tricky to write. sigh.

My aim is to make it really thread safe in #61 for now, I'm just laying foundations.

I'm going to change the singleton stuff. You're right. It's got surprise in it. I'm going to claim that I am right to have chosen the name, since a Singleton is actually capable of abstracting the correct creation of an object and having more than one if you're doing things across threads.

In fact, though. The Variable needs to be threadsafe, and it seems like let and Variable ought to share more implementation. Watch this space.

greghaskins · 2016-12-04T05:39:52Z

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

+    set(before());
+    block.run();
+    after();
+    clear();


At a minimum, clear() should be in a finally block. Depending on the outcome of other discussions, after() may also need to be.

ashleyfrieze · 2016-12-05T20:41:54Z

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

@@ -188,7 +205,8 @@ public static Configuration configure() {
   * @param block {@link com.greghaskins.spectrum.Block} to run once before each spec
   */
  public static void beforeEach(final com.greghaskins.spectrum.Block block) {
-    getCurrentSuiteBeingDeclared().beforeEach(block);
+    addHook(new HookContext(before(block), getDepth(),
+        HookContext.AppliesTo.ATOMIC_ONLY, HookContext.Precedence.LOCAL));


all hookcontexts created by Spectrum which finds it is to provide the depth.

The Precedence names and application are arbitrary and "whatever I had to do to get the tests to work"

ashleyfrieze · 2016-12-05T20:43:46Z

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


-  private static final Deque<Suite> suiteStack = new ArrayDeque<>();
+  private static final Variable<Deque<Suite>> suiteStack = new Variable<>(ArrayDeque::new);


It's a threadsafe Deque - I guess this is itself just a wrapper for ThreadLocal but it feels nice using a Spectrum construct.

ashleyfrieze · 2016-12-05T20:44:16Z

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

      final com.greghaskins.spectrum.Block definitionBlock) {
-    suiteStack.push(suite);
+    getSuiteStack().push(suite);


Abstracted out our own field now it's a supplier. variable.get().push() felt too long.

ashleyfrieze · 2016-12-05T20:45:08Z

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

-  private Set<String> namesUsed = new HashSet<>();
+  private final TaggingFilterCriteria tagging;
+  private BlockConfiguration preconditions = BlockConfiguration.Factory.defaultPreConditions();
+  private NameSanitiser nameSanitiser = new NameSanitiser();


Hid the name sanitiser's implementation in another class to reduce Suite.

ashleyfrieze · 2016-12-05T20:45:27Z

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

@@ -42,7 +43,8 @@
  }

  static Suite rootSuite(final Description description) {


rootSuite was a song from Chitty Chitty Bang Bang.

coveralls · 2016-12-05T20:56:53Z

Coverage remained the same at 100.0% when pulling a359a60 on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

ashleyfrieze · 2016-12-05T20:58:37Z

src/test/java/given/a/spec/with/exception/in/aftereach/block/WhenRunningTheSpec.java

@@ -27,7 +27,8 @@ public void thereIsOneFailureForEachAffectedTest() throws Exception {
  @Test
  public void theFailureExplainsWhatHappened() throws Exception {
    assertThat(this.result.getFailures().get(0),
-        is(failure("a passing test", Fixture.SomeException.class, "kaboom")));
+        is(failure("a passing test", RuntimeException.class,
+            "given.a.spec.with.exception.in.aftereach.block.Fixture$SomeException: kaboom")));


Changed purely down to the checked exception wrapping that now happens in ThrowingConsumer

ashleyfrieze · 2016-12-05T20:59:23Z

src/test/java/given/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/Fixture.java

@@ -13,11 +13,11 @@
        describe("an exploding beforeEach", () -> {

          beforeEach(() -> {
-            throw new SomeException("kaboom");
+            throw new SomeException("beforeEach went kaboom");


Just made the message more diagnostic.

ashleyfrieze · 2016-12-05T20:59:44Z

src/test/java/given/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/Fixture.java

@@ -34,7 +34,7 @@
    return Spec.class;
  }

-  public static class SomeException extends Exception {
+  public static class SomeException extends RuntimeException {


Changed to unchecked to preserve original message.

ashleyfrieze · 2016-12-05T21:00:30Z

.../given/a/spec/with/exception/in/beforeeach/block/and/aftereach/block/WhenRunningTheSpec.java

@@ -28,9 +28,11 @@ public void thereAreTwoFailuresForEachAffectedTest() throws Exception {
  @Test
  public void theFailuresExplainWhatHappened() throws Exception {
    assertThat(this.result.getFailures().get(0),
-        is(failure("a failing test", Fixture.SomeException.class, "kaboom")));
+        is(failure("a failing test", Fixture.SomeException.class,
+            "beforeEach went kaboom")));


Here's where the test uses the more specific error message - no tricks!

ashleyfrieze · 2016-12-05T21:01:44Z

src/test/java/specs/FixturesSpec.java

          assertThat(numbers, contains(1, 2, 3, 4));
        });

        it("runs afterEach blocks from parent context", () -> {
-          assertThat(numbers, contains(1, 2, 3, 4));
+          // if afterEach block didn't call clear, then
+          // the numbers would be of size 8, not a fresh four


Added narrative to explain what the test means.

ashleyfrieze · 2016-12-05T21:02:25Z

src/test/java/specs/FixturesSpec.java

@@ -486,7 +490,7 @@
          it("report each error", () -> {
            assertThat(result.get().getFailureCount(), is(2));

-            assertThat(failureMessages.get(), hasItem("boom 1"));
+            assertThat(failureMessages.get(), hasItem("java.lang.Exception: boom 1"));


The checked exception gets wrapped. This only happens in the innermost consumer. Outer layers are (and can be) more permissive of unchecked exceptions.

coveralls · 2016-12-05T21:02:30Z

Coverage remained the same at 100.0% when pulling 5ab06c9 on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

ashleyfrieze · 2016-12-05T21:02:46Z

src/test/java/specs/FixturesSpec.java

@@ -502,12 +506,12 @@
          (result) -> () -> result.get()
              .getFailures()
              .stream()
-              .map((failure) -> failure.getMessage())
+              .map(Failure::getMessage)


Function reference beats lambda.

Nice! I'm still getting used to all these cool Java 8 features.

ashleyfrieze · 2016-12-05T21:03:04Z

src/test/java/specs/FixturesSpec.java

              .collect(Collectors.toList());

      describe("in beforeEach and afterEach", () -> {

-        final Supplier<List<String>> exceptionsThrown = let(() -> new ArrayList<>());
+        final Supplier<List<String>> exceptionsThrown = let(ArrayList::new);


Function reference again.

ashleyfrieze · 2016-12-05T21:03:20Z

src/test/java/specs/FixturesSpec.java

@@ -519,21 +523,21 @@
          describe("an explosive suite", () -> {

            beforeEach(() -> {
-              throw recordException.apply(new Exception("boom beforeEach 1"));
+              throw recordException.apply(new RuntimeException("boom beforeEach 1"));


Runtime exceptions make the tests easier to read/write.

coveralls · 2016-12-05T21:08:00Z

Coverage remained the same at 100.0% when pulling 9d6df39 on ashleyfrieze:universal-hooks into 903d5a6 on greghaskins:master.

greghaskins · 2016-12-06T03:13:31Z

@ashleyfrieze Most excellent! A very nice overhaul of the codebase, with all original tests intact and passing!

I'm prepared to merge this PR as-is, with a few to-dos on my list before the next release. I think it makes sense to resolve those as separate PRs, since this one is big enough!

With all the features we've been adding, it feels like we're approaching a 2.0 milestone. With that in mind, I'd like to

Organize the public API as a whole
- Further package structure overhaul (make more stuff internal and split out syntax or dsl package)
- Move to org.spectrumbdd namespace for 2.0, keep com.greghaskins.spectrum for 1.0-compatibility, but @Deprecated
Write more documentation in general, and for hooks specifically. This might lead to API tweaks for clarity and ease-of-use.

The above bits will probably affect this code, perhaps moving pieces around and other minor changes. If you're good with that, then I will hit the Merge button on #77.

ashleyfrieze · 2016-12-06T06:05:55Z

Yeah. Merge merge merge!

I can redo my JUnit rules piece then. Awesome!

…resent, 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

…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

First cut - demonstrates the hooks mechanism.

d947193

Ashley Frieze added 2 commits December 4, 2016 01:05

Mid-way-through xxxAll/Each

b8cf10d

All hooks now use a uniform mechanism.

105a04b

ashleyfrieze commented Dec 4, 2016

View reviewed changes

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

}

}).run(description, notifier);

};

Copy link

Contributor Author

ashleyfrieze Dec 4, 2016

Choose a reason for hiding this comment

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

Lots of code goes bye bye...

ashleyfrieze commented Dec 4, 2016

View reviewed changes

Tidy up

0bf8a53

Tidy up

6b75425

greghaskins reviewed Dec 4, 2016

View reviewed changes

ashleyfrieze commented Dec 5, 2016

View reviewed changes

Ashley Frieze added 2 commits December 5, 2016 20:52

Removed extraneous block

7e4cafe

Minor reformat

a359a60

ashleyfrieze commented Dec 5, 2016

View reviewed changes

Used Arrays.stream in preference to Stream.of

5ab06c9

ashleyfrieze commented Dec 5, 2016

View reviewed changes

Removed extraneous comment

9d6df39

greghaskins merged commit c23a4a2 into greghaskins:master Dec 6, 2016

ashleyfrieze mentioned this pull request Dec 7, 2016

Executing tests in parallel would be impaired by synchronized block in Spectrum #67

Closed

This was referenced Dec 12, 2016

JUnit rules - attempt to implement them in Spectrum #56

Merged

Consolidation refactoring - before/around/let - the grand unified theory of hooks #76

Closed


		private static final Deque<Suite> suiteStack = new ArrayDeque<>();
		private static final Variable<Deque<Suite>> suiteStack = new Variable<>(ArrayDeque::new);

		@@ -42,7 +43,8 @@
		}

		static Suite rootSuite(final Description description) {

Grand unified theory of hooks #77

Grand unified theory of hooks #77

Conversation

ashleyfrieze commented Dec 4, 2016 • edited

coveralls commented Dec 4, 2016

coveralls commented Dec 4, 2016

ashleyfrieze commented Dec 4, 2016 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashleyfrieze Dec 4, 2016 • edited

Choose a reason for hiding this comment

coveralls commented Dec 4, 2016

coveralls commented Dec 4, 2016

ashleyfrieze commented Dec 4, 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

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

coveralls commented Dec 5, 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

coveralls commented Dec 5, 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

coveralls commented Dec 5, 2016

greghaskins commented Dec 6, 2016 • edited

ashleyfrieze commented Dec 6, 2016

ashleyfrieze commented Dec 4, 2016 •

edited

ashleyfrieze commented Dec 4, 2016 •

edited

ashleyfrieze Dec 4, 2016 •

edited

greghaskins commented Dec 6, 2016 •

edited