Don't rely on return value of ::GirlFriday::WorkQueue.immediate #1027
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change
Note: The bug and the fix here are related to Ruby's implicit return. A Ruby method always returns whatever its last line evaluates to.
The CI break happens on Ruby 3.0.0 builds, and as will be explained, is caused by a change in
Module#alias_method
.In Ruby < 3.x,
alias_method
returns the class object.https://ruby-doc.org/core-2.7.1/Module.html#method-i-alias_method
Starting in Ruby 3.x,
alias_method
returns its first argument.https://ruby-doc.org/core-3.0.0/Module.html#method-i-alias_method
So in Ruby 3.x:
Our code doesn't directly use
alias_method
. It usesGirlFriday::WorkQueue.immediate!
which does usealias_method
.https://github.com/mperham/girl_friday/blob/01c9e725bbba4d338d8dbda90f31ca40a83d3581/lib/girl_friday/work_queue.rb#L28-L31
Because of implicit return, this returned
GirlFriday::WorkQueue
until Ruby 3.0 where it now returns:<<
.The fix
The original code used the return value to mock the code under test so it would use the modified class. This is unnecessary because
immediate!
modifies the global class object itself, not an instance of the class. The modified class will be used without needing the mock. (If the mock were needed, you would useGirlFriday::WorkQueue
directly, rather than taking the return fromimmediate!
.)Notes
This error reproduces with some seed values for rspec and not with others. I was able to reproduce locally by using the seed value from the failing CI build. It also reproduces by isolating the girl_friday tests:
bundle exec rspec spec/rollbar/delay/girl_friday_spec.rb
So we know that when the specs ran in some order the error was masked, but I don't have insight into what specifically masked the error.
Type of change
Related issues
ch80862
Development
Code review