freerange / mocha

A mocking and stubbing library for Ruby
https://mocha.jamesmead.org
Other
1.23k stars 158 forks source link

Mocha::API#mocha_teardown not called under MiniTest v2.4.0 #33

Closed leehambley closed 12 years ago

leehambley commented 13 years ago
$ MOCHA_OPTIONS=debug ruby -Itest ./test/unit//api_test.rb
Detected MiniTest version: 2.4.0
*** MiniTest integration has not been verified but patching anyway ***
Monkey patching MiniTest >= v2.0.1 <= v2.0.2
Loaded suite ./test/unit//api_test
Started

ApiEpisodeTest:
     PASS test_season_number (0.00s) 
     PASS test_banner_path (0.00s) 
     PASS test_should_extract_the_network_id (0.00s) 
     PASS test_name (0.00s) 
     PASS test_overview (0.00s) 
     PASS test_episode_number (0.00s) 
     PASS test_attributes_hash (0.00s) 

Finished in 0.005293 seconds.

7 tests, 8 assertions, 0 failures, 0 errors, 0 skips

$ bundle show | ack minitest
  * minitest (2.4.0)

I can assert this by modifying [Mocha::API#mocha_teardown|https://github.com/floehopper/mocha/blob/master/lib/mocha/api.rb#L159] with a debugger/raise/warn statement, and we simply don't get in there.

This manifests as bleeding test suites, with my expectations and assertions leaking between tests. Resulting in tests which pass when run alone.

leehambley commented 13 years ago

I noticed you're running the build against Travis, so here's some additional notes from my env:

$ gem list --remote minitest

*** REMOTE GEMS ***

minitest (2.4.0)

$ rvm info (...snipped...)
  ruby:
    interpreter:  "ruby"
    version:      "1.9.2p180"
    date:         "2011-02-18"
    platform:     "x86_64-darwin10.7.0"
    patchlevel:   "2011-02-18 revision 30909"
    full_version: "ruby 1.9.2p180 (2011-02-18 revision 30909) [x86_64-darwin10.7.0]"

$ bundle show
Gems included by the bundle:
  * addressable (2.2.6)
  * ansi (1.3.0)
  * archive-tar-minitar (0.5.2)
  * bundler (1.0.15)
  * columnize (0.3.4)
  * crack (0.1.8)
  * fastimage (1.2.8)
  * linecache19 (0.5.12)
  * minitest (2.4.0)
  * mocha (0.9.12)
  * nokogiri (1.5.0)
  * rake (0.9.2)
  * ruby-debug-base19 (0.11.25)
  * ruby-debug19 (0.11.6)
  * ruby_core_source (0.1.5)
  * turn (0.8.2)
  * tv_data_api_client (0.0.1 06d16d7)
  * webmock (1.7.0)
floehopper commented 13 years ago

Hi Lee. Thanks for your bug report. I haven't had a chance to look at it in detail yet, but I did just push up some changes for recent versions of minitest. It's unlikely they're going to fix your problem, but it would be good to rule that out. Would you be able to try out HEAD mocha? The other quick thought I've had is that I know some other people have had problems using the turn gem in conjunction with mocha. Can you try removing that gem from your Gemfile? I'll try to have a proper look as soon as I can, but I might not be able to for a day or two. The only other thing I can suggest is to try removing one gem at a time and seeing whether that makes any difference. Alternatively start from a new project and build up adding one gem at a time. Cheers, James.

leehambley commented 13 years ago

The other quick thought I've had is that I know some other people have had problems using the turn gem in conjunction with mocha.

I hear you there, I'm expecting that Turn and Mocha are monkey patching the same methods, and someone is being an inconsiderate citizen.

Regarding the "start a new project" suggestion, this is re-factoring a monolithic Rails 3.0x app into a handful of co-operative packages, with their individual test suites; much cleaner, and also because with Turn, trying to diagnose these tests under Rails 3.1 was proving impossible (it only displays one line of backtrace!)

Is there a bigger problem with MiniTest and Test::Unit that they don't allow for sufficient hooking?

cameel commented 13 years ago

Test::Unit does not look too extensible in this regard. For example Turn has to check if it printed a dot to know that a test succeeded - there is no hook for that. Both Mocha and Turn make some ugly monkey-patches but I don't know if there's any other way to do it in a backwards-compatible way.

floehopper commented 13 years ago

Neither Test::Unit nor MiniTest have much capability for the type of hooks Mocha needs. A while ago I did start looking at whether Test::Unit::ExceptionHandler might give me the hooks I needed, but I didn't get as far as coming to any conclusions. I've been meaning to suggest suitable modifications to Test::Unit for a while, especially since they are now separate from Ruby itself and able to have their own release cycle.

leehambley commented 13 years ago

@floehopper, if you want someone to spar with, maybe we could pair remotely on that and push something back up to test unit, I'm speaking at a conference with Aaron Patterson in about 6 weeks, might be a great time to pick his brains about it?

floehopper commented 13 years ago

@leehambley Thanks for the offer of remote pairing. To be honest I'm struggling to find the time to do much work on Mocha at the moment, so the additional overhead of trying to coordinate a time to pair isn't very appealing. Correct me if I'm wrong, but I think your original issue (with Turn) has been addressed or at least worked around. I already have improving the Test::Unit & MiniTest integration on my todo list, so would you be happy to close this issue? Thanks, James.

leehambley commented 13 years ago

James, amen to not having much time to open source at the moment!

Using the latest versions of Turn and Mocha this is still apparently a problem, which issue did you think solved this with Turn? (The "work with gems that patch test::unit#run()" one?)

At least for this issue I need to come up with a concrete test case, and maybe the blame lies with Turn/ActiveSupport - I can't say for sure.

leehambley commented 13 years ago

I've just drafted a small test-case, before I close this issue here, and take it to turn, can you cast an eye over it and make sure i'm not missing anything ostensibly obvious.

leehambley commented 13 years ago

Additional info, when the load order is :mocha, :test_unit (no turn) - then the error is:

Loaded suite ./test_turn_stops_on_mocha
Started
.E.
Finished in 0.000854 seconds.

  1) Error:
test_bounced_count_really_gets_called(TestTurnStompsOnMocha):
Mocha::ExpectationError: unexpected invocation: #<AnyInstance:Widget>.increment_bounce_count()
unsatisfied expectations:
- expected exactly once, invoked twice: #<AnyInstance:Widget>.increment_bounce_count(any_parameters)

    ./test_turn_stops_on_mocha.rb:17:in `bounce'
    ./test_turn_stops_on_mocha.rb:49:in `test_bounced_count_really_gets_called'

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

Test run options: --seed 63218

When the load order is :test_unit, :mocha (no turn) then:

Loaded suite ./test_turn_stops_on_mocha
Started
...
Finished in 0.000668 seconds.

3 tests, 4 assertions, 0 failures, 0 errors, 0 skips

Test run options: --seed 22140

(or rather, everything is OK)

So, working from "Everything OK" to add turn, we then try :test_unit, :mocha, :turn:

Loaded suite ./test_turn_stops_on_mocha
Started

TestTurnStompsOnMocha:
     PASS test_bounced_count_increase_is_called_when_bounced (0.00s) 
    ERROR test_bounced_count_really_gets_called (0.00s) 
          Mocha::ExpectationError: unexpected invocation: #<AnyInstance:Widget>.increment_bounce_count()
unsatisfied expectations:
- expected exactly once, invoked twice: #<AnyInstance:Widget>.increment_bounce_count(any_parameters)

          ./test_turn_stops_on_mocha.rb:17:in `bounce'

     PASS test_initialize (0.00s) 

Finished in 0.001327 seconds.

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

So, then the last thing is to try :test_unit, :turn, :mocha:

Loaded suite ./test_turn_stops_on_mocha
Started

TestTurnStompsOnMocha:
 test_bounced_count_increase_is_called_when_bounced (0.00s) 
 test_bounced_count_really_gets_called (0.00s) 
 test_initialize (0.00s) 

Finished in 0.001075 seconds.

3 tests, 4 assertions, 0 failures, 0 errors, 0 skips

Which appears to work, except it's missing the "PASS" - I believe that's fixed upstream in Turn, here - but I'm not completely sure about the last point.

At least, given these results, I'd still like a final nod from you before I take it to the turn team.

Thanks @floehopper, appreciate the help and patience.

floehopper commented 13 years ago

@leehambley Thanks for writing the tests. I would not expect the tests to work when loading Mocha before Test::Unit. It would be nice for this to fail fast when Mocha is required and no test framework is available, but this is a known problem and it isn't straightforward to fix because there are other (non-Test::Unit and non-MiniTest) test frameworks that use Mocha.

Please do talk to the Turn folks. I'm open to making changes to Mocha to make integration more straightforward.

One suggestion for improving the tests would be to use Bundler and a Gemfile to specify exactly which gems and versions are required to make it easier for someone else to reproduce the problem. Presumably the tests need to be run in a specific order - does MiniTest guarantee they will be? Also I suspect it would be possible to write a slightly simpler test to demonstrate the problem, but I'm sure what you have will do.

Thanks, James.

leehambley commented 13 years ago

@floehopper, thanks for the sanity-check... yes I suspect a simpler test-case would be possible, but I won't bother. I considered using a Gemfile etc, but it's easy enough to understand what's going on in my opinion. Fair point about loading mocha before Test::Unit, no real stress there.

I'll take this upstream to turn and see what the can/will do about it. Thanks.

floehopper commented 12 years ago

I just ran your tests with Turn v0.8.3 and Mocha v0.10.0 and I believe the problem is fixed. In Ruby v1.9.2, Turn forced me to install the MiniTest gem and I had to add a gem 'minitest' statement at the top of your test, but otherwise everything worked ok. Can you close this issue if you're happy with the resolution? Thanks, James.

leehambley commented 12 years ago

@flowhopper sounds good! I haven't tried it myself, but I trust your skills over my own! Thanks for keeping this in mind. Merry Christmas!