Running tests?
niloc132 opened this issue · 11 comments
Hey guys, I'm looking at getting these tests running in open source, as I have a few changes I'd like to propose, and thought it might make sense to submit tests to help make my case. I nearly have the tests compiling, but one class is eluding me: com.google.common.annotations.UsedReflectively. I haven't found any trace of it on the internet - except for google/guava#1617, which says (as of 2013) that it could get put somewhere more general for others to see it.
In context (and from the linked issue above), it might just be the equivalent of @SuppressWarnings("unused"), but considering how it is used, it could also have specific meaning to something like GWT->Closure or J2CL->Closure, in that the method name shouldn't be obfuscated. @JsMethod (with exports turned on during tests) would be enough to ensure that for GWT2 at least, but this annotation might have other work to do in cases involving closure.
Removing all trace of this annotation seems to at least get tests to compile (though I'm still missing a few stray dependencies). Is this something that can be made more general, so I could propose a patch to get tests running?
UsedReflectively is an internal annotation. I've created a change request internally in order to replace it by SuppressWarning
Thanks Julien - I've got all dependencies (that I know of...) sorted, and the annotation removed, the next step is that I can't figure out how to tell bazel that I actually want sources of my dependencies. For example, the existing //third_party:gwt-jsinterop-annotations rule seems to point at the maven jar with both .class and .java, as well as .gwt.xml, but when run on that target, only classfiles are present, the sources have been stripped out. It seems like //third_party:libgwt-jsinterop-annotations-src.jar ought to point to the source jar (since maven_jar seems to build a jar for sources and a jar for bytecode, and a bazel java_import pointing at both), but the resulting jar actually is empty.
Is there a correct way in bazel to depend on a source jar from another target? The only alternative I'm currently seeing is to amend upstream gwt's user/BUILD.bazel to include the jsinterop targets, and depend on it that way.
bazelbuild/bazel#308 apparently documents the addition of downloading distinct source jars, but this (and the resulting bazelbuild/bazel#4798) shouldn't seem to apply, since the regular jar already had sources inside - we just need to get it on the classpath of the junit runner.
I didn't resolve the sources issue directly, but added a target to gwt's BUILD.bazel for the moment.
To get Truth working (which is shipped as a gwt-classified target), I had to replace the maven_jar impl with the skylark one. Frustratingly, this takes a completely different gav spec (order of artifact type and version is inverted in the string?!?), but lets you specify a different classifier, and then it seems to load correctly. Might work for sources too, will try that again so I can revert other changes.
A dozen or two new dependencies manually added later (will attempt the generate-workspace later to see if it gets it right on the first try from already-working poms), and I can run tests, but they are failing out of the box: JsTest and a few others have this code:
// The extra indirection here prevents GWT optimization over params.
@JsMethod(name = "getArguments", namespace = "jsinterop.base.GetArgumentsHelper")
private static native JsArrayLike<Object> getArguments(Object... args);
This doesn't work as-is in GWT (though the comment seems to suggest that it should, and isn't J2CL specific? why else mention GWT by name?), but instead GetArgumentsHelper needs to be annotated as @JsType (and -generateJsInteropExports turned on of course), and then all tests pass. Is this a faulty comment, or export error in dropping the JsType a annotation, or perhaps is there another way that this should still work in GWT with just the JsMethod annotation?
I'm working to simplify my code now, but with all current tests passing I'm hoping to add a missing feature, and possibly correct a few minor oversights.
I'm not certain that this is the right answer, I'm asking, because it doesn't look right to me either way.
Without the JsType (and implied -generateJsInteropExports), the @JsMethod(name = "getArguments", namespace = "jsinterop.base.GetArgumentsHelper") needed because "The extra indirection here prevents GWT optimization over params." doesn't actually exist... doesn't it?
Test failure that I get here (one of seven that fail):
7) testArguments(jsinterop.base.JsTest)
com.google.gwt.core.client.JavaScriptException: (TypeError) : Cannot read property "base" from undefined
at jsinterop.base.JsTest.testArguments(JsTest.java:212)
at Unknown.anonymous(GWTTestMetadataImpl.java:62)
at com.google.gwt.junit.client.impl.GWTTestAccessor.$invoke(GWTTestAccessor.java:35)
at com.google.gwt.junit.client.impl.GWTRunner.$executeTestMethod(GWTRunner.java:226)
at com.google.gwt.junit.client.GWTTestCase.$doRunTest(GWTTestCase.java:157)
at com.google.gwt.junit.client.GWTTestCase.$runBare(TestCase.java:59)
at com.google.gwt.junit.client.GWTTestCase.$__doRunTest(GWTTestCase.java:115)
at com.google.gwt.junit.client.impl.GWTRunner.$runTest(GWTRunner.java:302)
at com.google.gwt.junit.client.impl.GWTRunner.$doRunTest(GWTRunner.java:235)
at com.google.gwt.junit.client.impl.GWTRunner$TestBlockListener.$onSuccess(GWTRunner.java:106)
at com.google.gwt.junit.client.impl.GWTRunner$TestBlockListener.onSuccess(GWTRunner.java:100)
at com.google.gwt.user.client.rpc.impl.RequestCallbackAdapter.$onResponseReceived(RequestCallbackAdapter.java:232)
at com.google.gwt.http.client.Request.$fireOnResponseReceived(Request.java:250)
at com.google.gwt.http.client.RequestBuilder$1.onReadyStateChange(RequestBuilder.java:412)
at Unknown.anonymous(XMLHttpRequest.java:329)
at com.google.gwt.core.client.impl.Impl.apply(Impl.java:309)
at com.google.gwt.core.client.impl.Impl.entry0(Impl.java:368)
at Unknown.anonymous(Impl.java:78)
The alternative that I had come up with so far was that this was supposed to have jsni and a //J2CL_ONLY, but that didn't feel right, given the "GWT" comment.
Doh, got it, I was half right at least. -generateJsInteropExports is indeed assumed, but the method in GetArgumentsHelper already has @JsMethod.
Okay, back to trying to generate deps automatically - only 1100 lines of generated bzl instead of 500 handwritten ones so far!
In working on a proposed patch to #19 I was hoping to pick up not just "here's a working pom", but this original work of "can this code be compiled by a given GWT version". Unfortunately, tests might be in rougher shape than before.
The one CI task that can run in the current code invokes a single sh script:
jsinterop-base/.github/workflows/ci.yaml
Line 80 in 92d499b
Which in turn has a single line:
Line 16 in 92d499b
There is another command that will try to run if the file is present:
jsinterop-base/.github/workflows/ci.yaml
Lines 84 to 86 in 92d499b
But it doesn't appear that this samples file has ever existed in this repo.
Crucially, to this existing github issue, the following "build test" fails:
bazel build javatests/...
As a result, we naturally can't run the tests.
Can you help me to get the J2CL side of tests running, and I'll see about some validation to ensure that a given build with GWT? Bazel support for ingesting maven dependencies looks to have improved a bit since the last time I tried to attack this.
But it doesn't appear that this samples file has ever existed in this repo.
All our repositories clone a central configuration for various things like copybara, bazelrc, bazelversion, CI configurations etc for simplicity (i.e. there is a single source of truth which simplifies bunch of things). That's where build_test_samples is coming from and why it only runs if it exists.
the following "build test" fails
The tests were never properly ported to open-source. I will try to take a look tomorrow.
Hmm, it uses test suites and also depends truth. It will not be trivial to make them work. Rest assured J2CL part is tested internally :) GWT is also tested interally but not with Maven setup and also not the latest open-source GWT changes so we have the big gap there.
For GWT testing do you need to deal with Bazel at all? Can't you add something like gwt_build_test.sh and call it from rgw build_test.sh or build_test_samples.sh?
Rest assured J2CL part is tested internally :)
Of course, but open source you can only look at...
GWT is also tested interally but not with Maven setup and also not the latest open-source GWT changes so we have the big gap there.
Maven isn't really the important part so, much as "are the required dependencies there". According to the bug that prompted the pom change, old GWT (anything older than last week, going back to 2008 or so) can't read those annotations without sources, so jspecify need both the source jar and the .gwt.xml file. How did that pass internal tests without either a patch to com.google.gwt.dev.javac.BinaryTypeReferenceRestrictionsChecker.BinaryTypeReferenceVisitor (which it seems from the Guava comment never happened) or these things? Or did I misunderstand what "GWT is tested" means in this context?
For GWT testing do you need to deal with Bazel at all? Can't you add something like gwt_build_test.sh and call it from rgw build_test.sh or build_test_samples.sh?
I can almost certainly hack something together that rearranges things enough to test GWT - it wouldnt be a test of maven (do I test the pom-base with custom templating code? Or do I first try to land changes to j2cl to modify the maven deploy so it doesn't deploy, but installs locally? etc), just some hacked way to run the tests. And this still leads back to - if J2CL is the way forward in github.com/google repos, but can't run tests, is it the way forward, or should we be better off making the GWT pathways better, and letting the j2cl routes be the hacky ones...?
I don't want to suggest I get a real vote here, just to be sure we're on the same page - Google is clearly running these repos, and it isn't up to the rest of us how this goes. The flip side is true too - Google has the only authority or power to make decisions. Delegating to the community to fix for GWT what can't be fixed for J2CL means we have obligations (but no authority). I don't want to beat a dead horse here...
The GWT testing internally is effectively from source and via auto-generated gwt.xml files (it is essentially same as j2cl_library but it is a gwt_library). It doesn't involve any poms etc. So as a result it won't reproduce the problems like missing dependency on POM or gwt.xml doesn't include some source.
To cover that, you need something in open-source crafted with what is used by classic GWT user in open-source. (Note that we don't really have any benefit from that and or any maven releases for the matter other than trying to be helpful for the folks outside. GWT support is not the main goal of these projects)
I probably stated this earlier but the reason we manage to keep j2cl and all these different repos available to open-source is because they replicate internal infra which almosts free for us. That's double edge sword when there is a gap (e.g. test suite support), we need to invest to fill in that gap but can't always prioritize such work.
But going back to main topic; running j2cl_test in open-source is not relevant for the problem we hit in the release. For testing with GWT; even a shell script that we can run manually during the release process to smoke test our release would be good starting point to avoid what happened in the previous release and probably best bang for the buck.