Disable failing tests #494

ckoparkar · 2018-01-26T15:04:55Z

These failures are unrelated to the nREPL itself.

CLJS doesn't build on JDK9 because it depends on a package deprecated in JDK9 (javax.xml.bind). CLJS-2377 tracks it. It has been fixed on master, but not yet released. After that, we can enable CLJS-1.9 tests on JDK9, but not the others.
CLJS-1.9 also doesn't build on JDK7 anymore ? I think this is because of a dependency on the closure-compiler which requires JDK8 or higher (https://github.com/google/closure-compiler/wiki/Releases#september-10-2017-v20170910)

/cc @gonewest818

Before submitting a PR make sure the following things have been done:

The commits are consistent with our contribution guidelines
You've added tests to cover your change(s)
All tests are passing
The new code is not generating reflection warnings
You've updated the readme (if adding/changing middleware)

Thanks!

ckoparkar · 2018-01-26T15:15:39Z

All tests pass! Yay!

bbatsov · 2018-01-26T15:31:15Z

.travis.yml

@@ -44,6 +44,16 @@ matrix:
      jdk: oraclejdk8
      after_success: bash <(curl -s https://codecov.io/bash) -f target/coverage/codecov.json

+  exclude:
+    - env: VERSION=1.9 TARGET=test-cljs


I think you should also put the explanations here, so people would be able to quickly understand why've setup the build this way.

Good point. Added comments there.

gonewest818 · 2018-01-26T16:11:02Z

.travis.yml

+  exclude:
+    # CLJS doesn't build on JDK9 because it depends on a package (javax.xml.bind)
+    # deprecated in JDK9. See (CLJS-2377).
+    # After a CLJS release, the VERSION=1.9 build can be re-enabled.


In bhauman/lein-figwheel#612 another way to handle this was discussed. Conditionally adding :jvm-opts ["--add-modules" "java.xml.bind"] to the build.

I considered doing this. So we could add something like:

:jvm-opts ~(try (do (Class/forName "javax.xml.bind.DatatypeConverter") []) (catch ClassNotFoundException e ["--add-modules" "java.xml.bind"]))

But this seemed like too much of a hack to get the tests working. Should we take this path ?

On second thought, I should've mentioned this in the PR as well. Thanks for bringing it up.

No worries, I just wanted to put it out there. I don't have a strong opinion either way.

However, that said, another equally hacky approach would be to detect the java version in the Makefile and use lein update-in to inject the :jvm-opts as needed for cljs.

The Makefile has been the place where other transient toggling is being done, for instance filtering out some cloverage tests, and turning off mranderson.

Ah, that's true. One advantage of this would be that all toggling would be in one place.

But would doing this in Clojure be a bit safer than grepping the version there ? (I have to admit that I have minimal experience with Makefiles, so I might be totally wrong. I used Class/forName because of Alex Miller's comment on the same figwheel issue.)

If we're not disabling the tests and adding java-opts instead, I'm leaning toward adding it in the project.clj file and using Class/forName. But I'm fine with the Makefile version too.

Well, for instance in orchard there's this
https://github.com/clojure-emacs/orchard/blob/master/Makefile#L5-L8

which detects Java 9 and constructs a test selector :java9.0 which is then used to filter certain test cases based on metadata (e.g. lein with-profile +1.9 test :java9.0). The usage would be different in this case, but we're already taking a similar risk elsewhere.

Because the newer google/closure-compiler requires JDK 8 or higher (https://github.com/google/closure-compiler/wiki/Releases#september-10-2017-v20170910).

ckoparkar · 2018-01-27T00:42:08Z

Makefile

 	lein with-profile +$(VERSION),+test-cljs test
+endif



Oh well. Let's go ahead with the Makefile version then. Does this look ok ? I couldn't figure out how to use both with-profile and update-in at the same time. I'm not using Clojure/Leiningen for a while now. And I ran out of make-fu :P . Could you please help with this ?

Does that work? I've never tried expressing a recipe that way. I would have done the conditional test in bash syntax, ie. all the logic completely inside the recipe.

test-cljs: .source-deps if [ "$(JAVA_VERSION)" = "9.0" ]; then \ lein with-profile +$(VERSION),+test-cljs,+jxb test; \ else \ lein with-profile +$(VERSION),+test-cljs test; \ fi

Also, now that I'm thinking about it... I wonder if it's more future-proof to name the Leiningen profile "+java9.0" [edit: even better, "+java9-modules"] rather than "+jxb"? In the event the project requires other Java 9 modules down the road then this mechanism for adding modules has a general label instead of the specific one.

while I'm at my keyboard I'll take a look at the update-in usage also.

Doing it with update-in would look something like this:

test-cljs: .source-deps if [ "$(JAVA_VERSION)" = "9.0" ]; then \ lein with-profile +$(VERSION),+test-cljs \ update-in :jvm-opts concat "[\"--add-module\" \"java.xml.bind\"]" \ -- test; \ else \ lein with-profile +$(VERSION),+test-cljs test; \ fi

going this route you don't need the :jxb profile in project.clj at all.

gonewest818 · 2018-01-27T00:49:36Z

Makefile

@@ -3,6 +3,9 @@
 VERSION ?= 1.9
 export CLOVERAGE_VERSION = 1.0.11-SNAPSHOT

+# The test-cljs target needs to be modified if working with JDK9
+JAVA_VERSION = :java$(shell lein version | cut -d " " -f 5 | cut -d "." -f 1-2)


This is going to produce a string like :java9.0 (or :java1.8, :java1.7, it's annoying that the numbering convention changed).

Suggest you simplify by dropping the ":java" part:

JAVA_VERSION = $(shell lein version | cut -d " " -f 5 | cut -d "." -f 1-2)

which will produce a number like "#.#"

ckoparkar · 2018-01-27T03:41:40Z

Makefile

@@ -13,7 +16,13 @@ test-clj: .source-deps
 	lein with-profile +$(VERSION),+test-clj test

 test-cljs: .source-deps
-	lein with-profile +$(VERSION),+test-cljs test
+	if [ "$(JAVA_VERSION)" = "9.0" ] || [ "$(JAVA_VERSION)" = "9" ]; then \


Also, on my linux machine, JAVA_VERSION is simply "9" instead of "9.0". Is it different on yours ?

On Travis it returns "9.0"

right... I was in the middle of typing the same thing. The orchard builds are all getting "9.0" on the Travis CI infrastructure running Ubuntu 14.04.

What distribution are you running?

I'm using OpenJDK on Archlinux.

Should we leave the = "9" around, just in case ?

I'm launching a Docker container with Ubuntu 16.04 and apt-get install openjdk-9-jdk to see. I assume that's what Travis CI will move to next.

root@2735a24c8c08:~# ./lein version Leiningen 2.8.1 on Java 9-internal OpenJDK 64-Bit Server VM root@2735a24c8c08:~# ./lein version | cut -d " " -f 5 9-internal root@2735a24c8c08:~# ./lein version | cut -d " " -f 5 | cut -d "." -f 1-2 9-internal

That = "9" doesn't look like it's going to work. As of right now OpenJDK 9 on Ubuntu 16.04 calls itself 9-internal and you could use that, but really we should open a ticket that we need a way to detect Oracle and OpenJDK versions and report back consistently. I'd be shocked if something better isn't available.

Hmm. Strange. Yeah, we should track this somewhere. I've used 9-internal here though. The failing Travis builds would be a good forcing function for us to modify it.

ckoparkar · 2018-01-27T03:45:24Z

Makefile

-	lein with-profile +$(VERSION),+test-cljs test
+	if [ "$(JAVA_VERSION)" = "9.0" ] || [ "$(JAVA_VERSION)" = "9" ]; then \
+            lein with-profile +$(VERSION),+test-cljs \
+                 update-in :jvm-opts concat '["--add-modules" "java.xml.bind"]' \


Thanks a lot @gonewest818! I've pretty much used your code verbatim, except for using single quotes here. Escaping the double quotes didn't work.

The jxb profile sure was ugly. This looks so much better.

The javax.xml.bind has been deprecated in JDK9, but ClojureScript depends on it (CLJS-2377).

bbatsov · 2018-01-27T16:17:07Z

Nice! I was completely fine with the original solution, but you've managed to improve it quite a bit! Very useful (and also very educational for me)!

Thanks, guys! 🙇

ckoparkar · 2018-01-27T17:55:01Z

Thanks a lot for all the help @gonewest818!

bbatsov reviewed Jan 26, 2018

View reviewed changes

gonewest818 reviewed Jan 26, 2018

View reviewed changes

gonewest818 mentioned this pull request Jan 26, 2018

Fix #497 Auto-publish to clojars #498

Merged

5 tasks

Disable CLJS-1.9 test on JDK7

7f3b06a

Because the newer google/closure-compiler requires JDK 8 or higher (https://github.com/google/closure-compiler/wiki/Releases#september-10-2017-v20170910).

ckoparkar commented Jan 27, 2018

View reviewed changes

gonewest818 reviewed Jan 27, 2018

View reviewed changes

ckoparkar commented Jan 27, 2018

View reviewed changes

Fix CLJS test failures on JDK9

722b10f

The javax.xml.bind has been deprecated in JDK9, but ClojureScript depends on it (CLJS-2377).

bbatsov merged commit 04fd79d into clojure-emacs:master Jan 27, 2018

ckoparkar mentioned this pull request Jan 27, 2018

Use a standardized method to detect JDK version #500

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable failing tests #494

Disable failing tests #494

ckoparkar commented Jan 26, 2018 •

edited

ckoparkar commented Jan 26, 2018

bbatsov Jan 26, 2018

ckoparkar Jan 26, 2018

gonewest818 Jan 26, 2018

ckoparkar Jan 26, 2018 •

edited

ckoparkar Jan 26, 2018

gonewest818 Jan 26, 2018

ckoparkar Jan 26, 2018

gonewest818 Jan 26, 2018 •

edited

ckoparkar Jan 27, 2018

gonewest818 Jan 27, 2018 •

edited

gonewest818 Jan 27, 2018

gonewest818 Jan 27, 2018

gonewest818 Jan 27, 2018

ckoparkar Jan 27, 2018

ckoparkar Jan 27, 2018

gonewest818 Jan 27, 2018

ckoparkar Jan 27, 2018

gonewest818 Jan 27, 2018

gonewest818 Jan 27, 2018

ckoparkar Jan 27, 2018 •

edited

ckoparkar Jan 27, 2018

bbatsov commented Jan 27, 2018

ckoparkar commented Jan 27, 2018

Disable failing tests #494

Disable failing tests #494

Conversation

ckoparkar commented Jan 26, 2018 • edited

ckoparkar commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckoparkar Jan 26, 2018 • edited

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

gonewest818 Jan 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gonewest818 Jan 27, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckoparkar Jan 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Jan 27, 2018

ckoparkar commented Jan 27, 2018

ckoparkar commented Jan 26, 2018 •

edited

ckoparkar Jan 26, 2018 •

edited

gonewest818 Jan 26, 2018 •

edited

gonewest818 Jan 27, 2018 •

edited

ckoparkar Jan 27, 2018 •

edited