Include the original test name in expired stub error messages

[casperisfine]

It is currently very hard to track down such leaked stubs, when they happen, as they might be created in one test and cause an exception dozens of tests later.

By including the name of the test that invalidated them, it makes the debugging process a bit easier.

Note, I made setup also accept the current test instance purely for consistency with teardown but it's not strictly necessary.

[floehopper]

Thanks for your contribution. Will this work with other test libraries, i.e. test-unit & rspec?

[casperisfine]

Ah, I wasn't aware mocha was used in other contexts.

If that's so, rather than pass the Minitest::Test instance and mocha render it, I think we can pass an optional string, and then each integration can render the "source" how it's best for them.

Would that work for you?

[floehopper]

Ah, I wasn't aware mocha was used in other contexts.

If that's so, rather than pass the Minitest::Test instance and mocha render it, I think we can pass an optional string, and then each integration can render the "source" how it's best for them.

Would that work for you?

Yes, I think that would work. Making it optional seems sensible for now at least to give integrations not maintained in this repo (e.g. rspec) time to make the change.

[casperisfine]

Alright, I updated the code, now the origin string is optional, and the Minitest integrations computes it.

[casperisfine]

For the record, this made it super easy to find the issue, it was:

ind_delivery_mock = Mocha::Mock.new(Sales::FindDeliveryOptionsForOrder)

With this patch it now immediately failed with:

Minitest::UnexpectedError: NoMethodError: undefined method `sequences' for class Sales::FindDeliveryOptionsForOrder
    mocha (a9e8daa98a85) lib/mocha/mock.rb:118:in `block in expects'
    mocha (a9e8daa98a85) lib/mocha/argument_iterator.rb:13:in `each'
    mocha (a9e8daa98a85) lib/mocha/mock.rb:114:in `expects'

[floehopper]

Alright, I updated the code, now the origin string is optional, and the Minitest integrations computes it.

For the record, this made it super easy to find the issue, it was:

ind_delivery_mock = Mocha::Mock.new(Sales::FindDeliveryOptionsForOrder)

With this patch it now immediately failed with:

Minitest::UnexpectedError: NoMethodError: undefined method `sequences' for class Sales::FindDeliveryOptionsForOrder
    mocha (a9e8daa98a85) lib/mocha/mock.rb:118:in `block in expects'
    mocha (a9e8daa98a85) lib/mocha/argument_iterator.rb:13:in `each'
    mocha (a9e8daa98a85) lib/mocha/mock.rb:114:in `expects'

@casperisfine Thanks. That looks good.

I've just dropped support for Ruby v2.0 in #642, so no need to worry about that build failure.

However, I notice that the Ruby v2.1 build is failing because the version of Minitest included in the stdlib at that point did not have a #name method. Unless it's particularly awkward, I'd like to continue to support Ruby v2.1 for as long as possible and it looks as if Minitest had a #__name__ method that might be suitable. Otherwise, I'd be OK with just giving the class name in that case.

Also I think ideally I'd like to add support for this feature to the Test::Unit adapter at the same time since support for it is part of this project.

I'll try to find some time to dig into these issues, but I'm busy with client work at the moment, so it might have to wait a while. On that note, would Shopify be prepared to sponsor the project? That way it would be easier for me to justify taking time away from billable client work - we're just a 3-person shop, so not much slack in the system! Thanks again for your work on this.

[casperisfine]

However, I notice that the Ruby v2.1 build is failing because the version of Minitest included in the stdlib at that point did not have a #name method

Thanks for the heads up, I'll try to make it work, I'll let you know. Testing with such old rubies is hard today, but I can perhaps downgrade Minitest on a newer Ruby.

Also I think ideally I'd like to add support for this feature to the Test::Unit adapter at the same time

I can have a look as well.

---

would Shopify be prepared to sponsor the project?

I'll forward the request to the relevant people, but I suspect not.

To be very frank with you (hopefully not rude), getting approvals for this kind of things in big companies is like pulling teeth, and mocha isn't critical enough to our operations to really make a string case for it. Please don't take this the wrong way, but while we're happy to use mocha, if it were to disappear or no longer be maintained we'd just migrate to something else or maintain a fork ourselves as it wouldn't be a ton of work.

Additionally, and on a more personal note, Shopify is kinda already supporting this project by sending patches, me and a number of other employees provided what is essentially free labour in the form of PRs over the years. I'm not gonna try to quantify it, but it's non negligible. And to be clear it's particularly the case of this PR. Shopify has 0 interest in this PR, I solved Shopify problem with a quick fork, I'm only submitting this upstream because I believe it can helps other members of the community, and I'm doing so on Shopify payroll, that alone is thousands of dollars out of Shopify's pocket given to this project.

All this to say, if what you need is "developer time", that's super easy to get. If what you need is cash, it's more tricky.

[casperisfine]

Alright, compat with 2.1 is fixed, now looking at test/unit.

[casperisfine]

Alright, I think I have it working with TestUnit, it's exposed as #name https://github.com/test-unit/test-unit/blob/c26354f5ccf000222e99e70a7daf992b50e40670/lib/test/unit/testcase.rb#L760-L764

But if I purposedly call a non existing method, the test suite doesn't fail, so it seems the TestUnit integration isn't actually covered.

[casperisfine]

Alright, tested manually with the following script

# frozen_string_literal: true

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'
  gem 'test-unit'
  gem 'mocha', github: 'https://github.com/freerange/mocha/pull/641'
end

require "test/unit"
require 'mocha/test_unit'

$leaky_mock = nil

class TC_MyTest < Test::Unit::TestCase
  3.times do |i|
    define_method(:"test_fail_#{i}") do
      $leaky_mock ||= begin
        bad_mock = mock(:leaky)
        bad_mock.expects(:call)
        bad_mock
      end

      $leaky_mock.call
      assert(true, 'Assertion was true.')
    end
  end
end

$ ruby /tmp/mocha.rb 
Loaded suite /tmp/mocha
Started
E
============================================================================================================================================================================================================================================================================================================================
Error: test_fail_1(TC_MyTest): Mocha::StubbingError: #<Mock:leaky> was instantiated in test_fail_0(TC_MyTest) but it is receiving invocations within another test. This can lead to unintended interactions between tests and hence unexpected test failures. Ensure that every test correctly cleans up any state that it introduces.
/tmp/mocha.rb:25:in `block (2 levels) in <class:TC_MyTest>'
============================================================================================================================================================================================================================================================================================================================
E
============================================================================================================================================================================================================================================================================================================================
Error: test_fail_2(TC_MyTest): Mocha::StubbingError: #<Mock:leaky> was instantiated in test_fail_0(TC_MyTest) but it is receiving invocations within another test. This can lead to unintended interactions between tests and hence unexpected test failures. Ensure that every test correctly cleans up any state that it introduces.
/tmp/mocha.rb:25:in `block (2 levels) in <class:TC_MyTest>'
============================================================================================================================================================================================================================================================================================================================
Finished in 0.001023 seconds.
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
3 tests, 2 assertions, 0 failures, 2 errors, 0 pendings, 0 omissions, 0 notifications
33.3333% passed
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
2932.55 tests/s, 1955.03 assertions/s

[floehopper]

would Shopify be prepared to sponsor the project?

I'll forward the request to the relevant people, but I suspect not.

To be very frank with you (hopefully not rude), getting approvals for this kind of things in big companies is like pulling teeth, and mocha isn't critical enough to our operations to really make a string case for it. Please don't take this the wrong way, but while we're happy to use mocha, if it were to disappear or no longer be maintained we'd just migrate to something else or maintain a fork ourselves as it wouldn't be a ton of work.

Additionally, and on a more personal note, Shopify is kinda already supporting this project by sending patches, me and a number of other employees provided what is essentially free labour in the form of PRs over the years. I'm not gonna try to quantify it, but it's non negligible. And to be clear it's particularly the case of this PR. Shopify has 0 interest in this PR, I solved Shopify problem with a quick fork, I'm only submitting this upstream because I believe it can helps other members of the community, and I'm doing so on Shopify payroll, that alone is thousands of dollars out of Shopify's pocket given to this project.

All this to say, if what you need is "developer time", that's super easy to get. If what you need is cash, it's more tricky.

Thank you. I appreciate your frankness. As I hope I've already indicated, I am very grateful for your contribution (and those of other Shopify developers). I just always feel bad when I can't respond as quickly as I would like or that to do so I have to sacrifice a chunk of personal time. Anyway, any help in whatever form is always extremely welcome! ❤️

[floehopper]

Alright, compat with 2.1 is fixed, now looking at test/unit.

Brilliant - thank you!

[floehopper]

Alright, I think I have it working with TestUnit, it's exposed as #name https://github.com/test-unit/test-unit/blob/c26354f5ccf000222e99e70a7daf992b50e40670/lib/test/unit/testcase.rb#L760-L764

But if I purposedly call a non existing method, the test suite doesn't fail, so it seems the TestUnit integration isn't actually covered.

Great - I'll have a quick look now to see if I can add some test coverage.

[floehopper]

Great - I'll have a quick look now to see if I can add some test coverage.

I've added an integration test in #644 and merged it. I'll try to get it releases as soon as I can.

[casperisfine]

Thanks! ❤️

No rush for the release, I already found our leak an fixed it so this can take as long as it needs.

[floehopper]

Released in v2.2.0