Fix support for JRuby 9.2.0.0#1409
Conversation
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
I understand the need for this (and that's why I'm clicking approve), but I have some concerns about the future of this testing.
At this point JRuby 9.2.0.0 is more than two years old. If library developers already normally don't even test on JRuby, they definitely will not test on old JRuby. This makes my crystal ball (ruby ball?) predict other CI failures due to bugs that may have been fixed in newer JRuby versions.
When that happens, we may need to revisit this approach.
|
|
||
| responses = responses.to_a # Force resolution of lazy enumerator |
There was a problem hiding this comment.
May be worth commenting here that this was causing us trouble with older JRuby versions (as otherwise it may not be entirely clear that this is very deliberate)
There was a problem hiding this comment.
Added an explanation there.
| - &config-jruby-9_2_0 # Oldest supported version | ||
| <<: *filters_all_branches_and_tags | ||
| ruby_version: 'jruby-9.2.0' | ||
| image: marcotc/docker-library:ddtrace_rb_jruby_9_2_0 |
There was a problem hiding this comment.
Perhaps call this jruby-9_2_0_0 everywhere, just to match how JRuby sets its versions?
There was a problem hiding this comment.
I like the naming suggestion, performed both.
| - test-3.0 | ||
| - test-3.0-jit | ||
| - test-jruby-9.2.0 | ||
| - test-jruby-9.2 |
There was a problem hiding this comment.
Should we rename this one jruby-9_2-latest or something to better distinguish from the other?
There was a problem hiding this comment.
I like the naming suggestion, performed both.
| # Force resolution of lazy enumerator. | ||
| # | ||
| # The "correct" method to call here would be `#force`, | ||
| # as this method was created to force the eager loading | ||
| # of a lazy enumerator. | ||
| # | ||
| # Unfortunately, JRuby < 9.2.9.0 erroneously eagerly loads | ||
| # the lazy Enumerator during intermediate steps. | ||
| # This forces us to use `#to_a`, as this method works for both | ||
| # lazy and regular Enumerators. | ||
| # Using `#to_a` can mask the fact that we expect a lazy | ||
| # Enumerator. | ||
| responses = responses.to_a |
There was a problem hiding this comment.
Interestingly enough, this was incorrect in TruffleRuby as well, lol: truffleruby/truffleruby#2273 . Seems like there may be gaps in https://github.com/ruby/spec around this.
There was a problem hiding this comment.
I think that's accurate, as the JRuby repo got an update to their lazy enum specs, copied from ruby/spec, around the same time the bug was fixed.
In #1409 we added JRuby 9.2.0.0 explicitly to CI, because there was a behavior change between <= 9.2.8.0 and >= 9.2.9.0 and we wanted to support customers running on either side of the behavior change. Keeping JRuby 9.2.0.0 working is non-trivial work, since it's missing hundreds of fixes and changes from the 9.2 branch. We already had to change things to acommodate it in #1434, and while investigating msgpack/msgpack-ruby#239 I found YET another issue where 9.2.0.0 is broken but later releases are fine. Thus, since the only place in our test suite where we actually test for different behavior is related to #1409, let's replace 9.2.0.0 in our test suite with 9.2.8.0. Hopefully 9.2.8.0 is in a bit of a better shape, and we'll avoid needing to chase down any more 9.2.0.0-specific bugs.
2 participants
We advertise as supporting JRuby 9.2.0.0, which is the most recent major JRuby release, supporting Ruby 2.5 syntax.
But our existing test suite only runs on JRuby 9.2.11.1.
This causes one issue to come up recently around a bug that was fixing in 9.2.9.0, but affected our 9.2 supported versions older than that.
This PR addresses that issue, regarding
Enumerator::Lazy#slice_beforereturning an eager Enumerator instead of a lazy oh, as well as adding JRuby 9.2.0.0 to CI, given it's the oldest supported JRuby version.The PR keeps JRuby 9.2.11.1 in CI, as there have been significant behaviour changes since JRuby 9.2.0.0 that warrant a distinct test suite run.