Moves testing methods checkTreeEquals* out of Node and into `Node…

…Subject`.

All call sites are updated and tests for the code are moved as well.

This results in better error messages because now `NodeSubject` has more context about the assertion failure. It also eliminates some ugly `@VisbleForTesting` annotations.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=238346642

src/com/google/javascript/jscomp/ReplaceMessages.java

@@ -183,9 +183,12 @@ private void updateFunctionNode(JsMessage message, Node functionNode)
     Node valueNode = constructAddOrStringNode(iterator, argListNode);
     Node newBlockNode = IR.block(IR.returnNode(valueNode));
 
-    // TODO(user): checkTreeEqual is overkill. I am in process of rewriting
-    // these functions.
-    if (newBlockNode.checkTreeEquals(oldBlockNode) != null) {
+    if (!newBlockNode.isEquivalentTo(
+        oldBlockNode,
+        /* compareType= */ false,
+        /* recurse= */ true,
+        /* jsDoc= */ false,
+        /* sideEffect= */ false)) {
       newBlockNode.useSourceInfoIfMissingFromForTree(oldBlockNode);
       functionNode.replaceChild(oldBlockNode, newBlockNode);
       compiler.reportChangeToEnclosingScope(newBlockNode);

src/com/google/javascript/rhino/Node.java

@@ -1880,99 +1880,6 @@ public final boolean hasChild(Node child) {
     return false;
   }
 
-  /**
-   * Checks if the subtree under this node is the same as another subtree. Returns null if it's
-   * equal, or a message describing the differences. Should be called with {@code this} as the
-   * "expected" node and {@code actual} as the "actual" node.
-   */
-  @VisibleForTesting
-  @Nullable
-  public final String checkTreeEquals(Node actual) {
-      NodeMismatch diff = checkTreeEqualsImpl(actual);
-      if (diff != null) {
-        return "Node tree inequality:" +
-            "\nTree1:\n" + toStringTree() +
-            "\n\nTree2:\n" + actual.toStringTree() +
-            "\n\nSubtree1: " + diff.nodeExpected.toStringTree() +
-            "\n\nSubtree2: " + diff.nodeActual.toStringTree();
-      }
-      return null;
-  }
-
-  /**
-   * Checks if the subtree under this node is the same as another subtree. Returns null if it's
-   * equal, or a message describing the differences. Considers two nodes to be unequal if their
-   * JSDocInfo doesn't match. Should be called with {@code this} as the "expected" node and {@code
-   * actual} as the "actual" node.
-   *
-   * @see JSDocInfo#equals(Object)
-   */
-  @VisibleForTesting
-  @Nullable
-  public final String checkTreeEqualsIncludingJsDoc(Node actual) {
-      NodeMismatch diff = checkTreeEqualsImpl(actual, true);
-      if (diff != null) {
-        if (diff.nodeActual.isEquivalentTo(diff.nodeExpected, false, true, false)) {
-          // The only difference is that the JSDoc is different on
-          // the subtree.
-          String jsDocActual = diff.nodeActual.getJSDocInfo() == null ?
-              "(none)" :
-              diff.nodeActual.getJSDocInfo().toStringVerbose();
-
-          String jsDocExpected = diff.nodeExpected.getJSDocInfo() == null ?
-              "(none)" :
-              diff.nodeExpected.getJSDocInfo().toStringVerbose();
-
-          return "Node tree inequality:" +
-              "\nTree:\n" + toStringTree() +
-              "\n\nJSDoc differs on subtree: " + diff.nodeExpected +
-              "\nExpected JSDoc: " + jsDocExpected +
-              "\nActual JSDoc  : " + jsDocActual;
-        }
-        return "Node tree inequality:" +
-            "\nExpected tree:\n" + toStringTree() +
-            "\n\nActual tree:\n" + actual.toStringTree() +
-            "\n\nExpected subtree: " + diff.nodeExpected.toStringTree() +
-            "\n\nActual subtree: " + diff.nodeActual.toStringTree();
-      }
-      return null;
-  }
-
-  /**
-   * Compare this node to the given node recursively and return the first pair of nodes that differs
-   * doing a preorder depth-first traversal. Package private for testing. Returns null if the nodes
-   * are equivalent. Should be called with {@code this} as the "expected" node and {@code actual} as
-   * the "actual" node.
-   */
-  @Nullable
-  final NodeMismatch checkTreeEqualsImpl(Node actual) {
-    return checkTreeEqualsImpl(actual, false);
-  }
-
-  /**
-   * Compare this node to the given node recursively and return the first pair of nodes that differs
-   * doing a preorder depth-first traversal. Should be called with {@code this} as the "expected"
-   * node and {@code actual} as the "actual" node.
-   *
-   * @param jsDoc Whether to check for differences in JSDoc.
-   */
-  @Nullable
-  private NodeMismatch checkTreeEqualsImpl(Node actual, boolean jsDoc) {
-    if (!isEquivalentTo(actual, false, false, jsDoc)) {
-      return new NodeMismatch(this, actual);
-    }
-
-    for (Node expectedChild = first, actualChild = actual.first;
-         expectedChild != null;
-         expectedChild = expectedChild.next, actualChild = actualChild.next) {
-      NodeMismatch res = expectedChild.checkTreeEqualsImpl(actualChild, jsDoc);
-      if (res != null) {
-        return res;
-      }
-    }
-    return null;
-  }
-
   /** Checks equivalence without going into child nodes */
   public final boolean isEquivalentToShallow(Node node) {
     return isEquivalentTo(node, false, false, false, false);
@@ -3081,31 +2988,6 @@ public void setQuotedString() {
     throw new IllegalStateException(this + " is not a StringNode");
   }
 
-  static final class NodeMismatch {
-    final Node nodeExpected;
-    final Node nodeActual;
-
-    NodeMismatch(Node nodeExpected, Node nodeActual) {
-      this.nodeExpected = nodeExpected;
-      this.nodeActual = nodeActual;
-    }
-
-    @Override
-    public boolean equals(@Nullable Object object) {
-      if (object instanceof NodeMismatch) {
-        NodeMismatch that = (NodeMismatch) object;
-        return that.nodeExpected.equals(this.nodeExpected)
-            && that.nodeActual.equals(this.nodeActual);
-      }
-      return false;
-    }
-
-    @Override
-    public int hashCode() {
-      return Objects.hashCode(nodeExpected, nodeActual);
-    }
-  }
-
 
   /*** AST type check methods ***/
 

src/com/google/javascript/rhino/testing/Asserts.java

@@ -118,14 +118,18 @@ public static void assertEquivalenceOperations(JSType a, JSType b) {
    * similar API in JUnit 4.13, but the compiler is currently pinned on 4.12, which doesn't include
    * it.
    */
-  public static void assertThrows(
-      Class<? extends Throwable> exceptionClass, ThrowingRunnable runnable) {
+  @SuppressWarnings("unchecked")
+  public static <T extends Throwable> T assertThrows(
+      Class<T> exceptionClass, ThrowingRunnable runnable) {
     try {
       runnable.run();
-      assertWithMessage("Did not get expected exception: %s", exceptionClass).fail();
     } catch (Throwable expectedException) {
       assertThat(expectedException).isInstanceOf(exceptionClass);
+      return (T) expectedException; // Unchecked cast. J2CL doesn't support `Class::cast`.
     }
+
+    assertWithMessage("Did not get expected exception: %s", exceptionClass).fail();
+    throw new AssertionError("Impossible");
   }
 
   /** Functional interface for use with {@link #assertThrows}. */

src/com/google/javascript/rhino/testing/NodeSubject.java

@@ -39,14 +39,24 @@
 
 package com.google.javascript.rhino.testing;
 
+import static com.google.common.collect.Streams.stream;
+import static com.google.common.truth.Fact.fact;
+import static com.google.common.truth.Fact.simpleFact;
 import static com.google.common.truth.Truth.assertAbout;
 
+import com.google.common.collect.Streams;
+import com.google.common.truth.Fact;
 import com.google.common.truth.FailureMetadata;
 import com.google.common.truth.StringSubject;
 import com.google.common.truth.Subject;
 import com.google.errorprone.annotations.CheckReturnValue;
+import com.google.javascript.rhino.JSDocInfo;
 import com.google.javascript.rhino.Node;
 import com.google.javascript.rhino.Token;
+import java.util.ArrayList;
+import java.util.Optional;
+import java.util.function.Function;
+import javax.annotation.Nullable;
 
 /**
  * A Truth Subject for the Node class. Usage:
@@ -59,6 +69,9 @@
  * </pre>
  */
 public final class NodeSubject extends Subject<NodeSubject, Node> {
+
+  private Function<Node, String> serializer;
+
   @CheckReturnValue
   public static NodeSubject assertNode(Node node) {
     return assertAbout(nodes()).that(node);
@@ -72,12 +85,79 @@ private NodeSubject(FailureMetadata failureMetadata, Node node) {
     super(failureMetadata, node);
   }
 
-  @Override // TODO(nickreid): This isn't really equality based. Use a different name.
-  public void isEqualTo(Object o) {
-    check().that(o).isInstanceOf(Node.class);
-    Node node = (Node) o;
+  /**
+   * Specify a function to use when rendering {@code Node}s into assertion failure messages.
+   *
+   * <p>A common choice of serializer is {@link Compiler::toSource}, to as render JavaScript code.
+   */
+  public NodeSubject usingSerializer(Function<Node, String> serializer) {
+    this.serializer = serializer;
+    return this;
+  }
+
+  @Override
+  public void isEqualTo(Object expected) {
+    throw new UnsupportedOperationException("Use an overload with a declared type.");
+  }
+
+  /**
+   * Compare the node-trees of actual and expected.
+   *
+   * <p>The per-node comparison ignores:
+   *
+   * <ul>
+   *   <li>Types
+   *   <li>JSDoc
+   *   <li>Side-effects
+   * </ul>
+   */
+  // TODO(nickreid): This isn't really equality based. Use a different name.
+  public void isEqualTo(Node expected) {
+    isEqualToInternal(expected, /* checkJsdoc= */ false);
+  }
+
+  /**
+   * Compare the node-trees of actual and expected.
+   *
+   * <p>The per-node comparison ignores:
+   *
+   * <ul>
+   *   <li>Types
+   *   <li>Side-effects
+   * </ul>
+   */
+  // TODO(nickreid): This isn't really equality based. Use a different name.
+  public void isEqualIncludingJsDocTo(Node expected) {
+    isEqualToInternal(expected, /* checkJsdoc= */ true);
+  }
+
+  // TODO(nickreid): This isn't really equality based. Use a different name.
+  public void isEqualToInternal(Node expected, boolean checkJsdoc) {
+    isNotNull();
+    assertNode(expected).isNotNull();
+
+    findFirstMismatch(actual(), expected, checkJsdoc)
+        .ifPresent(
+            (mismatch) -> {
+              ArrayList<Fact> facts = new ArrayList<>();
+              facts.add(fact("Actual", serializeNode(actual())));
+              facts.add(fact("Expected", serializeNode(expected)));
+
+              Node misActual = mismatch.actual;
+              facts.add(fact("Actual mismatch", serializeNode(misActual)));
+              if (checkJsdoc) {
+                facts.add(fact("Actual JSDoc", jsdocToStringNullsafe(misActual.getJSDocInfo())));
+              }
 
-    check("checkTreeEquals(%s)", node).that(actual().checkTreeEquals(node)).isNull();
+              Node misExpected = mismatch.expected;
+              facts.add(fact("Expected mismatch", serializeNode(misExpected)));
+              if (checkJsdoc) {
+                facts.add(
+                    fact("Expected JSDoc", jsdocToStringNullsafe(misExpected.getJSDocInfo())));
+              }
+
+              failWithoutActual(simpleFact("Node tree inequality"), facts.toArray(new Fact[0]));
+            });
   }
 
   public NodeSubject isEquivalentTo(Node other) {
@@ -193,4 +273,55 @@ public NodeSubject hasChildren(boolean hasChildren) {
   public StringSubject hasStringThat() {
     return check("getString()").that(actual().getString());
   }
+
+  @Override
+  protected String actualCustomStringRepresentation() {
+    return serializeNode(actual());
+  }
+
+  /**
+   * Compare the given node-trees recursively and return the first pair of nodes that differs doing
+   * a pre-order traversal.
+   *
+   * @param jsDoc Whether to check for differences in JSDoc.
+   */
+  private static Optional<NodeMismatch> findFirstMismatch(
+      Node actual, Node expected, boolean jsDoc) {
+    if (!actual.isEquivalentTo(
+        expected, /* compareType= */ false, /* recurse= */ false, jsDoc, /* sideEffect= */ false)) {
+      return Optional.of(new NodeMismatch(actual, expected));
+    }
+
+    // `isEquivalentTo` confirms that the number of children is the same.
+    return Streams.zip(
+            stream(actual.children()),
+            stream(expected.children()),
+            (actualChild, expectedChild) -> findFirstMismatch(actualChild, expectedChild, jsDoc))
+        .filter(Optional::isPresent)
+        .findFirst()
+        .orElse(Optional.empty());
+  }
+
+  /** A pair of nodes that were expected to match in some way but didn't. */
+  private static final class NodeMismatch {
+    final Node actual;
+    final Node expected;
+
+    NodeMismatch(Node actual, Node expected) {
+      this.actual = actual;
+      this.expected = expected;
+    }
+  }
+
+  private String serializeNode(Node node) {
+    if (serializer != null) {
+      return serializer.apply(node);
+    } else {
+      return node.toStringTree();
+    }
+  }
+
+  private static String jsdocToStringNullsafe(@Nullable JSDocInfo jsdoc) {
+    return jsdoc == null ? "(null)" : jsdoc.toStringVerbose();
+  }
 }

test/com/google/javascript/jscomp/CodePrinterTest.java

@@ -17,8 +17,8 @@
 package com.google.javascript.jscomp;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.javascript.jscomp.CompilerTestCase.lines;
+import static com.google.javascript.rhino.testing.NodeSubject.assertNode;
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
@@ -1999,18 +1999,9 @@ public void testParsePrintParse() {
 
   private void testReparse(String code) {
     Compiler compiler = new Compiler();
-    Node parse1 = parse(code);
-    Node parse2 = parse(new CodePrinter.Builder(parse1).build());
-    String explanation = parse1.checkTreeEquals(parse2);
-    assertWithMessage(
-            "\nExpected: "
-                + compiler.toSource(parse1)
-                + "\nResult: "
-                + compiler.toSource(parse2)
-                + "\n"
-                + explanation)
-        .that(explanation)
-        .isNull();
+    Node parseOnce = parse(code);
+    Node parseTwice = parse(new CodePrinter.Builder(parseOnce).build());
+    assertNode(parseTwice).usingSerializer(compiler::toSource).isEqualTo(parseOnce);
   }
 
   @Test