Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use updated JRubies in CircleCI #2418

Merged
merged 3 commits into from
Nov 24, 2022
Merged

Use updated JRubies in CircleCI #2418

merged 3 commits into from
Nov 24, 2022

Conversation

lloeki
Copy link
Member

@lloeki lloeki commented Nov 23, 2022

What does this PR do?

Use JRubies built from #2412

How to test the change?

CI shoudl be green

@lloeki lloeki requested a review from a team November 23, 2022 20:52
@lloeki
Copy link
Member Author

lloeki commented Nov 23, 2022

Bah, a commit went missing during rebase,

@lloeki
Copy link
Member Author

lloeki commented Nov 24, 2022

@marcotc looks like we might have legit failures on JRuby 9.3.9.0, I threw a retry at it and the same expectations failed.

@ivoanjo
Copy link
Member

ivoanjo commented Nov 24, 2022

The spec that failed is a weird one because different Rubies have different behaviors:

it 'reports errors only once', if: (RUBY_VERSION < '2.6.0' || PlatformHelpers.truffleruby?) do
expect(error.type).to eq('RuntimeError')
expect(error.message).to eq('first error')
expect(error.backtrace).to match(/first error \(RuntimeError\).*second error \(RuntimeError\)/m)
# Expect 2 "first-class" exception lines: 'first error' and 'second error'.
expect(error.backtrace.each_line.reject { |l| l.start_with?("\tfrom") }).to have(2).items
end
it 'reports errors only once', if: (RUBY_VERSION >= '2.6.0' && PlatformHelpers.mri?) do
expect(error.type).to eq('ArgumentError')
expect(error.message).to eq('circular causes')
expect(error.backtrace).to match(/circular causes \(ArgumentError\).*first error \(RuntimeError\)/m)
# Expect 2 "first-class" exception lines: 'circular causes' and 'first error'.
# Ruby doesn't report 'second error' as it was never successfully set as the cause of 'first error'.
expect(error.backtrace.each_line.reject { |l| l.start_with?("\tfrom") }).to have(2).items
end
it 'reports errors only once', if: (RUBY_VERSION >= '2.6.0' && PlatformHelpers.jruby?) do
expect(error.type).to eq('RuntimeError')
expect(error.message).to eq('circular causes')
expect(error.backtrace)
.to match(/circular causes \(RuntimeError\).*first error \(RuntimeError\)/m)
# Expect 3 "first-class" exception lines: 'circular causes', 'first error' and 'second error'.
expect(error.backtrace.each_line.reject { |l| l.start_with?("\tfrom") }).to have(3).items
end
end

I guess JRuby has now adopted pre 2.6/TruffleRuby behavior?

@lloeki lloeki force-pushed the use-updated-jrubies branch from 2ce9cde to 0eca071 Compare November 24, 2022 12:08
@lloeki lloeki changed the base branch from master to align-ci-cache-keys November 24, 2022 12:09
@lloeki
Copy link
Member Author

lloeki commented Nov 24, 2022

Changed base to depend on #2422

@lloeki
Copy link
Member Author

lloeki commented Nov 24, 2022

I guess JRuby has now adopted pre 2.6/TruffleRuby behavior?

In the JRuby 9.3.7.0 release notes there's a reference to jruby/jruby#7267.

Circular exception handling can cause infinite loop since 9.3.4.0

Other releases (9.3.6.0, 9.3.9.0) don't seem to have reference to anything possibly related.

@lloeki lloeki mentioned this pull request Nov 24, 2022
Base automatically changed from align-ci-cache-keys to master November 24, 2022 14:32
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM. Rubocop has a line-too-long complaint. Sigh, I hate this Rubocop rule -- you add one more character to a PR that worked and Rubocop breaks everything :(

@@ -130,7 +130,7 @@ def call
end
end

it 'reports errors only once', if: (RUBY_VERSION < '2.6.0' || PlatformHelpers.truffleruby?) do
it 'reports errors only once', if: (RUBY_VERSION < '2.6.0' || PlatformHelpers.truffleruby? || PlatformHelpers.jruby? && RUBY_ENGINE_VERSION >= '9.3.7.0') do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding some parenthesis so people don't have to wonder about precedence ;)

Suggested change
it 'reports errors only once', if: (RUBY_VERSION < '2.6.0' || PlatformHelpers.truffleruby? || PlatformHelpers.jruby? && RUBY_ENGINE_VERSION >= '9.3.7.0') do
it 'reports errors only once', if: (RUBY_VERSION < '2.6.0' || PlatformHelpers.truffleruby? || (PlatformHelpers.jruby? && RUBY_ENGINE_VERSION >= '9.3.7.0')) do

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add too much parentheses and Rubocop will complain :D

I would otherwise agree in general but I'm pretty sure most everyone knows the precedence between || and && so it's quire readable.

@lloeki lloeki force-pushed the use-updated-jrubies branch from ce45b43 to 32594dd Compare November 24, 2022 14:47
It seems like behaviour changed starting with JRuby 9.3.7.0.
@lloeki lloeki force-pushed the use-updated-jrubies branch from 32594dd to 978fee2 Compare November 24, 2022 14:49
@lloeki lloeki merged commit f5751d5 into master Nov 24, 2022
@lloeki lloeki deleted the use-updated-jrubies branch November 24, 2022 15:09
@github-actions github-actions bot added this to the 1.7.0 milestone Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants