Skip to content
This repository

Please stop monkey patching minitest #87

Closed
tenderlove opened this Issue · 19 comments

3 participants

Aaron Patterson James Mead Kouhei Sutou
Aaron Patterson

Or at least provide a way to opt out or in.

Right now, mocha monkey patches minitest regardless of whether or not I asked it to. This is a bad idea because:

We are using minitest version 3.0.0 on Rails master, but because mocha hasn't copy / pasted the runner from minitest version 3.0.0, and due to a bug in the monkey patch selection code, it means that I cannot use minitest's before_setup and after_teardown hooks.

I would like to suggest two things:

  1. Provide a module that people can mix in to their test classes that implements mocha's setup and teardown requirements (this let's people opt-in on a per class basis)
  2. Provide an API for monkey patching everything (this lets people use mocha globally, but opt-in)

On Rails master, my only path forward is to either remove mocha or to copy / paste minitest's runner in to Rails. I am excited about neither.

Thanks!

James Mead
Owner

Thanks for raising the issue.

With regard to point 1, if you require mocha_standalone and then include Mocha::API, you should have Mocha::API#mocha_setup, Mocha::API#mocha_verify & Mocha::API#mocha_teardown available to you. Perhaps this will meet your needs. I think this is how Rspec integrates with Mocha.

Either I or one of my colleagues at @freerange will have a look into your suggestions as soon as we can.

Aaron Patterson

@floehopper awesome! :heart::heart::heart::heart:

I think what we can do is write a module like this:

module Mocha
  module MiniTestHooks
    def before_setup
      Mocha::API.mocha_setup
      super
    end

    def after_teardown
      super
      Mocha::API.mocha_verify
      Mocha::API.mocha_teardown
    end
end

Then when people want it globally, we just mix in to MiniTest::TestCase. Of course this will only work with minitest 3.0.0 and greater, but it does keep us from monkey patching and gives us a path to stop checking for minitest versions.

I think we could do one of two things for older versions of minitest, either:

  1. Backport the run method to older versions of minitest so that we can reuse the before setup and after teardown hooks
  2. Make mocha require minitest >= 3.0.0

I'm going to work on a module like that for our tests on Rails master so that we can use minitest 3.0.0 and still have mocha. I'll ping you after I get that working, and maybe we can push the module out of Rails and up stream to you.

Thanks!

James Mead
Owner

Hmm. The thing I forgot about is that ideally mocha_verify should be called at the end of the test, before the decision has been taken as to whether the test has passed or failed - this is not really the same as at teardown time. Also ideally an assertion counter should be passed in as an argument so that Mocha can record expectation verifications as an "assertion". I knew there was a reason this was tricky. I will give this some more thought.

James Mead
Owner

And there's also some "translation" of Mocha exceptions into MiniTest exceptions in the current integration code.

Aaron Patterson

I've pushed this to rails master. If an assertion exception is raised during teardown (or even after_teardown) minitest will declare that test as a failure:

require 'minitest/autorun'

class OmgTest < MiniTest::Unit::TestCase
  def after_teardown
    if __name__ == 'test_fails'
      assert_equal 1, 2
    else
      assert_equal 1, 1
    end
  end

  def test_fails
    assert_equal 1, 1
  end

  def test_passes
    assert_equal 1, 1
  end
end

If you run, the output is this:

[aaron@higgins git]$ ruby test.rb
Run options: --seed 14247

# Running tests:

F.

Finished tests in 0.048435s, 41.2925 tests/s, 82.5849 assertions/s.

  1) Failure:
test_fails(OmgTest) [test.rb:6]:
Expected: 1
  Actual: 2

2 tests, 4 assertions, 1 failures, 0 errors, 0 skips
[aaron@higgins git]$

If you raise a minitest assertion, you can make the test fail. For example we can have the after_teardown rescue mocha failure and reraise the exception:

require 'minitest/autorun'

class OmgTest < MiniTest::Unit::TestCase
  class MochaFailure < Exception
    # raised on a mocha failure
  end

  def verify_mocks
    if __name__ == 'test_fails' # a mock failure
      raise MochaFailure, 'omg, your mock failed'
    end
  end

  def after_teardown
    begin
      verify_mocks
    rescue MochaFailure => e
      # if we catch a mocha failure, rerraise a minitest assertion
      # failure so that the test fails
      raise MiniTest::Assertion, e.message, e.backtrace
    end
    super
  end

  def test_fails
    assert_equal 1, 1
  end

  def test_passes
    assert_equal 1, 1
  end
end

Will output:

[aaron@higgins git]$ ruby test.rb
Run options: --seed 57976

# Running tests:

F.

Finished tests in 0.001135s, 1762.1145 tests/s, 1762.1145 assertions/s.

  1) Failure:
test_fails(OmgTest) [test.rb:10]:
omg, your mock failed

2 tests, 2 assertions, 1 failures, 0 errors, 0 skips
[aaron@higgins git]$

The assertion counter can be set up in the before_setup method if you like.

Hope that helps!

Aaron Patterson

WRT increasing assertion values, the assert methods on the test case are public. It's kinda hacky, but you could increase the assertion count without mucking with ivars. For example:

require 'minitest/autorun'

class OmgTest < MiniTest::Unit::TestCase
  class MochaVerifier
    class MochaFailure < Exception
      # raised on a mocha failure
    end

    def initialize tc
      @tc = tc
    end

    def verify!
      if @tc.__name__ == 'test_fails' # a mock failure
        raise MochaFailure, 'omg, your mock failed'
      else
        5.times { # pretend there were 5 mock verifications
          @tc.assert true
        }
      end
    end
  end

  def before_setup
    @_mocha_verify = MochaVerifier.new self
    super
  end

  def after_teardown
    begin
      @_mocha_verify.verify!
    rescue MochaVerifier::MochaFailure => e
      # if we catch a mocha failure, rerraise a minitest assertion
      # failure so that the test fails
      raise MiniTest::Assertion, e.message, e.backtrace
    end
    super
  end

  def test_fails # this fails
    assert_equal 1, 1
  end

  def test_passes # this should have 6 assertions total
    assert_equal 1, 1
  end
end

Outputs:

[aaron@higgins git]$ ruby test.rb
Run options: --seed 17705

# Running tests:

F.

Finished tests in 0.001091s, 1833.1806 tests/s, 6416.1320 assertions/s.

  1) Failure:
test_fails(OmgTest) [test.rb:15]:
omg, your mock failed

2 tests, 7 assertions, 1 failures, 0 errors, 0 skips

7 assertions total, 6 of them for test_passes, 1 of them is the successful assertion in test_fails.

James Mead
Owner

Thanks. That's all really useful stuff. My plan is to bring all the Test::Unit and pre-v3.0.0 MiniTest monkey-patching up-to-date and then have a go at introducing the type of module that you are suggesting.

James Mead
Owner

I've had a bit more of a play with this and the main problem I'm having is that a Mocha::ExpectationError can be raised as the result of an unexpected invocation (either there is no expectation on a mock object or the Mocha::Expectation#never constraint has been applied to an expectation). Although the most likely time for this to happen is during the test body, it could conceivably happen in setup or teardown. I started to look at ways to rescue the exception and translate it into a MiniTest exception, but it's pretty awkward, which I think I resorted to monkey-patching in the first place. However, I'm now wondering whether a better approach might be to have a exception factory in Mocha which knows whether it should be raising MiniTest exceptions or Test::Unit exceptions - that way there would be no need for rescuing & translating. I'll keep you posted.

Aaron Patterson

Would it be possible to make Mocha::ExpectationError inherit from MiniTest::Assertion? If you did that, then you wouldn't have to do any exception translation.

James Mead
Owner

The reason I had avoided doing that was that I want to continue to support Test::Unit, but maybe Mocha could dynamically decide whether to have Mocha::ExpectationError inherit from MiniTest::Assertion or its Test::Unit equivalent. Unfortunately I don't have a lot of spare time at the moment, but I hope to get to work on this soon.

Kouhei Sutou
kou commented

Please don't inherit MiniTest::Assertion. There are not-minitest testing frameworks for Ruby. :cry:

James Mead
Owner

I think you misunderstand me - Mocha would choose whether to inherit from MiniTest::Assertion or Test::Unit::AssertionFailedError or whatever is appropriate for the current test framework - I have no intention of making Mocha MiniTest-specific.

Kouhei Sutou
kou commented

Ah, sorry. I commented to #87 (comment).

I understand that you don't put minitest-specific change for the exception. :smile:

James Mead
Owner

I've found time to do a bit more playing around with this. Another problem I've hit is that using #before_teardown to call #mocha_verify means that when an expectation is not satisfied, the @passed instance variable is not set to false as it is with the monkey-patch. This relates to the fact that #mocha_verify ought to be semantically considered part of the test and not just some cleanup that needs to happen after the test.

It doesn't look like there are currently any specific internal usages of the @passed instance variable, but it is available through the public MiniTest API as #passed? which might lead to confusion.

I came across this because I am using #passed? in a lot of the Mocha acceptance tests which run a test within a test. Note that the @failures and @errors are updated as you would expect. In some ways, I don't really see the purpose of @passed in that it seems to duplicate information.

One possibility is to override the #run_test method to call #mocha_verify in there. Another might be to override #passed? to ignore @passed and use @failures and @errors, but then we're straying back into monkey-patching territory.

I'll give it some more thought.

James Mead
Owner

My latest attempt at doing this is in the minitest-and-testunit-integration-without-monkey-patching branch. More specifically in Mocha::Integration::MiniTest.

This branch also introduces integration tests like this one for MiniTest. However, these tests are not currently wired in to the build.

James Mead
Owner

The activesupport-integration-testing branch contains some WIP on adding similar integration tests for ActiveSupport/MiniTest/Mocha & ActiveSupport/Test::Unit/Mocha. I'm not convinced these tests belong in the Mocha repo, but they're a start.

One thing I've realised these integration tests don't cover very well is that all Mocha-related state is torn down properly and there is no bleed through between tests.

James Mead
Owner

Because I'm going to be away for a while, I thought I'd write up some quick notes about what I think needs doing on the minitest-and-testunit-integration-without-monkey-patching branch before it could be merged and released :-

  • Decide whether require 'mocha/integration/mini_test' and require 'mocha/integration/test_unit' should actually include the relevant integration module into the test library, or whether the user should do this explicitly. The benefit of not doing this automatically is that the module could just be included into individual test cases. However it then puts an additional overhead on developers, particularly on those just upgrading an existing project. Another option is to have a different file to require if you want the automatic include (c.f. require 'minitest/autorun'). I think I prefer the latter option.
  • Protect against integration modules being included twice - check ancestors?
  • Should we continue to support automatic test library detection? and potentially include into both test libraries? Watch out for Test::Unit shim around MiniTest. Maybe the best thing is to keep the current behaviour i.e. require 'mocha' detects which test library you're using and automatically includes the relevant integration module; then separately people could just require the integration module. However, perhaps this isn't a good idea - because of the difficulty in failing fast at the bottom of Mocha::MonkeyPatching?
  • Wire integration tests into build, both local and travis-ci.
  • Documentation for Mocha::Integration modules
  • Documentation for mocha_standalone.rb i.e. for library integrators.
James Mead
Owner

@tenderlove I believe the latest release of mocha (v0.13.0) incorporates the changes we discussed. Please close this issue if you are happy it is now resolved. Thanks for your patience.

James Mead floehopper closed this
James Mead
Owner

I believe this is fixed, so I'm closing the issue. Please re-open if I am mistaken. Thanks.

Trong Tran trongrg referenced this issue in thoughtbot/shoulda-matchers
Merged

Loosen Bourne dependency #256

James Mead floehopper referenced this issue from a commit in freerange/mocha.methods
James Mead Use public API of MiniTest for registering assertions.
As suggested by @tenderlove in [1]. This reduces the dependency on
private & potentially more changeable behaviour in MiniTest and gets us
closer to the goal of not having to monkey-patch.

[1] freerange/mocha#87 (comment)
1b07950
James Mead floehopper referenced this issue from a commit in freerange/mocha.methods
James Mead Use public API of MiniTest for registering assertions.
As suggested by @tenderlove in [1]. This reduces the dependency on
private & potentially more changeable behaviour in MiniTest and gets us
closer to the goal of not having to monkey-patch.

[1] freerange/mocha#87 (comment)
fa0e61d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.