Specs with Mock started failing on v5.16.0

[baelter]

Don't have a lot of information yet.

But some specs using MiniTest::Mock starting failing after upgrading minitest.

Will update when I figure out what's going on.

[baelter]

Likely due to changes in argument handling in mock.rb

Seeing: ArgumentError: mocked method :my_method? expects 2 arguments, got [:arg1]

But I am calling it with one argument and one keyword argument.

[N0xFF]

How can we allow any kwargs?

In 5.15 it was possible with Hash class:

mock.expect(:my_method, 42, [Hash])

[baelter]

With this change kwargs should not be in the args array anymore:
v5.15.0...v5.16.0#diff-2a72d0121a0eb22e9f32a1ce654e5132391273007bf643b5ae68fbbc2800dea4L81
But maybe that should be fixed since it's a breaking change?

[sedubois]

We also had a broken test when updating from 5.15.0 to 5.16.0. We use MiniTest::Mock through Rails ActiveSupport assert_called_with, which does not seem to offer a way to pass kwargs separately.

[eileencodes]

@sedubois assert_called_with comes from Rails not minitest. However is a private API. I'm working on fixing it for the framework's tests but it shouldn't be consumed by applications. Because it's private I do not intend to maintain the existing method signature or behavior.

[zenspider]

I didn't realize that by adding real kwsupport I'd break people's kludged keywords... how were you doing it? I assume a hash on the end of args? Can someone provide some failing examples? This issue is not workable as-is.

[zenspider]

@eileencodes lemme know if there's anything I can do to help you on your end.

[thomthom]

I ran into this one as well. In my case it all related to testing Observer logic from the std lib;

C:/Ruby27-x64/lib/ruby/2.7.0/observer.rb:197: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
C:/projects/extension-sources/vendor/bundle/ruby/2.7.0/gems/minitest-5.16.0/lib/minitest/mock.rb:31: warning: The called method is defined here

I'll try to distil a minimal example.

I had builds for Ruby 2.5, 2.7 and 3.1 - it only failed on 2.7.

[thomthom]

Got a minimal repro. In my case the issue is that I have some objects that implement to_hash. When these objects are passed as arguments to Observerables notify_observers call there's an error thrown:

require 'minitest/autorun'

require 'observer'

class Example

  include Observable

  def trigger
    changed
    notify_observers(:triggered, self)
  end

  # Remove this method and the test will pass.
  # In Ruby 2.7 this cause the mock-observer interaction to fail with MiniTest 5.16.0.
  def to_hash
    { foo: 'bar' }
  end

end

class ExampleTest < Minitest::Test

  def test_example_trigger
    observable = Example.new
    mock = Minitest::Mock.new
    mock.expect(:hash, mock.object_id)
    mock.expect(:event, nil, [:triggered, observable])
    observable.add_observer(mock, :event)
    observable.trigger
  end

end

Ruby 2.7 and MiniTest 5.16.0:

  1) Error:
ExampleTest#test_example_trigger:
ArgumentError: mocked method :event expects 2 arguments, got [:triggered]
    C:/Ruby27-x64/lib/ruby/2.7.0/observer.rb:197:in `block in notify_observers'
    C:/Ruby27-x64/lib/ruby/2.7.0/observer.rb:196:in `each'
    C:/Ruby27-x64/lib/ruby/2.7.0/observer.rb:196:in `notify_observers'
    C:/Users/Thomas/source/example/tests/standalone/model/example_test.rb:11:in `trigger'
    C:/Users/Thomas/source/example/tests/standalone/model/example_test.rb:30:in `test_example_trigger'

In Ruby 2.5 and Ruby 3.1 that example works just fine.

If I remove the to_hash method (not an option) then Ruby 2.7 also passes.

Previous versions of MiniTest also worked fine with the above example.

Looks like something in Ruby 2.7 and MiniTest 5.16.0 causes the to_hash'able object to be resolves incorrectly when passed as an argument. There was some changes in Ruby to how hashes and keyword params were resolves, weren't there?

[zrzka]

Just experienced it as well. Ruby 3.1.2, Rails 6.1.6 and MiniTest 5.16.0. 5.15.0 works fine.

DockerhubPublisherTest#test_publish_multi_arch:
--
  | ArgumentError: mocked method :tag expects 1 arguments, got []
  | lib/licensor/dockerhub_publisher.rb:192:in `block in tag'
  | lib/licensor/dockerhub_publisher.rb:190:in `map'
  | lib/licensor/dockerhub_publisher.rb:190:in `tag'
  | lib/licensor/dockerhub_publisher.rb:78:in `publish_multi_arch_image'
  | lib/licensor/dockerhub_publisher.rb:56:in `publish'

It's caused by:

mock.expect :tag, true, [{ repo: 'foo/bar', tag: "1.0.0-#{Licensor::DockerhubPublisher::X86_SUFFIX}" }]

Called in the code as:

image.tag(repo: repo, tag: tag)

It's an old code base, those things were not touched for a long time.

[baelter]

Think you need something like
If mocked method accepts kwags
and no kwargs passed to expect
and last arg of expect args is hash
pop last arg as kwarg and use to call mock

[dazuma]

We also just ran into this for some of Google's libraries. In 5.15, when mocking a method with keyword arguments, we'd need to pass the keyword arguments as a hash at the end of the positional arguments array passed to Mock#expect. Now in 5.16, that usage no longer works, and we need to pass the keyword arguments separately. So it's effectively a breaking change. The good news is, the new usage does work across Ruby 2 and 3.

[zenspider]

@baelter unfortunately, you have no idea what the real method takes at the point where expect is called.

And I'm in the sad position of looking at the last arg and not being sure if the user really wanted to pass a hash or if they wanted kwargs... Maybe a warning for a while?

[thomthom]

What would the warning be?

Would behaviour remain as in 5.16? (How would you then mock a method call where the only or last item is a Hash?)

[zenspider]

I now have a fix for the old-kwargs scenario as shown by @zrzka ... I don't yet have a fix for 2.6/2.7 for the observer issue from @thomthom yet (but I haven't investigated, so I'm not sure it is out of my hands or not).

Current fix: Set MT_KWARGS_HACK=1 to get the old scenario to work. If you set that var AND have real kwords AND have a hash at the end, it'll warn (once)... so it should only be an issue as you're transitioning over to the new setup.

[zenspider]

OK. I've investigated the observer scenario. Because Observer in 2.7 uses send without a **kwargs it is going to be shoving kwargs into a plain *args and then complaining about it. The good news is with both MT_KWARGS_HACK=1 and without, the observer code warns but passes.... So I'm gonna chalk that up as unfortunate, but working fine.

The workarounds are to either upgrade past ruby 2.7, not upgrade into minitest 5.16, or not declare your class as "is-a Hash" by having a to_hash method (consider having to_h instead and using it explicitly where needed).

If it works, but warns, it's still working.

[zenspider]

I misstated the observer scenario above. With the code I just pushed, here's the outcomes:

    # without ENV:
    mock.expect(:event, nil, [:triggered, observable])   # original: fails
    mock.expect(:event, nil, [:triggered, **observable]) # use to_hash: fails
    mock.expect(:event, nil, [:triggered], observable)   # kwargs: pass
    mock.expect(:event, nil, [:triggered], **observable) # kwargs/to_hash: pass

    # with ENV MT_KWARGS_HACK=1
    mock.expect(:event, nil, [:triggered, observable])   # original: fails
    mock.expect(:event, nil, [:triggered, **observable]) # use to_hash: pass
    mock.expect(:event, nil, [:triggered], observable)   # kwargs: pass
    mock.expect(:event, nil, [:triggered], **observable) # kwargs/to_hash: pass

I doubt I'm going to come up with a working version that deals with: ruby 2.7's observer + Example w/ to_hash + minitest 5.16 with or without the ENV adjustment above. I think it's just gonna have to be modified in 1 of the 3 ways above.

Given that the problem goes away with either: not treating your class as a is-a-Hash or using ruby 3.0+ or using your expectations in a more complaint way...

... well let's just say I'm open to a compelling argument for more compatibility, I just doubt it'll be that compelling at this point.

[zenspider]

I have a rule about not releasing after midnight ESPECIALLY if liquor has been involved... but this fix will come out soon enough.

[thomthom]

Thanks for investigating that. I might not really need to use to_hash and could possibly use to_h instead. (I need to double check my JSON serialization logic.)

[zenspider]

to_hash is like to_ary and friends (vs to_h or to_a)... Only should be used if they're really implicitly "is-a"... So def worth considering

[zenspider]

OK. This is released in 5.16.1. Please update, export MT_KWARGS_HACK=1, and see how that works for y'all.

(and please only use this workaround in order to have working tests while you transition to proper kwargs calling conventions.)

[casperisfine]

One type of case I wasn't able to translate is:

mock.expect(return_value, [positional_1, Hash])

As to allow any keyword arg. It's not necessarily good code, but the translation is really non-trivial.

[zenspider]

@casperisfine and this doesn't work using the env? Can you scratch me up a quick test? You can steal a skeleton from 2-3 commits ago.

[casperisfine]

this doesn't work using the env?

I haven't tried, I just rewrote these mocks using mocha.

Can you scratch me up a quick test?

👀

[casperisfine]

Here you go: Shopify@a527a74, apparently it doesn't work with the env either.

[eileencodes]

I have another example of code that breaks with teh upgrade in Ruby 2.7 (works fine in 3.0+) and doesn't work with the ENV var hack. My PR is here rails/rails#45370, failures here https://buildkite.com/rails/rails/builds/87503#01818819-5daf-4cd7-bdfb-e48ec43c837c and you can run one of the Railties tests with bundle exec ruby -Ilib:test test/generators/actions_test.rb -n test_rails_command_with_env_option_should_run_rails_with_the_env_environment. Let me know if it's easier to debug outside of Rails and I'll try to make a demo app or script that repros.

[zenspider]

@casperisfine I have your test in and passing...

Passed: 2.6.10, 2.7.6, 3.0.4, 3.1.2

[zenspider]

@tsugimoto stub isn't mock so please file a separate issue

[casperisfine]

I have your test in and passing...

It's failing on CI: https://github.com/Shopify/minitest/runs/6999031451?check_suite_focus=true#step:5:477

[miguelperez]

OK. This is released in 5.16.1. Please update, export MT_KWARGS_HACK=1, and see how that works for y'all.

(and please only use this workaround in order to have working tests while you transition to proper kwargs calling conventions.)

Hey, thanks for the update. I would like to know what are "proper kwargs calling conventions".

For example, tests implemented following this https://www.reinhardt.io/ruby/testing/2021/05/25/stubbing-and-mocking-in-minitest.html started to fail.

test "it will set the type to load" do
  mock = Minitest::Mock.new

  args_checker = lambda do |args|
    assert_equal :load, args[:type]
  end
  mock.expect :notify!, true, [args_checker]

  SomeClass.stub(:new, mock) do
    logic_that_triggers_notify
  end

  mock.verify
end

# method in question is defined like this:
def notify!(type:)
  something
end

The number of tests that fail is small < 5. So I can change them but would like to understand better whether the tests could be specified in a more proper way.

[zenspider]

@casperisfine sorry... by "in" I meant locally... I've finally polished, committed, and pushed.

[zenspider]

@eileencodes yeah... i'mma need some help untangling that patch into a test I can debug

[zenspider]

@miguelperez I'm guessing you want something more like:

mock.expect :notify!, true, [], type: :load

(no args_checker at all)

[zenspider]

@eileencodes this will work (once released):

https://github.com/rails/rails/pull/45370/files#diff-14cab604508ced2a334cad4623993cac14e1d4b55d6b4e1bc681b830924f492aL178

[zenspider]

@eileencodes BUT!I don't want to release until I get a thumbs up from you (or a thumbs down and smaller repro)

[eileencodes]

BUT!I don't want to release until I get a thumbs up from you (or a thumbs down and smaller repro)

It doesn't look like it's fixed, but I don't know how to get the unreleased version hooked up to Rails CI to do a full test it since there's no gemspec. I did this locally by creating a gemspec manually but that's not going to work on Rails CI without forking, which I guess I can do to see the full set of failures.

Is there another way to do this more easily? I've never used hoe so I'm not sure how to make it play nicely with bundler's expectations.

[zenspider]

@eileencodes I really just need a failing test extracted enough that I don't need the whole world to come with it. I think I've done it. I've got a run w/:

  1) Error:
ActiveSupport::Cache::RedisCacheStoreTests::InitializationTest#test_stuff:
ArgumentError: mocked method :call expects 1 arguments, got []

with the following:

require "minitest/autorun"

class Redis
end

class ActiveSupport
  class Cache
    class RedisCacheStore
      attr_accessor :kws
      def initialize(**kws)
        self.kws = kws
      end

      def redis
        call(:url                => "redis://localhost:6379/0",
             :connect_timeout    => 20,
             :read_timeout       => 1,
             :write_timeout      => 1,
             :reconnect_attempts => 0,
             :driver             => "hiredis")
      end

      def call **kws
        Redis.new
      end
    end
  end
end

module ActiveSupport::Cache::RedisCacheStoreTests
  REDIS_URL  = "redis://localhost:6379/0"
  REDIS_URLS = %w[ redis://localhost:6379/0 redis://localhost:6379/1 ]
  REDIS_UP   = true
  DRIVER     = "hiredis"

  class InitializationTest < Minitest::Test
    def build(**kwargs)
      ActiveSupport::Cache::RedisCacheStore.new(driver: DRIVER, **kwargs.merge(pool: false)).tap(&:redis)
    end

    def test_stuff
      default_args = {
        connect_timeout: 20,
        read_timeout: 1,
        write_timeout: 1,
        reconnect_attempts: 0,
        driver: DRIVER
      }

      mock = Minitest::Mock.new
      mock.expect(:call, Redis.new, [{ url: REDIS_URLS.first }.merge(default_args)])
      mock.expect(:call, Redis.new, [{ url: REDIS_URLS.last }.merge(default_args)])

      Redis.stub(:new, mock) do
        @cache = build url: REDIS_URLS
        assert_kind_of ::Redis, @cache.redis
      end

      assert_mock(mock)
    end
  end
end

BUT! this fails across the board, regardless of version it seems. Found it here:

https://buildkite.com/rails/rails/builds/87503#01818819-5d9b-439d-ba67-f905c797161c/1169-1202

can you verify that the test extraction is close enough for the repro you're seeing? It just needs to be able to run against a local copy (or install) of minitest and reproduce the error.

[eileencodes]

I really just need a failing test extracted enough that I don't need the whole world to come with it. I think I've done it. I've got a run w/:

I understand but I wanted to know all of the failures so I can give you an example of the different sets of failures we have rather than having to go back and forth if one set is fixed and another is not.

[zenspider]

@eileencodes I've run both activemodel/test/cases/validations/with_validation_test.rb and activesupport/test/cache/stores/redis_cache_store_test.rb and verified failures on 2 tests on minitest 5.16.0. They fail under 5.16.0 as-is but pass with MT_KWARGS_HACK=1:

10013 % ruby27 test/cases/validations/with_validation_test.rb -n ValidatesWithTest#test_passes_all_configuration_options_to_the_validator_class
Run options: -n ValidatesWithTest#test_passes_all_configuration_options_to_the_validator_class --seed 24736

# Running:

/Users/ryan/Work/git/rails/rails/activemodel/lib/active_model/validations/with.rb:86: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/ryan/.rubies/ruby-2.7.6/lib/ruby/gems/2.7.0/gems/minitest-5.16.1/lib/minitest/mock.rb:149: warning: The called method `method_missing' is defined here
E

Error:
ValidatesWithTest#test_passes_all_configuration_options_to_the_validator_class:
ArgumentError: mocked method :new expects 1 arguments, got []
    /Users/ryan/Work/git/rails/rails/activemodel/lib/active_model/validations/with.rb:86:in `block in validates_with'
    /Users/ryan/Work/git/rails/rails/activemodel/lib/active_model/validations/with.rb:85:in `each'
    /Users/ryan/Work/git/rails/rails/activemodel/lib/active_model/validations/with.rb:85:in `validates_with'
    test/cases/validations/with_validation_test.rb:87:in `block in <class:ValidatesWithTest>'

rails test test/cases/validations/with_validation_test.rb:80

Finished in 0.000491s, 2036.6598 runs/s, 0.0000 assertions/s.
1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
10014 % MT_KWARGS_HACK=1 ruby27 test/cases/validations/with_validation_test.rb -n ValidatesWithTest#test_passes_all_configuration_options_to_the_validator_class
Run options: -n ValidatesWithTest#test_passes_all_configuration_options_to_the_validator_class --seed 58606

# Running:

/Users/ryan/Work/git/rails/rails/activemodel/lib/active_model/validations/with.rb:86: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/ryan/.rubies/ruby-2.7.6/lib/ruby/gems/2.7.0/gems/minitest-5.16.1/lib/minitest/mock.rb:149: warning: The called method `method_missing' is defined here
.

Finished in 0.009052s, 110.4728 runs/s, 110.4728 assertions/s.
1 runs, 1 assertions, 0 failures, 0 errors, 0 skips

Unfortunately a lot of the other failing groups seem out of reach for me.

I'm trying to find where one might put a global env for circleci. There might be some config on via the website that I don't have access to. It's been a few years and I think we did ours via yml files like GHA workflows... So I'm trying out putting it in tools/test_common.rb as that seems to hint it might be the right place.

[zenspider]

@eileencodes PS activesupport test above runs fine w/ no extra -I or bundle exec... activemodel does not. I found that AS uses require_relative and the helper requires bundler/setup whereas AM does not. I'm not really wanting to clean all that up in this PR but I can put a small start in for the test file I was trying.

[zenspider]

I haven't heard anything back on the rails side... I'm going to release what I've got.

[zenspider]

This is released in 5.16.2. Thanks for reporting! @baelter, please close if this addresses your issues.

Others: please open a NEW issue if you're edge cases aren't addressed.

[baelter]

Works when changing to the new method signature, so that is good. We can lift the pin and update our specs. MT_KWARGS_HACK=1 also works fine in our case. Nice job!

[PetrKaleta]

Hey guys,
please have a look on the following example:

# frozen_string_literal: true

require 'minitest/autorun'
require 'minitest/spec'

class UserSychronizer
  def initialize(user_id)
    @user_id = user_id
  end

  # @param data [Hash]
  def synchronize(data)
    UserProfileSynchronizer.new(@user_id).synchronize(data)
  end
end

class UserProfileSynchronizer
  def initialize(user_id)
    @user_id = user_id
  end

  # @param data [Hash]
  def synchronize(data)
    # TODO: profile sync
  end
end

describe UserSychronizer do
  describe '#synchronize' do
    before do
      @user_id = 1
      @data = {
        first_name: 'Foo',
        last_name: 'Bar'
      }
    end

    it 'calls UserProfileSynchronizer#synchronize' do
      mock = Minitest::Mock.new
      mock.expect :synchronize, nil, [@data]

      UserProfileSynchronizer.stub :new, mock do
        UserSychronizer.new(@user_id).synchronize(@data)
      end

      assert_mock mock
    end
  end
end

It worked without any issues on minitest 5.15.0 (ruby 2.7.6) but after upgrading to minitest 5.16.2 (ruby 2.7.6) I am getting:

# Running:

_minitest_mock.rb:13: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/Users/foo/.asdf/installs/ruby/2.7.6/lib/ruby/gems/2.7.0/gems/minitest-5.16.2/lib/minitest/mock.rb:150: warning: The called method `method_missing' is defined here
E

Finished in 0.001020s, 980.3922 runs/s, 0.0000 assertions/s.

  1) Error:
UserSychronizer::#synchronize#test_0001_calls UserProfileSynchronizer#synchronize:
ArgumentError: mocked method :synchronize expects 1 arguments, got []
    _minitest_mock.rb:13:in `synchronize'
    _minitest_mock.rb:43:in `block (4 levels) in <main>'
    _minitest_mock.rb:42:in `block (3 levels) in <main>'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

I am receiving exactly the same error if I use mock.expect :synchronize, nil, [Hash].

Any ideas?

Update:
Solved by MT_KWARGS_HACK=1