Integrate intellij scala aspect integration testing into rules_scala #309

greggdonovan · 2017-10-17T00:58:56Z

Related to #308

As we want to test multiple versions of the intellij plugin (master and a release tag) we are managing a manual clone of intellij instead of using a git submodule. I'm not sure how to get submodules to handle a test matrix of intellij versions reasonably.

bazel-io · 2017-10-17T00:58:58Z

Can one of the admins verify this patch?

googlebot · 2017-10-17T00:58:59Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). 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.

googlebot · 2017-10-17T01:00:15Z

CLAs look good, thanks!

johnynek · 2017-10-17T01:24:31Z

test this please

greggdonovan · 2017-10-17T02:18:28Z

Sorry -- the git clone failed. PR updated.

I'll try setting up my own travis account to iterate on this tomorrow.

ittaiz · 2017-10-17T03:26:59Z

Looks good now!
I looked at only one cell in the matrix and it's currently broken because our integration is broken :)
I think we'll need to keep this PR open until we have another PR which branched out of this branch and fixes the current tests.
Thanks Gregg!

kevingessner · 2017-10-17T14:22:44Z

test_run.sh

+    cd intellij && git fetch && git pull
+  fi
+  git checkout "${intellij_git_tag}"
+  bazel test --test_output=errors --incompatible_disallow_set_constructor=false --override_repository io_bazel_rules_scala=${rules_scala_dir} //aspect/testing/tests/src/com/google/idea/blaze/aspect/scala/...


you should quote ${rules_scala_dir} in case it has whitespace

Good idea, I'll update the PR.

kevingessner · 2017-10-17T14:24:10Z

test_run.sh

+  intellij_git_tag=$1
+  rules_scala_dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+
+  if [[ "$runner" == "run_test_ci" ]]; then


this feels brittle. can you detect CI another way? maybe test -d intellij?

It doesn't rely on any Travis specific env variables, ci is passed to the script and then runner is set based upon that value so I can't see a situation where it would break nor be able to be handled across multiple ci environments.

@kevingessner How about:

if [[ "$1" == "ci" ]]; then

That is one fewer level of indirection, at least.

"brittle" in that, if someone changes the name of the run_test_ci runner function, this will not error but will start behaving badly -- it'd be better if this was a reference to the function that was safe against changes.

oh yeah, missed that. thanks!
I think @greggdonovan's latest is better.

@greggdonovan can you change the above to if [[ "$1" == "ci" ]]; then like you suggested?

ittaiz · 2017-10-17T16:16:07Z

Why is this brittle? That's how the script works

…

On Tue, Oct 17, 2017 at 5:25 PM Kevin Gessner ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test_run.sh <#309 (comment)> : > @@ -525,6 +525,23 @@ revert_change() { mv $1/$2.bak $1/$2 } +test_intellij_aspect() { + intellij_git_tag=$1 + rules_scala_dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd ) + + if [[ "$runner" == "run_test_ci" ]]; then this feels brittle. can you detect CI another way? maybe test -d intellij? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#309 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFytaxUKcBZWfH6QbtL6HNMLSnU45ks5stLjigaJpZM4P7b8t> .

johnynek · 2017-11-09T00:41:58Z

@greggdonovan I want to merge this, but it is still red on travis. Any ideas on how to get it green?

greggdonovan · 2017-11-09T01:57:07Z

@johnynek Thanks! It seems like we have to understand the exact nature of the failures and resolve them to make this green.

For example, testScalaLibrary expects both a JAR and an ijar but the test failure claims we're only supplying the JAR. My understanding is that we supplied both JAR and ijar for libraries, so this is surprising to me. I'll take a deeper look and see if I can provide more context.

greggdonovan · 2017-11-12T18:58:32Z

The remaining test failure appears to be due to a transient failure to connect to Maven Central.

Otherwise, everything else is green. As long as we're happy with the ijar change and a re-run on CI passes, I think this should be OK to commit.

As far as the transient failure goes, I'll open a separate ticket to switch native.maven_jar usage to java_import_external or see if it makes sense to incorporate it into the scala_import PR.

ittaiz · 2017-11-12T19:57:34Z

Thanks! I'll take a look at the change tomorrow. The plans with respect to repository rule and java_import_external is to add a jvm_import_external which gets a string template of the rule it needs to serialize and then add a scala_import_external that unsurprisingly uses scala_import. If you want to advance it on the Bazel side that will be great. Shouldn't be a lot of work even

…

On Sun, 12 Nov 2017 at 20:58 Gregg Donovan ***@***.***> wrote: The remaining test failure appears to be due to a transient failure to connect to Maven Central. Otherwise, everything else is green. As long as we're happy with the ijar change and a re-run on CI passes, I think this should be OK to commit. As far as the transient failure goes, I'll open a separate ticket to switch native.maven_jar usage to java_import_external or see if it makes sense to incorporate it into the scala_import PR. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#309 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF0g-vULNvnFF5ayWB5X0TlPUm7Gcks5s1z_ZgaJpZM4P7b8t> .

ittaiz · 2017-11-15T13:04:02Z

test_run.sh

+  intellij_git_tag=$1
+  rules_scala_dir=$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )
+
+  if [[ "$runner" == "run_test_ci" ]]; then


@greggdonovan can you change the above to if [[ "$1" == "ci" ]]; then like you suggested?

ittaiz · 2017-11-15T13:04:44Z

test_run.sh

@@ -604,3 +621,5 @@ $runner test_scala_library_expect_failure_on_missing_direct_java
 $runner bazel run test:test_scala_proto_server
 $runner test_scala_library_expect_failure_on_missing_direct_deps_warn_mode_java
 $runner test_scala_library_expect_better_failure_message_on_missing_transitive_dependency_labels_from_other_jvm_rules
+$runner test_intellij_aspect ac2bb7 # TODO(https://github.com/bazelbuild/intellij/issues/146): replace with release tag when fixed.


should we remove this? IIRC intellij team said they're not going to have tags any time soon

ittaiz · 2017-11-15T13:09:15Z

scala/scala.bzl

-def _format_full_jars_for_intellij_plugin(full_jars):
-    return [struct (class_jar = jar, ijar = None) for jar in full_jars]
+def _format_full_jars_for_intellij_plugin(full_jars, ijar):
+    return [struct (class_jar = jar, ijar = ijar) for jar in full_jars]


@chaoren what does the plugin do with the ijar field?
I set it to None since I didn't see any drawbacks from setting it to None and on the other hand it becomes a bit more tricky to match the java-code ijar to the java-code full jar and the scala-code ijar to the scala-code full jar. It's not rocket science but a bit more cumbersome than I want and without any value I can see.

@greggdonovan the above implementation might be a problem since it passes the scala-code ijar for the java-code full jar. Depending on @chaoren's response we'll need to:

Change the intellij tests to expect None (if he agrees we don't need it).

Change our code to pass the right tuples so that we pass the right ijar to the full jar (if he surfaces reasons to using the ijar).

btw, if @chaoren says this can be ignored we can merge this PR and later remove it once he fixes the test

We actually just use the class_jar if the ijar is not available, so you might see some performance hits.

What performance hit? In loading bigger jars to memory?
@greggdonovan this sounds like the current change pleases the test but will break the actual production

Yeah pretty much. class_jar can always replace ijar, except it's much larger.

I see.
@greggdonovan do you want to take a crack at passing the matching ijar to the relevant full jar?

ittaiz · 2017-11-15T21:10:15Z

@chaoren: "It's possible to output multiple jars now?"- not quite, we output one scala jar and one java jar (if both exist). If you're asking on whether you can remove the single support from your bzl then I'm not sure if other scala rules still use that struct.

…

On Wed, 15 Nov 2017 at 22:07 Chaoren Lin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In scala/scala.bzl <#309 (comment)> : > @@ -595,8 +595,8 @@ def _collect_jars_from_common_ctx(ctx, extra_deps = [], extra_runtime_deps = []) return struct(compile_jars = cjars, transitive_runtime_jars = transitive_rjars, jars2labels=jars2labels, transitive_compile_jars = transitive_compile_jars) -def _format_full_jars_for_intellij_plugin(full_jars): - return [struct (class_jar = jar, ijar = None) for jar in full_jars] +def _format_full_jars_for_intellij_plugin(full_jars, ijar): + return [struct (class_jar = jar, ijar = ijar) for jar in full_jars] Yeah pretty much. class_jar can always replace ijar, except it's much larger. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#309 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF3IGsezrQwSUwki6CM0-GMQU8fRQks5s20R7gaJpZM4P7b8t> .

ittaiz · 2017-11-27T19:28:10Z

@greggdonovan I think this fell off of your radar. Any chance you can carry it through?

greggdonovan · 2017-11-27T19:41:45Z

@ittaiz Yes, sorry about that! I'll take a look this week. Thanks for catching the green test not being the whole story. ;-)

Can you give me more details on the difference between the Scala JARs and the Java JARs? I'm a bit fuzzy on the distinction. Thanks!

ittaiz · 2017-11-27T20:34:48Z

Sure no problem.
Essentially a scala_library can generate either a jar for the scala sources, a jar for the java sources or both. If we supply intellij with the ijars we need to propagate both ijars and pass the full with the ijar of each language. Alternatively we can just decide to punt on this for now, change the intellij tests to not expect the ijar for the time being and continue to pass None

ittaiz · 2017-12-13T05:09:16Z

Hi,
I know you're swamped but just another (hopefully) friendly ping

greggdonovan · 2017-12-20T15:17:31Z

Good news, bad news. With the ijar .bzl changes reverted, the build passes without any runtime changes.

However, on the OSX Travis builders, we're hitting the travis.org timeout of 50 minutes with the added time for this test. The test downloads about ~800mb of IntelliJ IDEs and other artifacts and spends ~3 minutes compiling protoc, so there may not be much to do to speed it up.

I can think of a few options to work around this:

We can add rows in the build matrix just for this test, keeping us under 50 minutes.
We can kick off this test at the beginning of the build in parallel.
Just run this test periodically via a Travis cron.

ittaiz · 2017-12-20T18:01:07Z

Option 1 is to synthetically simulate multiple travis configurations? Sounds good actually. Can you try it and see how much it complicated the script

…

On Wed, 20 Dec 2017 at 17:17 Gregg Donovan ***@***.***> wrote: Good news, bad news. With the ijar .bzl changes reverted, the build passes without any runtime changes. However, on the OSX Travis builders, we're hitting the travis.org timeout of 50 minutes <https://docs.travis-ci.com/user/customizing-the-build/#Build-Timeouts> with the added time for this test. The test downloads about ~800mb of IntelliJ IDEs and other artifacts and spends ~3 minutes compiling protoc, so there may not be much to do to speed it up. I can think of a few options to work around this: - We can add rows in the build matrix just for this test, keeping us under 50 minutes. - We can kick off this test at the beginning of the build in parallel. - Just run this test periodically via a Travis cron <https://docs.travis-ci.com/user/cron-jobs/>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#309 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF9DEFZ7FsMqAtaJEHb4yS-OYKGvPks5tCSUMgaJpZM4P7b8t> .

…by manually cloning intellij. As we want to test multiple versions of the intellij plugin (master and a release tag) we are managing a manual clone of intellij instead of using a git submodule.

…e GitHub keys. See: https://travis-ci.org/bazelbuild/rules_scala/jobs/288835001#L705

…ith Bazel 0.7.0 support.

…cting.

This was changed here: bazelbuild/intellij@29b78dd.

…eded.

…the timeout - Split the test_runner into its own file. - Run the IntelliJ aspect in separate rows in the build matrix.

env vars do not appear to be analyzed semantially to parse out distinct combinations. See: https://docs.travis-ci.com/user/customizing-the-build/#Build-Matrix

greggdonovan · 2017-12-26T14:33:29Z

I think this is ready for another look, @ittaiz. The Travis build matrix now has separate rows for the IntelliJ aspect test.

ittaiz

Looks good, thanks!
Can you please remind me how come the tests now pass?

greggdonovan · 2017-12-27T02:27:25Z

Thanks, @ittaiz!

This now passes because intellij stopped expecting the ijars here.

We can revisit re-adding them to the intellij tests and scala.bzl if we feel the ijars are useful for the Intellij plugin.

ittaiz · 2017-12-27T05:08:15Z

Well done!
Important contribution in intellij scala stability :)
I think next step should probably be to add a few tests over in the intellij plugin repo for covering scala_import
The current implementation is untested with respect to the plugin.
I actually think we can simplify it somewhat.
@hmemcpy the intellij plugin now supports the option of having only java provider, right?
maybe we should remove the "intellij metadata" from scala_import and manually test it.

googlebot added the cla: no label Oct 17, 2017

greggdonovan force-pushed the intellij_integration_via_manual_clone branch from 3aa1605 to 1068af6 Compare October 17, 2017 01:00

googlebot added cla: yes and removed cla: no labels Oct 17, 2017

kevingessner reviewed Oct 17, 2017

View reviewed changes

ittaiz mentioned this pull request Oct 22, 2017

[Potentially breaking change] changed specs2 to single bind #319

Merged

greggdonovan force-pushed the intellij_integration_via_manual_clone branch from 2133722 to 50b898b Compare November 9, 2017 01:30

ittaiz suggested changes Nov 15, 2017

View reviewed changes

greggdonovan force-pushed the intellij_integration_via_manual_clone branch from 7c0c0ce to f82563b Compare December 18, 2017 19:28

greggdonovan added 2 commits December 25, 2017 21:31

Integrate intellij scala aspect integration testing into rules_scala …

d8c2513

…by manually cloning intellij. As we want to test multiple versions of the intellij plugin (master and a release tag) we are managing a manual clone of intellij instead of using a git submodule.

Use https instead of git for cloning as travis does not appeat to hav…

fee073e

…e GitHub keys. See: https://travis-ci.org/bazelbuild/rules_scala/jobs/288835001#L705

greggdonovan added 8 commits December 25, 2017 21:31

Quote rules_scala_dir in case the path contains a space.

c4d993c

Remove incompatible_disallow_set_constructor as that has been fixed w…

570c1e0

…ith Bazel 0.7.0 support.

Move the ijar to the output location that the IntelliJ plugin is expe…

703bb35

…cting.

Remove git tag test

53ae885

Make the local/ci semantics clearer.

943f96a

Set the ijar back to None to match intellij

454dfe9

This was changed here: bazelbuild/intellij@29b78dd.

Checkout the intellij repo in CI with depth 1. Full history is not ne…

048c032

…eded.

Extend the Travis build matrix to allow IntelliJ aspect to run under …

917411d

…the timeout - Split the test_runner into its own file. - Run the IntelliJ aspect in separate rows in the build matrix.

greggdonovan force-pushed the intellij_integration_via_manual_clone branch from 320632b to 917411d Compare December 26, 2017 02:32

greggdonovan added 2 commits December 25, 2017 21:46

Manually specify the environment variable build matrix.

e14f130

env vars do not appear to be analyzed semantially to parse out distinct combinations. See: https://docs.travis-ci.com/user/customizing-the-build/#Build-Matrix

Only checkout intellij when needed.

3d089b8

ittaiz approved these changes Dec 26, 2017

View reviewed changes

ittaiz merged commit 7104a0e into bazel-contrib:master Dec 27, 2017

Integrate intellij scala aspect integration testing into rules_scala #309

Integrate intellij scala aspect integration testing into rules_scala #309

Conversation

greggdonovan commented Oct 17, 2017

bazel-io commented Oct 17, 2017

googlebot commented Oct 17, 2017

googlebot commented Oct 17, 2017

johnynek commented Oct 17, 2017

greggdonovan commented Oct 17, 2017

ittaiz commented Oct 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Oct 17, 2017 via email

johnynek commented Nov 9, 2017

greggdonovan commented Nov 9, 2017

greggdonovan commented Nov 12, 2017

ittaiz commented Nov 12, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ittaiz commented Nov 15, 2017 via email

ittaiz commented Nov 27, 2017

greggdonovan commented Nov 27, 2017

ittaiz commented Nov 27, 2017

ittaiz commented Dec 13, 2017

greggdonovan commented Dec 20, 2017

ittaiz commented Dec 20, 2017 via email

greggdonovan commented Dec 26, 2017

ittaiz left a comment

Choose a reason for hiding this comment

greggdonovan commented Dec 27, 2017

ittaiz commented Dec 27, 2017