Closure-compiler: Turn compareJsDoc on in all unit tests

[Dominator008]

git grep "compareJsDoc = false" gives:

test/com/google/javascript/jscomp/AmbiguatePropertiesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/AngularPassTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/CheckAccessControlsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/CollapsePropertiesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/CollapseVariableDeclarationsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/DevirtualizePrototypeMethodsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/Es6InlineTypesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/GenerateExportsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/InferJSDocInfoTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/InlinePropertiesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/InlineVariablesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/JsdocToEs6TypedConverterTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/NormalizeTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/OptimizeParametersTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/ProcessCommonJSModulesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/ProcessDefinesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/RenameVarsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/ReplaceIdGeneratorsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/ReplaceMessagesForChromeTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/ReplaceMessagesTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/ReplaceStringsTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/RuntimeTypeCheckTest.java: compareJsDoc = false;
test/com/google/javascript/jscomp/VarCheckTest.java: compareJsDoc = false;

We should eventually turn it on for all of them and remove the compareJsDoc field.

[MatrixFrog]

This would be an easy (but tedious) task that a new contributor could do if they wanted to.

[sveningsonrobin]

If possible I would like to claim this issue @Dominator008

I have started to remove compareJsDoc, however I am running in to a strange issue in CheckAccessControlsTest.java. The following code:

public void testFileoverviewVisibilityDoesNotApplyToGoogProvidedNamespace2() {
    test(
        ImmutableList.of(
            SourceFile.fromCode(
                Compiler.joinPathParts("foo", "bar.js"),
                LINE_JOINER.join(
                "/**\n",
                "  * @fileoverview\n",
                "  * @package\n",
                "  */\n",
                "goog.provide('foo.bar');")),
            SourceFile.fromCode("foo.js", "goog.provide('foo');"),
            SourceFile.fromCode(
                "bar.js",
                "goog.require('foo');\n"
                + "/** @const */ var x = foo;")),
        ImmutableList.of(
            SourceFile.fromCode(
                Compiler.joinPathParts("foo", "bar.js"),
                LINE_JOINER.join(
                "/** @const */ var foo={};",
                "/**\n",
                "  * @fileoverview\n",
                "  * @package\n",
                "  */\n",
                "/** @const */ foo.bar={};")),
            SourceFile.fromCode("foo.js", ""),
            SourceFile.fromCode("bar.js", "/** @const */ var x=foo")),
        null, null);
}

-- Source: link to fork commit

Generates this error:

java.lang.AssertionError: Not true that <(com.google.javascript.jscomp.JSError[]) [JSC_BAD_PACKAGE_PROPERTY_ACCESS. Access to package-private property foo of foo/bar.js not allowed here. at bar.js line 2 : 22]> is empty
    at com.google.javascript.jscomp.CompilerTestCase.test(CompilerTestCase.java:1422)
    ...

Any hints on why the introduction of jsdoc breaks the test? Has it anything to do with the @public jsdoc notation? (this would be strange though since no jsdoc notation should change the code meaning, right?)

[sveningsonrobin]

Also, I am working under the assumption that you are trying to phase out the use of normal string concatenation, and replace it with the use of LINE_JOINER.joininstead. Is this correct?

[sveningsonrobin]

Another problem that has occurred is that for the files JsdocToEs6TypedConverterTest.javaand Es6InlineTypesTest.java, an error is generated:

junit.framework.AssertionFailedError: Unexpected parse error(s): MISPLACED_TYPE_SYNTAX. Can only have JSDoc or inline type annotations, not both at expected0 line 1 : 39

Me and @lbercken has tried to wrap our heads around this issue for a while. Correct me if I am wrong, but we came to the conclusion that these tests should never be possible to run if the flag compareJsDoc is true, since the inline type annotation can't be combined with JSDoc, and therefore we can never do these checks. What is a good solution to this problem? I assume you want to get rid of the compareJsDoc flag completely, so we have to change how these tests work?

[sveningsonrobin]

Except for the three files containing failed tests described above, I have now successfully removed all of the compareJsDoc = false as well as added LINE_JOINER.join everywhere it is necessary. This is done with 12 commits to our own fork. Do you want me to do 1 huge PR with all 12 commits squashed, or 12 separate PRs, or is there another better solution?

[MatrixFrog]

Personally if I were working on this I would ignore the JsdocToEs6TypedConverterTest. Anyway one big PR is probably a bit easier in this case.

[sveningsonrobin]

Personally if I were working on this I would ignore the JsdocToEs6TypedConverterTest

But would this issue ever be closed and resolved then? Because to remove the flag completely, these errors needs to be adressed, right?

[MatrixFrog]

Correct. I just don't think it's worth our time worrying about the "ES6 typed" mode. Go for it though if you want.

[sveningsonrobin]

If I understand the issue correctly, the solutions are narrowed down to either:

• Remove the compareJsDoc flag, remove the tests for "ES6 typed" (and also remove "ES6 typed" support in the closure compiler?)

or

• Leave the flag, and continue testing "ES6 typed"

Because no solution can do both, right?

[MatrixFrog]

Enabling compareJsDoc for as many classes as possible is worthwhile even if we don't get 100% of them. The bonus from getting 100% is we get to remove the flag, but I'm happy (to the extent that I'm ever "happy") even without that bonus.

[sveningsonrobin]

Okay, fair enough! But just to be clear, there is no possible way of getting rid of the flag unless support of "ES6 typed" is revoked? Or am I missing something?

[MatrixFrog]

If every test stops using the flag we can get rid of the flag.