Fix Issue greghaskins#132 -- Make let Lazily Evaluated

The previous implementation would cause all `let` blocks to be evaluated at the start of the test rather than on an as-needed basis, which is inconsistent with the behavior developers who are used to RSpec would expect. Now, `let` only evaluates its block when the test first needs a value, allowing the `let` block to depend on state that is being setup by other blocks (`beforeEach`, other `let` blocks, etc).

Author: Guy Paddock

src/main/java/com/greghaskins/spectrum/internal/hooks/LetHook.java

@@ -1,24 +1,77 @@
 package com.greghaskins.spectrum.internal.hooks;
 
+import com.greghaskins.spectrum.Block;
 import com.greghaskins.spectrum.ThrowingSupplier;
+import com.greghaskins.spectrum.Variable;
+import com.greghaskins.spectrum.internal.DeclarationState;
+import com.greghaskins.spectrum.internal.RunReporting;
+
+import org.junit.runner.Description;
+import org.junit.runner.notification.Failure;
 
 /**
- * Implementation of let as a supplying hook.
+ * Implementation of {@code let} as a supplying hook.
+ *
+ * <p>Using {@code let} allows you to define shared values that can be used by multiple tests,
+ * without having to worry about cleaning up the values between tests to prevent shared state in
+ * one test from affecting the results of another.
+ *
+ * <p>Values are lazily initialized and then cached, so a value is not calculated until the first
+ * time it is needed in a given test. Subsequent fetches of the value within the same test will
+ * return the cached value.
  */
-public class LetHook<T> extends AbstractSupplyingHook<T> {
-
+public class LetHook<T> implements SupplyingHook<T> {
   private final ThrowingSupplier<T> supplier;
+  private final Variable<T> cachedValue = new Variable<>();
+  private boolean isCached;
 
   public LetHook(final ThrowingSupplier<T> supplier) {
     this.supplier = supplier;
+    this.isCached = false;
   }
 
-  protected T before() {
-    return supplier.get();
+  @Override
+  public void accept(final Description description,
+      final RunReporting<Description, Failure> reporting, final Block block)
+      throws Throwable {
+    try {
+      block.run();
+    } finally {
+      clear();
+    }
+  }
+
+  @Override
+  public T get() {
+    assertSpectrumIsRunningTestsNotDeclaringThem();
+
+    if (!this.isCached) {
+      this.cachedValue.set(supplier.get());
+
+      this.isCached = true;
+    }
+
+    return this.cachedValue.get();
   }
 
   protected String getExceptionMessageIfUsedAtDeclarationTime() {
     return "Cannot use the value from let() in a suite declaration. "
         + "It may only be used in the context of a running spec.";
   }
+
+  private void clear() {
+    this.isCached = false;
+    this.cachedValue.set(null);
+  }
+
+  /**
+   * Will throw an exception if this method happens to be called while Spectrum is still defining
+   * tests, rather than executing them. Useful to see if a hook is being accidentally used during
+   * definition.
+   */
+  private void assertSpectrumIsRunningTestsNotDeclaringThem() {
+    if (DeclarationState.instance().getCurrentSuiteBeingDeclared() != null) {
+      throw new IllegalStateException(getExceptionMessageIfUsedAtDeclarationTime());
+    }
+  }
 }

src/test/java/specs/LetSpecs.java

@@ -1,6 +1,8 @@
 package specs;
 
 import static com.greghaskins.spectrum.dsl.specification.Specification.afterEach;
+import static com.greghaskins.spectrum.dsl.specification.Specification.beforeEach;
+import static com.greghaskins.spectrum.dsl.specification.Specification.context;
 import static com.greghaskins.spectrum.dsl.specification.Specification.describe;
 import static com.greghaskins.spectrum.dsl.specification.Specification.it;
 import static com.greghaskins.spectrum.dsl.specification.Specification.let;
@@ -15,6 +17,7 @@
 
 import com.greghaskins.spectrum.Spectrum;
 import com.greghaskins.spectrum.SpectrumHelper;
+import com.greghaskins.spectrum.Variable;
 
 import org.junit.runner.Result;
 import org.junit.runner.RunWith;
@@ -29,7 +32,6 @@
 public class LetSpecs {
   {
     describe("The `let` helper function", () -> {
-
       final Supplier<List<String>> items = let(() -> new ArrayList<>(asList("foo", "bar")));
 
       it("is a way to supply a value for specs", () -> {
@@ -41,18 +43,36 @@ public class LetSpecs {
 
         items.get().add("baz");
         items.get().add("blah");
+
         assertThat(items.get(), contains("foo", "bar", "baz", "blah"));
       });
 
+      context("when the value returned by the supplier is `null`", () -> {
+        final AtomicInteger callCounter = new AtomicInteger();
+
+        final Supplier<String> stringLet = let(() -> {
+          if (callCounter.getAndIncrement() == 0) {
+            return null;
+          } else {
+            return "fail";
+          }
+        });
+
+        it("does not call the supplier multiple times", () -> {
+          assertThat(stringLet.get(), is(nullValue()));
+          assertThat(stringLet.get(), is(nullValue()));
+        });
+      });
+
       it("creates a fresh value for every spec", () -> {
         assertThat(items.get(), contains("foo", "bar"));
       });
 
       describe("in complex test hierarchies", () -> {
         describe("a new let object is created for each spec", () -> {
           AtomicInteger integer = new AtomicInteger();
-          describe("a thing", () -> {
 
+          describe("a thing", () -> {
             final Supplier<Integer> intLet = let(integer::getAndIncrement);
 
             it("starts with one value", () -> {
@@ -85,14 +105,26 @@ public class LetSpecs {
                 });
               });
             });
+          });
+        });
+      });
+
+      describe("lazy initialization", () -> {
+        context("when setup has to be done before a let is evaluated", () -> {
+          final Variable<Integer> theVariable = new Variable<>();
+          final Supplier<Integer> result = let(() -> theVariable.get());
 
+          beforeEach(() -> {
+            theVariable.set(123);
           });
 
+          it("does not cache a value until the let is referenced", () -> {
+            assertThat(result.get(), is(123));
+          });
         });
       });
 
       describe("when trying to use a value outside a spec", () -> {
-
         final Supplier<Result> result =
             let(() -> SpectrumHelper.run(getSuiteThatUsesLetValueOutsideSpec()));