Run tests against supported jRuby versions #4703

rrosenblum · 2017-09-05T21:48:43Z

This is related to #4686. We aren't running tests against all supported jRuby versions. It would be really nice if we could specify jruby-9.0 and jruby-9.1 to test against the latest versions. I know that we have had issues with the Travis aliases in the past. It seems like we should always run against jruby-head even if it is an allowed failure.

https://docs.travis-ci.com/user/languages/ruby/
http://rubies.travis-ci.org/

pocke · 2017-09-06T07:06:02Z

.travis.yml

+  - jruby-9.0.5.0
+  - jruby-9.1.9.0
+  - jruby-head
+  - jruby-1.7.26


I think we don't support jruby-1.7.x https://github.com/bbatsov/rubocop#compatibility

Thanks, I removed it from the list.

koic · 2017-09-06T07:36:36Z

.travis.yml

@@ -3,7 +3,10 @@ cache: bundler
 language: ruby
 dist: trusty
 rvm:
-  - jruby-9.0.1.0
+  - jruby-9.0.5.0
+  - jruby-9.1.9.0


JRuby 9.1.12 is actually available on Travis CI. For example rails/rails uses it.

https://travis-ci.org/rails/rails/jobs/271990276#L585-L586

Thanks, updated.

I went based on what is listed on http://rubies.travis-ci.org/. This list doesn't appear to be fully up to date.

That's right. I don't know when list of http://rubies.travis-ci.org/ will be updated, but as far as I know Travis CI can use the new JRuby version based on RVM update.

koic · 2017-09-06T18:09:32Z

The build error of JRuby 9.1.12.0 in this PR is probably resolved in #4708.

However, there are matrices that can not be tested until the end.
https://travis-ci.org/bbatsov/rubocop/jobs/272483545#L692

It seems that Travis CI terminated due to a timeout of 50 minutes.
https://docs.travis-ci.com/user/customizing-the-build#Build-Timeouts

Below we can see that the build of JRuby 9.1.12.0 is slow.
https://travis-ci.org/bbatsov/rubocop/builds/272483540

This problem seems to need to be considered separately from the build error.

rrosenblum · 2017-09-06T18:28:20Z

The build error of JRuby 9.1.12.0 in this PR is probably resolved in #4708.

Yes, swapping each_with_index for each fixed the problem.

koic · 2017-09-07T00:42:32Z

.travis.yml

@@ -3,7 +3,9 @@ cache: bundler
 language: ruby
 dist: trusty
 rvm:
-  - jruby-9.0.1.0
+  - jruby-9.0.5.0
+  - jruby-9.1.12.0


Thanks for the update. Just a few hours ago JRuby 9.1.13.0 was released (!) .
http://jruby.org/2017/09/06/jruby-9-1-13-0

This is already available on Travis CI. Would please you update it?

This release includes bug fixes for Enumerable#each_with_index.
jruby/jruby#4724.

bbatsov · 2017-09-08T05:12:28Z

It would be really nice if we could specify jruby-9.0 and jruby-9.1 to test against the latest versions. I know that we have had issues with the Travis aliases in the past. It seems like we should always run against jruby-head even if it is an allowed failure.

My main concern is that the test suit is already pretty slow and adding more builds to the matrix doesn't really help. Why would someone stick with 9.0 instead of upgrading to 9.1? For Rbx and JRuby we've traditionally use on CI just their latest stable version.

rrosenblum · 2017-09-08T13:54:58Z

Why would someone stick with 9.0 instead of upgrading to 9.1

I am not familiar with jRuby's release schedule or their support of major and minor versions. I assumed a jump from 9.0 to 9.1 was similar to minor version updates in Ruby. I am fine removing tests for 9.0.

Unfortunately, the jRuby test failures are coming from 9.1 and not 9.0.

rrosenblum · 2017-09-08T14:00:52Z

Travis run failure for jRuby 9.1.13.0

Error: Your application used more memory than the safety cap of 500M.
Specify -J-Xmx####M to increase it (#### = cap size in MB).
Specify -w for full java.lang.OutOfMemoryError: Java heap space stack trace

I wasn't expecting the error to be running out of memory. I am not sure what baseline memory usage for RuboCop in jRuby is, but 500M seems incredibly high.

rrosenblum · 2017-09-08T14:08:31Z

Just ran the jRuby tests locally to compare memory usage. I wound up with 2 Java processes when running bundle exec spec. One process was 750M. The other was 300M.

rrosenblum · 2017-09-13T00:49:17Z

I am having a bit of trouble getting spec/rubocop/cop/style/string_literals_spec.rb:155 autocorrects words with non-ascii chars to work with RUBYOPT="-E ASCII". The test is a little weird because were are processing unicode characters with ASCII. jRuby complains about incompatible character encoding, and I'm not really sure how to handle this. It is related to #3017, #3189, and #3311.

Does anyone have any thoughts on this?

deivid-rodriguez · 2017-09-16T11:47:11Z

@rrosenblum Having a look at this right now.

deivid-rodriguez · 2017-09-16T12:14:59Z

@rrosenblum This should get tests passing

index ff7a9eda..80e3892b 100644
--- a/lib/rubocop/cop/util.rb
+++ b/lib/rubocop/cop/util.rb
@@ -295,8 +295,7 @@ module RuboCop
       end
 
       def compatible_external_encoding_for?(src)
-        src = src.dup if RUBY_VERSION < '2.3'
-        src.force_encoding(Encoding.default_external).valid_encoding?
+        src.dup.force_encoding(Encoding.default_external).valid_encoding?
       end
 
       def to_supported_styles(enforced_style)

deivid-rodriguez · 2017-09-16T12:49:39Z

To optimize memory usage, you could instead add another check for jruby, to avoid dupping the string when not necessary.

deivid-rodriguez · 2017-09-17T09:29:32Z

To clarify my previous comment, this is the simplest patch to get this passing

diff --git a/lib/rubocop/cop/util.rb b/lib/rubocop/cop/util.rb
index ff7a9eda..88d5d85b 100644
--- a/lib/rubocop/cop/util.rb
+++ b/lib/rubocop/cop/util.rb
@@ -295,7 +295,7 @@ module RuboCop
       end
 
       def compatible_external_encoding_for?(src)
-        src = src.dup if RUBY_VERSION < '2.3'
+        src = src.dup if RUBY_VERSION < '2.3' || RUBY_ENGINE == 'jruby'
         src.force_encoding(Encoding.default_external).valid_encoding?
       end

rrosenblum · 2017-09-18T13:03:44Z

Thanks for the help @deivid-rodriguez. The tests passed locally. Hopefully the Travis build will be successful this time.

Drenmi · 2017-09-18T13:47:47Z

🎉

deivid-rodriguez · 2017-09-18T14:15:54Z

You're welcome! 😃

olleolleolle · 2017-09-18T19:44:45Z

.travis.yml

@@ -23,6 +24,7 @@ matrix:
    - rvm: jruby-9.0.1.0


The allow_failures section now contains a non-included matrix item. Remove the line - rvm: jruby-9.0.1.0

deivid-rodriguez · 2017-09-19T10:18:57Z

.travis.yml

  matrix:
    - 'TASK=spec'
    - 'TASK=ascii_spec'
    - 'TASK=internal_investigation'
 matrix:
  allow_failures:
-    - rvm: jruby-9.0.1.0
+    - rvm: jruby-9.1.13.0


Why? jruby-9.0.1.0 was temporarily moved to allow_failures because of a CI problem unrelated to RuboCop. But it's working now and I think RuboCop still officially supports jruby by making sure it's build is green...

Because jruby-9.0.1.0 is not in the list of rubies that are actually run on the CI.

I mean the "plus" part of the diff, not the removal. Why putting jruby-9.1.13.0 in allowed_failures?

Same answer. 😛 9.0.1.0 was replaced with 9.1.13.0 in the list of rubies (L:6).

Oh. I see what you mean now @deivid-rodriguez. Got hung up on the number. Yeah. Should be passing now?

I don't get it. We were enforcing jruby to pass when using 9.0.1.0, then we temporarily removed it because of a CI issue, now we upgrade to 9.1.13.0 and things are working in CI. Shouldn't it be again not present in allowed_failures?

I understand if you want to split that change to a separate commit, I guess. But the final diff here should not include that in the allowed_failures list. Unless you'd like that change as a separate PR?

Sorry, I would've spared the last comment if I had seen #4703 (comment) before 😄.

Yeah, should be passing now.

@deivid-rodriguez Good catch. I wasn't really thinking when I made the change. I agree, since we support jRuby, we should enforce green builds. The build is passing in jRuby so I will remove it from the allowed_failures section.

Follow up for rubocop#4703. JRuby 9.1.13 and 9.2.0.0-SNAPSHOT (head) are specified for Travis CI. `Dir.tmpdir` on JRuby 9.1.6 or higher returns `/tmp`. It is not the current directory. JRuby 9.1.5 or lower ==================== ```console % cd /home/vagrant && rvm use jruby-9.1.5.0 && ruby -rtmpdir -ve 'p Dir.tmpdir' Using /home/vagrant/.rvm/gems/jruby-9.1.5.0 jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 OpenJDK 64-Bit Server VM 24.121-b00 on 1.7.0_121-b00 +jit [linux-x86_64] /home/vagrant/.rvm/rubies/jruby-9.1.5.0/lib/ruby/stdlib/tmpdir.rb:40: warning: shadowing outer local variable - dir "/home/vagrant" ``` JRuby 9.1.6 or higher ===================== ```console % cd /home/vagrant && rvm use jruby-9.1.6.0 && ruby -rtmpdir -ve 'p Dir.tmpdir' Using /home/vagrant/.rvm/gems/jruby-9.1.6.0 jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 OpenJDK 64-Bit Server VM 24.121-b00 on 1.7.0_121-b00 +jit [linux-x86_64] "/tmp" ``` I think that this workaround code is unnecessary already. Related Issue ... jruby/jruby#405

Follow up for #4703. JRuby 9.1.13 and 9.2.0.0-SNAPSHOT (head) are specified for Travis CI. `Dir.tmpdir` on JRuby 9.1.6 or higher returns `/tmp`. It is not the current directory. JRuby 9.1.5 or lower ==================== ```console % cd /home/vagrant && rvm use jruby-9.1.5.0 && ruby -rtmpdir -ve 'p Dir.tmpdir' Using /home/vagrant/.rvm/gems/jruby-9.1.5.0 jruby 9.1.5.0 (2.3.1) 2016-09-07 036ce39 OpenJDK 64-Bit Server VM 24.121-b00 on 1.7.0_121-b00 +jit [linux-x86_64] /home/vagrant/.rvm/rubies/jruby-9.1.5.0/lib/ruby/stdlib/tmpdir.rb:40: warning: shadowing outer local variable - dir "/home/vagrant" ``` JRuby 9.1.6 or higher ===================== ```console % cd /home/vagrant && rvm use jruby-9.1.6.0 && ruby -rtmpdir -ve 'p Dir.tmpdir' Using /home/vagrant/.rvm/gems/jruby-9.1.6.0 jruby 9.1.6.0 (2.3.1) 2016-11-09 0150a76 OpenJDK 64-Bit Server VM 24.121-b00 on 1.7.0_121-b00 +jit [linux-x86_64] "/tmp" ``` I think that this workaround code is unnecessary already. Related Issue ... jruby/jruby#405

rrosenblum force-pushed the travis branch 2 times, most recently from 399124c to ba1d3d1 Compare September 5, 2017 21:54

rrosenblum changed the title ~~Run tests against jRuby 9.1.x and head~~ Run tests against supported jRuby versions Sep 5, 2017

pocke reviewed Sep 6, 2017

View reviewed changes

koic reviewed Sep 6, 2017

View reviewed changes

rrosenblum force-pushed the travis branch 2 times, most recently from 08650c6 to 16683b0 Compare September 6, 2017 13:29

koic mentioned this pull request Sep 6, 2017

Add new Lint/RedundantWithIndex cop #4708

Merged

11 tasks

koic reviewed Sep 7, 2017

View reviewed changes

rrosenblum force-pushed the travis branch from 01e92fa to 6c7e0b2 Compare September 7, 2017 13:07

rrosenblum force-pushed the travis branch 2 times, most recently from 00ef180 to d8c64d2 Compare September 12, 2017 17:19

This was referenced Sep 16, 2017

Remove Lint/InvalidCharacterLiteral cop #4746

Merged

Allow JRuby failures on TravisCI #4748

Merged

rrosenblum force-pushed the travis branch from d8c64d2 to 8371bfa Compare September 18, 2017 13:01

olleolleolle reviewed Sep 18, 2017

View reviewed changes

rrosenblum force-pushed the travis branch from 8371bfa to 28a512d Compare September 18, 2017 21:29

deivid-rodriguez reviewed Sep 19, 2017

View reviewed changes

rrosenblum added 2 commits September 19, 2017 08:31

Run Travis tests against more versions of jRuby

fb9f7ab

Dup strings in jRuby before modifying encoding

71d1ae5

rrosenblum force-pushed the travis branch from 28a512d to 71d1ae5 Compare September 19, 2017 12:31

bbatsov merged commit 7416bab into rubocop:master Sep 24, 2017

koic mentioned this pull request Oct 10, 2017

Remove JRuby workaround for Travis CI #4852

Merged

11 tasks

rrosenblum deleted the travis branch February 26, 2018 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run tests against supported jRuby versions #4703

Run tests against supported jRuby versions #4703

rrosenblum commented Sep 5, 2017 •

edited

Loading

pocke Sep 6, 2017

rrosenblum Sep 6, 2017

koic Sep 6, 2017

rrosenblum Sep 6, 2017

koic Sep 6, 2017

koic commented Sep 6, 2017

rrosenblum commented Sep 6, 2017

koic Sep 7, 2017

koic Sep 7, 2017

bbatsov commented Sep 8, 2017

rrosenblum commented Sep 8, 2017

rrosenblum commented Sep 8, 2017

rrosenblum commented Sep 8, 2017

rrosenblum commented Sep 13, 2017 •

edited

Loading

deivid-rodriguez commented Sep 16, 2017

deivid-rodriguez commented Sep 16, 2017

deivid-rodriguez commented Sep 16, 2017

deivid-rodriguez commented Sep 17, 2017

rrosenblum commented Sep 18, 2017

Drenmi commented Sep 18, 2017

deivid-rodriguez commented Sep 18, 2017

olleolleolle Sep 18, 2017 •

edited

Loading

deivid-rodriguez Sep 19, 2017

Drenmi Sep 19, 2017

deivid-rodriguez Sep 19, 2017 •

edited

Loading

Drenmi Sep 19, 2017

Drenmi Sep 19, 2017

deivid-rodriguez Sep 19, 2017

deivid-rodriguez Sep 19, 2017

rrosenblum Sep 19, 2017

Run tests against supported jRuby versions #4703

Run tests against supported jRuby versions #4703

Conversation

rrosenblum commented Sep 5, 2017 • edited Loading

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

koic commented Sep 6, 2017

rrosenblum commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbatsov commented Sep 8, 2017

rrosenblum commented Sep 8, 2017

rrosenblum commented Sep 8, 2017

rrosenblum commented Sep 8, 2017

rrosenblum commented Sep 13, 2017 • edited Loading

deivid-rodriguez commented Sep 16, 2017

deivid-rodriguez commented Sep 16, 2017

deivid-rodriguez commented Sep 16, 2017

deivid-rodriguez commented Sep 17, 2017

rrosenblum commented Sep 18, 2017

Drenmi commented Sep 18, 2017

deivid-rodriguez commented Sep 18, 2017

olleolleolle Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivid-rodriguez Sep 19, 2017 • edited Loading

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

rrosenblum commented Sep 5, 2017 •

edited

Loading

rrosenblum commented Sep 13, 2017 •

edited

Loading

olleolleolle Sep 18, 2017 •

edited

Loading

deivid-rodriguez Sep 19, 2017 •

edited

Loading