From a14f0b9a6701c26bf01f122ce9faf4a5c03d0ada Mon Sep 17 00:00:00 2001
From: lharker <lharker@google.com>
Date: Fri, 15 Feb 2019 09:22:17 -0800
Subject: [PATCH] Don't warn on test methods in ES6 classes in the missing
 JSDoc lint check

This has been broken for a while but was unnoticed due to a separate bug, which prevented any missing JSDoc lint warnings on ES6 class methods in modules.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=234156744
---
 .../google/javascript/jscomp/NodeUtil.java    |  2 +-
 .../jscomp/lint/CheckJSDocStyle.java          | 16 ++++++----
 .../jscomp/lint/CheckJSDocStyleTest.java      | 29 +++++++++++++++++++
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java
index 41e4a7a12b3..20a92c55fcc 100644
--- a/src/com/google/javascript/jscomp/NodeUtil.java
+++ b/src/com/google/javascript/jscomp/NodeUtil.java
@@ -5355,7 +5355,7 @@ static Node getBestLValueOwner(@Nullable Node lValue) {
   }
 
   /** Get the name of the given l-value node. */
-  static String getBestLValueName(@Nullable Node lValue) {
+  public static String getBestLValueName(@Nullable Node lValue) {
     if (lValue == null || lValue.getParent() == null) {
       return null;
     }
diff --git a/src/com/google/javascript/jscomp/lint/CheckJSDocStyle.java b/src/com/google/javascript/jscomp/lint/CheckJSDocStyle.java
index 95be5058c65..8e7c751fe1e 100644
--- a/src/com/google/javascript/jscomp/lint/CheckJSDocStyle.java
+++ b/src/com/google/javascript/jscomp/lint/CheckJSDocStyle.java
@@ -226,12 +226,8 @@ private void visitClass(NodeTraversal t, Node cls) {
   }
 
   private void checkMissingJsDoc(NodeTraversal t, Node function) {
-    if (isFunctionThatShouldHaveJsDoc(t, function)) {
-      String name = NodeUtil.getName(function);
-      // Don't warn for test functions, setUp, tearDown, etc.
-      if (name == null || !ExportTestFunctions.isTestFunction(name)) {
-        t.report(function, MISSING_JSDOC);
-      }
+    if (isFunctionThatShouldHaveJsDoc(t, function) && !isTestMethod(function)) {
+      t.report(function, MISSING_JSDOC);
     }
   }
 
@@ -241,6 +237,7 @@ private void checkMissingJsDoc(NodeTraversal t, Node function) {
    */
   private boolean isFunctionThatShouldHaveJsDoc(NodeTraversal t, Node function) {
     if (!(t.inGlobalHoistScope() || t.inModuleScope())) {
+      // TODO(b/233631820): this should check for the module hoist scope instead
       return false;
     }
     if (NodeUtil.isFunctionDeclaration(function)) {
@@ -272,6 +269,13 @@ private boolean isFunctionThatShouldHaveJsDoc(NodeTraversal t, Node function) {
     return false;
   }
 
+  /** Whether this is a test method (test* or setup/teardown) that does not require JSDoc */
+  private boolean isTestMethod(Node function) {
+    Node bestLValue = NodeUtil.getBestLValue(function);
+    String name = bestLValue != null ? NodeUtil.getBestLValueName(bestLValue) : null;
+    return name != null && ExportTestFunctions.isTestFunction(name);
+  }
+
   private boolean isConstructorWithoutParameters(Node function) {
     return NodeUtil.isEs6Constructor(function)
         && !NodeUtil.getFunctionParameters(function).hasChildren();
diff --git a/test/com/google/javascript/jscomp/lint/CheckJSDocStyleTest.java b/test/com/google/javascript/jscomp/lint/CheckJSDocStyleTest.java
index 9c7085bb7b1..ea113c45ac0 100644
--- a/test/com/google/javascript/jscomp/lint/CheckJSDocStyleTest.java
+++ b/test/com/google/javascript/jscomp/lint/CheckJSDocStyleTest.java
@@ -318,6 +318,21 @@ public void testMissingJsDoc_noWarningIfNotTopLevelAndNoParams() {
         "})"));
   }
 
+  @Test
+  public void testMissingJsDoc_noWarning_wizConstructorAndDeps() {
+    // Exempt Wiz controller constructor and deps() method because Wiz automatically adds JSDoc
+    // NOTE(lharker@): right now this does not warn because of b/124061048: the behavior is correct
+    // but for the wrong reason.
+    testSame(
+        lines(
+            "goog.module('a.b.MyController');",
+            "class MyController extends SomeParentController {",
+            "  static deps() { return {model: 0}; }",
+            "  constructor({model}) {}",
+            "}",
+            "registerController(MY_CONTROLLER, MyController);"));
+  }
+
   @Test
   public void testMissingJsDoc_noWarningOnTestFunctions() {
     testSame("function testSomeFunctionality() {}");
@@ -332,6 +347,15 @@ public void testMissingJsDoc_noWarningOnTestFunctions() {
     testSame("var tearDown = function() {};");
   }
 
+  @Test
+  public void testMissingJsDoc_noWarningOnTestMethods() {
+    testSame("class MyClass { testSomeFunctionality() {} }");
+    testSame("goog.module('mod'); class MyClass { testSomeFunctionality() {} }");
+    testSame("a.b.c = class { testSomeFunctionality() {} }");
+    testSame("class MyClass { setUp() {} }");
+    testSame("class MyClass { tearDown() {} }");
+  }
+
   @Test
   public void testMissingJsDoc_noWarningOnTestFunctions_withES6Modules() {
     testSame("export function testSomeFunctionality() {}");
@@ -351,6 +375,9 @@ public void testMissingJsDoc_noWarningOnEmptyConstructor_withES6Modules() {
   public void testMissingJsDoc_googModule() {
     testWarning("goog.module('a.b.c'); function f() {}", MISSING_JSDOC);
     testWarning("goog.module('a.b.c'); var f = function() {};", MISSING_JSDOC);
+    // TODO(b/124061048): these should also warn for missing JSDoc
+    testSame("goog.module('a.b.c'); class Foo { constructor(x) {} }");
+    testSame("goog.module('a.b.c'); class Foo { someMethod() {} }");
   }
 
   @Test
@@ -382,6 +409,8 @@ public void testMissingJsDoc_ES6Module05() {
   public void testMissingJsDoc_googModule_noWarning() {
     testSame("goog.module('a.b.c'); /** @type {function()} */ function f() {}");
     testSame("goog.module('a.b.c'); /** @type {function()} */ var f = function() {};");
+    // No param constructors do not require JSDoc
+    testSame("goog.module('a.b.c'); class Foo { constructor() {} }");
   }
 
   @Test