search

google / jsinterop-generator

Generates Java annotated with JsInterop from JavaScript extern sources
Apache License 2.0
75 stars 24 forks source link

Fix simple test to reflect the generated source. #8

Closed niloc132 closed 6 years ago

niloc132 commented 6 years ago

These imports probably shouldnt be necessary, but seems to reflect the current state of what the generator emits, and allowing the test to otherwise pass means that we can still use the test to know that other aspects of the project are working.

Error before this change:

FAIL: //javatests/jsinterop/generator/externs/simpleclass:SimpleClass (see /private/var/tmp/_bazel_colin/5b520ca804080a736147bef5f5abb949/execroot/com_google_jsinterop_generator/bazel-out/darwin-fastbuild/testlogs/javatests/jsinterop/generator/externs/simpleclass/SimpleClass/test.log)

Contents of that file:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
-----------------------------------------------------------------------------
/private/var/tmp/_bazel_colin/5b520ca804080a736147bef5f5abb949/sandbox/2848044578543892971/execroot/com_google_jsinterop_generator/bazel-out/darwin-fastbuild/bin/javatests/jsinterop/generator/externs/simpleclass/SimpleClass.runfiles/com_google_jsinterop_generator
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/PrivateClass.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleClass.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleClass__Constants.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleInterface.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleInterface__Constants.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleStructuralInterface.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/PrivateClass.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleClass.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleClass__Constants.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleInterface.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleInterface__Constants.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleStructuralInterface.java
sed: 1: "/var/folders/hd/d1bzsn8 ...": invalid command code f
--- /var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleClass.java  2018-05-19 08:22:10.000000000 +0000
+++ /var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleClass.java  2018-05-19 13:22:27.000000000 +0000
@@ -1,7 +1,5 @@
 package jsinterop.generator.externs.simpleclass;

-import java.lang.Object;
-import java.lang.String;
 import jsinterop.annotations.JsMethod;
 import jsinterop.annotations.JsOverlay;
 import jsinterop.annotations.JsPackage;
content of expected file "/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.1jGAxjjI/jsinterop/generator/externs/simpleclass/SimpleClass.java" and actual file "/var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.bxeOcEpL/SimpleClass.java" differ
googlebot commented 6 years ago

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

niloc132 commented 6 years ago

Remaining tests are fixed in the same way in github.com/vertispan/jsinterop-generator/commit/08648b7b031efad300f33430bde90f62194eb87c

googlebot commented 6 years ago

CLAs look good, thanks!

jDramaix commented 6 years ago

The generator emits all imports but we use the google java format tool in order to format both generated files and golden files before to compare them. The formatter should remove the unnecessary imports (at least the internal version does). I'll investigate.

niloc132 commented 6 years ago

Hmm - comment typos probably aren't covered by that though, how are those tests passing internally? Or is the diff presented when the test fails just a hint at the problem, but comments don't actually get used in comparing?

jDramaix commented 6 years ago

Should be fixed with : 899c11d09e7fe731a453ea0bef2ba3a8f5312e83

Could you retest?

jDramaix commented 6 years ago

Hmm - comment typos probably aren't covered by that though, how are those tests passing internally?

Comments are stripped before comparing file contents.

niloc132 commented 6 years ago

Will retest today, thanks. Would be nice to apply the same comment-stripper before seeing output, and maybe don't even put the comments in the "expected" source if they aren't actually expected to be there?

Or, since they are actually useful (before super() calls), perhaps don't strip them?

Thanks again.

niloc132 commented 6 years ago

Looking better for sure - 2 failed, both comment related. Here's the generated diffs.

//javatests/jsinterop/generator/externs/dependency:dependency FAILED in 8.6s

--- /var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.i2Tjvcz1/jsinterop/generator/externs/dependency/MyLibThirdPartyClass.java      2018-05-30 20:17:48.000000000 +0000
+++ /var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.KaBTtyky/MyLibThirdPartyClass.java     2018-05-31 01:17:49.000000000 +0000
@@ -51,17 +51,14 @@
   public ThirdParty2Class extraField;

   public MyLibThirdPartyClass(ThirdPartyClass.ConstructorFooUnionType foo) {
-    // This call is only here for java compilation purpose.
     super((ThirdPartyClass.ConstructorFooUnionType) null);
   }

   public MyLibThirdPartyClass(String foo) {
-    // This call is only here for java compilation purpose.
     super((ThirdPartyClass.ConstructorFooUnionType) null);
   }

   public MyLibThirdPartyClass(double foo) {
-    // This call is only here for java compilation purpose.
     super((ThirdPartyClass.ConstructorFooUnionType) null);
   }

//javatests/jsinterop/generator/externs/inheritance:Inheritance FAILED in 21.8s

--- /var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.665RbXj7/jsinterop/generator/externs/inheritance/GreatParentClass.java 2018-05-30 20:17:24.000000000 +0000
+++ /var/folders/hd/d1bzsn8x6s385s5nkc1m29zr0000gn/T/tmp.2jS0YUtu/GreatParentClass.java 2018-05-31 01:17:26.000000000 +0000
@@ -8,7 +8,7 @@
   public double greatParentClassProperty;

   public GreatParentClass(String s, boolean b, double n) {
-    // This call is only here for java compilation purpose.
+    // This call is only there for java compilation purpose.
     super((Object) null);
   }

From this it appears that the import issue is resolved, but that comments are not stripped, changes are detected.

I can update this PR to reflect this? Or do you want to change how the test works instead?

jDramaix commented 6 years ago

out of curiosity: Are you on mac os? Tests works fine on my linux machine but fails on my Mac.

Seems the sed command I use in javatests/jsinterop/generator/jsinterop_generator_test.sh for stripping comments doesn't work on mac os :

# allows us to comment some part of java code when a feature is not implemented.
strip_java_comments() {
  find "$1" -type f -name '*.java' -print -exec sed -i '/\/\/.*$/d' '{}' \;
}

I'll investigate that tomorrow

niloc132 commented 6 years ago

Yes, I am running on a mac.

jDramaix commented 6 years ago

With the BSD build of sed (one installed by default on Mac OS X), backup extension is mandatory when you specify -i option.

I've a patch internally to fix that. It should be pushed today

jDramaix commented 6 years ago

Should be fixed with bbbd78c could you sync and rerun the tests?

niloc132 commented 6 years ago

All passing locally now, thanks!