-
-
Notifications
You must be signed in to change notification settings - Fork 358
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
RSpec mocks 3.12 breaks test suites due to kwargs in incomprehensible manner #1499
Comments
I'm very confused by those messages, they are not at all what I would expect from an
We do use |
I'm having similar issue, but not on 3.12. It was when I was updating to rspec-mocks 3.11.2 from 3.11.1 ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [x86_64-darwin21] Example of failing expectation: expect(described_class.tracker).to receive(:track).with(
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
) Failure output:
When trying to debug the above, I switched to the expect(described_class.tracker).to receive(:track) do |args|
expected_args = {
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
}
# I used .eq() and .match() and both seem to work and pass.
expect(args).to match(expected_args)
end |
Ok, updated rspec-mocks to 3.12.0 (had to bump rspec, rspec-core, rspec-expectations, etc, to 3.12.0 first) The syntax to call this # NOTE: this example passes when I converted it to the block-form of .receive(...) from .receive(...).with(...)
it do
expect(described_class.tracker).to receive(:track) do |args|
expected_args = {
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
}
expect(args).to match(expected_args)
end
described_class.track(user: user, event: event, properties: properties, timestamp: timestamp)
end I have not converted all of my examples in this file to use the block form of This failure is for this example: it do
expect(described_class.tracker).to receive(:track).with(
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
)
described_class.track(user: user, event: event, properties: properties, timestamp: timestamp)
end
That does not seem like a different error under rspec + rspec-mocks 3.12.0, which makes me think it was introduced as part of rspec-mocks 3.11.2. |
An obvious difference is |
Nope, same failure when changing the call to it do
expect(described_class.tracker).to receive(:track).with(
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
)
described_class.track(user: user, event: event, properties: properties.merge(common_event_properties), timestamp: timestamp)
end
Update Converting this failing spec example to the block-form of it do
expect(described_class.tracker).to receive(:track) do |args|
expected_args = {
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
}
expect(args).to match(expected_args)
end
described_class.track(user: user, event: event, properties: properties.merge(common_event_properties), timestamp: timestamp)
end |
By the way, @pirj, I appreciate your quick replies here. I was simply commenting initially help build a context of the possible bug that the original poster was seeing. I was hoping it may be enough to realize what recent change may have caused this. Thanks for sticking with it. |
I'd rather take the I understand that the block form doesn't have this failure, but it doesn't help much to track the problem. My initial assumption was that on the code you pass it in as a hash, but pass kwargs to |
Changes to rspec-mocks from 3.11.2 -> 3.12.0 that seem related, but I've not completely understood them yet: lib/rspec/mocks/argument_list_matcher.rb: v3.11.1...v3.12.0#diff-ae171e2be1f799a6704fcbadb353721dda4d078185aa265671e07b14594c72e5L49-R74 lib/rspec/mocks/error_generator.rb: v3.11.1...v3.12.0#diff-5b28aeeb36e96d3d3567364a3c0dff29001eae3a81bce2752e29b9d3bca04fbeR271-R281 lib/rspec/mocks/matchers/receive.rb: v3.11.1...v3.12.0#diff-8efd45e556df39ba1efa67c1086d56aba98e46a6a4fb31e9d9246505caf4451cL65-R65 lib/rspec/mocks/message_expectation.rb: v3.11.1...v3.12.0#diff-afc11368b62c27a5bd155b766ec9cf4e96a9331d073b6123a9bce36a270de640L142-R810 lib/rspec/mocks/method_double.rb: v3.11.1...v3.12.0#diff-df0ec6501cb8ef91a9806b4a90bb265b7d4affbbf4994bde7033f02b9ca92827R66-R82 lib/rspec/mocks/method_reference.rb: v3.11.1...v3.12.0#diff-68b5edb44e7a864abc3329011e7186acd8ec5895890682dda8e6f986af1584dcL188-R204 There may be more. |
@voxik - expect(Guard::LiveReload::Reactor).to receive(:new).with(
+ expect(Guard::LiveReload::Reactor).to receive(:new).with({ As
|
The same seem to be the case for your other PR: - .with(groups: [:frontend], plugins: [])
+ .with({ groups: [:frontend], plugins: [] }) session.interactor_scopes = scopes and scopes, = session.convert_scopes([]) If e.g.
I might be mistaken, but an option is to pass them there as such: Reactor.new(**options) but that won't work with If this is the culprit, I'm uncertain if we should check if the stub's method signature has kwargs, and if it doesn't, skip checking for hash vs kwargs difference on Ruby 3+. |
@cfurrow It's hard to say that there is anything wrong with |
This repo demonstrates the failure exactly: https://github.com/cfurrow/rspec-mocks-fail-example
|
I'm trying to setup a branch on https://github.com/cfurrow/rspec-mocks-fail-example that changes rspec-mocks back to v3.11.1, but doing so has not given me the same failure I saw in my main repo. It fails the same way that rspec-mocks 3.12.0 is failing. https://github.com/cfurrow/rspec-mocks-fail-example/compare/rspec-mocks-3.11.1?expand=1 I still only get one passing and one failing spec example, like I do in v3.12.0. Hmm. |
Surrounding the args in Puzzling. it 'does not fail when I use .receive().with()' do
- expect(described_class.tracker).to receive(:track).with(
+ expect(described_class.tracker).to receive(:track).with({
user_id: user.id,
event: event,
properties: properties.merge(common_event_properties),
timestamp: timestamp,
anonymous_id: nil
- )
+ }) Our CI environment is still setup with rspec 3.11.0, rspec-mocks 3.11.1, rspec-expectations 3.11.1, etc, and is not showing failures for the specs I've shown in this issue. |
The original error and your error are different, RSpec is providing a diff but it seems the order has changed for you, where as the original error in this issue provides no details at all as to whats gone wrong |
Same for your example as for original one, you pass an options hash, and Ruby doesn't unfold that to kwargs. The following fixes it: - tracker.track(payload)
+ tracker.track(**payload) I'm not sure why you didn't see the "(keyword arguments)" vs "(options hash)" in the failure message, but I see it on Ruby 3.1.0. If you're still inclined to think that |
(re opened because the original poster has not got a sensible error output, I don't think RSpec is at fault see my earlier reproduction but I'd like to hear from the orignal poster again before closing) |
The frustrating thing is, the specs were passing before the gem version changes. Below is a diff of the extent of my PR changes that was primarily made to bump rspec-rails from 6.0.0 to 6.0.1, and the specs that I've outlined above were passing, but now are not. The specs have not been modified, and continue to pass on other branches within our CI environment. diff --git a/Gemfile.lock b/Gemfile.lock
- rspec (3.11.0)
- rspec-core (~> 3.11.0)
- rspec-expectations (~> 3.11.0)
- rspec-mocks (~> 3.11.0)
- rspec-core (3.11.0)
- rspec-support (~> 3.11.0)
- rspec-expectations (3.11.1)
+ rspec (3.12.0)
+ rspec-core (~> 3.12.0)
+ rspec-expectations (~> 3.12.0)
+ rspec-mocks (~> 3.12.0)
+ rspec-core (3.12.0)
+ rspec-support (~> 3.12.0)
+ rspec-expectations (3.12.0)
- rspec-support (~> 3.11.0)
- rspec-mocks (3.11.1)
+ rspec-support (~> 3.12.0)
+ rspec-mocks (3.12.0)
- rspec-support (~> 3.11.0)
- rspec-rails (6.0.0)
+ rspec-support (~> 3.12.0)
+ rspec-rails (6.0.1)
- rspec-support (3.11.1)
+ rspec-support (3.12.0)
- zeitwerk (2.6.1)
+ zeitwerk (2.6.5) So, with this ^ context, my assumption was something in the realm of rspec, or rspec-* seems to have caused a previously "fine" behavior and code to now come back as failing. That's when I found this current issue that seemed like a possible lead. I understand that it seems to be some issue between passing a Hash or kwargs, but my point is that my code has not changed. The code may be passing kwargs when it should be passing a Hash, or it should be splatting it Something seems to have altered beyond my current understanding in one, or more, rspec gems. My guess was it's centered around mocks because we're dealing with |
Sorry, I have not received notifications from GH up until now 😖 Well, the Guard test suite is doing (or did, there were some big changes) heavy mocking and this is where it actually fails: https://github.com/guard/guard/blob/v2.18.0/spec/spec_helper.rb#L229-L231 Yes, there is Please note that I am not related to Guard project in any way. I was just trying to fix Fedora package, which suddenly started to fail with RSpec update. Therefore I am not familiar with the test suite to understand on the first look that |
IOW one concern is that |
The guard-livereload fails here: Again, there is abort. But the issue is that something is newly using |
@cfurrow I believe it is this small change that now makes your specs fail. It is correct, as you expect ( @voxik There's an open issue about |
I still think that rspec-mocks could "do less" during the double call evaluation. Loading library once the failure is identified is probably too much (although when I suggested the preloading of Possibly the stubs should not affect the evaluation. The failure was due to kwargs, then there was an attempt to construct some error message and this should not be affected by the test doubles. Or maybe use some "less verbose mode" be default. Don't provide the details of the failure, just provide the line of failure. I.e. better to say less then to provide misinformation. |
Or just don't use |
Thanks for the note, @pirj -- that was the bit of code I'd not fully understood, but assumed it was the cause of my current frustration. I'll continue cleaning up my code to correct how we pass Hash or kwargs to I appreciate all the help and context on this. |
You can turn off verifying doubles if you want RSpec to do "less", although with projects outside your control thats more difficult.
We can't provide diffs without Given that it does seem that the weird ENV stubbing is actually to blame here, I'm going to close this now. |
Subject of the issue
I have just sent several PR to fix test suites of Guard projects:
guard/guard#986
guard/guard-livereload#194
What is problematic is that RSpec breaks the test suite in incomprehensible way. E.g. the guard test suite failed like this:
which does not even look like failure, except the return code. Please note that there is not
COLUMNS
string in the whole guard code base.The guard-livereload was failing in following way:
This is frustrating, because if you check the fixes, they really fix the kwargs. One might say that Guard is using some unorthodox ways of stubbing some calls. But OTOH, it is not nice that it really is not obvious what is going on. And surprisingly, in both cases the error is related to loading/using
pp
.I don't know if there is anything actionable here. Maybe the
pp
could be preloaded or it would not need to be used at all. Maybe there is a way to release changes like this when they are more polished. Maybe you can distill some bits of this report into separate tickets. I leave it to your consideration and will not be super sad if you just nod and close the ticket :)The text was updated successfully, but these errors were encountered: