Avoid re-defining classes in the standard library. #17

Closed
floehopper opened this Issue Mar 5, 2013 · 14 comments

Projects

None yet

3 participants

@floehopper

Someone has raised an issue on the Mocha repo. As I said in the comments there, it feels pretty nasty that you have to re-declare the Test::Unit::TestCase class and re-define the MiniTest::Unit::TestCase class.

Is there not a way you could declare a new sub-class of MiniTest::Spec and include all the relevant ActiveSupport::Testing modules into it?

@metaskills
Owner

Hey James! Big fan!

Is there not a way you could declare a new sub-class of MiniTest::Spec and
include all the relevant ActiveSupport::Testing modules into it?

I've tried that approach in the past and it too gets really janky and brittle. I would be the first one to admit that the monkey patching is really strong, but the objective of the gem to integrate and appear that rails-core has made the same decision to use MiniTest::Spec means it may have to happen.

No matter the outcome, I will keep this ticket open and as I find some time to play with out foundation. Maybe there is something that can be done. In the meantime, how about I assume the responsibility and include something like this in our project if Mocha is defined or part of the bundle?

require 'mocha/integration/test_unit'
module Mocha::Integration::TestUnit
  def self.activate
    true
  end
end

I use Mocha in all my projects, so this would be important for me too. Thoughts?

@metaskills
Owner

I would like to lay the blame a bit on rails-core. A ruby 2.0 compatible project requiring 'test/unit' seems like the stronger smell to me :) anyways, looking forward to fixing this both short and long term.

@floehopper

Hi Ken. Thanks for the quick response. I don't think think that adding that code snippet to minitest-spec-rails is sensible, even in the short term. I'm hoping to release a version of Mocha in the not too distant future which will allow you to manually choose whether to integrate with Test::Unit or MiniTest (this is instead of the automatic integration that happens currently). I think this change would solve the problem we've run into here, although it would probably also require a small change to Rails. In the meantime I have an idea for a small change to the Test::Unit version-detection code which would make it resilient to the minitest-spec-rails monkey-patching. Cheers, James.

@metaskills
Owner

I like the idea of explicitly setting stuff and I really appreciate the effort on your part too. Will keep this open and watch out for your other work. Thanks!

@floehopper

I've pushed an attempt at a slightly better fix in Mocha - see freerange/mocha#143 (comment)

@floehopper

Have you considered submitting pull requests to Rails to make it easier to switch to MiniTest::Spec without having to monkey-patch? In my experience they are pretty open to that kind of thing.

@metaskills
Owner

I have not since I believe what they would say is that if you want to offer a different testing solution, then I should go the bolt it on the side approach where one would use the proper railtie/engine hooks much like rspec-rails. This is the approach @blowmage first went in minitest-rails. It has generators, etc, but also does some monkey patching like mine to to work within rails default structure too. Just seems like a general bad situation all around but I am open to talking, just feel like it'll be wasted time.

@metaskills
Owner

Note to self, mocha commit: freerange/mocha@c516606

@blowmage
blowmage commented Mar 5, 2013

FWIW, currently minitest-rails creates a new TestCase that inherits from MiniTest::Spec, and includes all the relevant ActiveSupport::Testing modules.

https://github.com/blowmage/minitest-rails/blob/master/lib/minitest/rails/test_case.rb

And then it overrides ActiveSupport::TestCase with the new TestCase that inherits from MiniTest::Spec.

https://github.com/blowmage/minitest-rails/blob/master/lib/minitest/rails.rb#L17

@metaskills
Owner

Thanks Mike, I will look into that.

@metaskills
Owner

As @blowmage pointed out to me, this commit should alleviate the monkey patching.

class ActiveSupport::TestCase
  extend MiniTest::Spec::DSL
end

I will work on this very soon!

@floehopper

Thanks for letting me know. The latest Mocha release (v0.13.3) includes the fix I mentioned above.

@metaskills
Owner

James, I'm closing this ticket out. The work in the spec_dsl_module branch, notable these two commits.

Once the new minitest is released, we should be good to go. Thanks for taking the time to work with me!

@metaskills metaskills closed this Mar 9, 2013
@metaskills
Owner

I just released version 4.7.0 of minitest-spec-rails which uses the Minitest::Spec::DSL module to avoid a great deal of monkey patching.

@floehopper floehopper added a commit to freerange/mocha.methods that referenced this issue Mar 14, 2014
@floehopper floehopper Make auto-activation of Test::Unit integration more resilient.
This change is specifically to cope with the nasty re-defining of
classes that is done by the `minitest-spec-rails` gem [1]. This
confuses the Test::Unit version detection logic in Mocha and it thinks
that the Ruby 1.8 standard library version of Test::Unit is being used
when in fact there is no Test::Unit version present.

I'm not completely sure why this has reared its head [2] with Rails 4
and/or Ruby 2, and haven't checked carefully that this change is
(a) completely logical; and (b) hasn't introduced any regressions,
but it does seem to fix the problem. I'm going to push it now and try to
track down any regressions separately - I'm pretty hopeful there won't
be any.

Note that it will be possible to avoid this problem in the future
when we introduce the ability to manually activate specific test library
integration and stop using the automatic activation.

[1] metaskills/minitest-spec-rails#17
[2] freerange/mocha#143
2ab2314
@floehopper floehopper added a commit to freerange/mocha.methods that referenced this issue Mar 14, 2014
@floehopper floehopper Make auto-activation of Test::Unit integration more resilient.
This change is specifically to cope with the nasty re-defining of
classes that is done by the `minitest-spec-rails` gem [1]. This
confuses the Test::Unit version detection logic in Mocha and it thinks
that the Ruby 1.8 standard library version of Test::Unit is being used
when in fact there is no Test::Unit version present.

I'm not completely sure why this has reared its head [2] with Rails 4
and/or Ruby 2, and haven't checked carefully that this change is
(a) completely logical; and (b) hasn't introduced any regressions,
but it does seem to fix the problem. I'm going to push it now and try to
track down any regressions separately - I'm pretty hopeful there won't
be any.

Note that it will be possible to avoid this problem in the future
when we introduce the ability to manually activate specific test library
integration and stop using the automatic activation.

[1] metaskills/minitest-spec-rails#17
[2] freerange/mocha#143
43f957e
@grosser grosser pushed a commit to grosser/mocha that referenced this issue Aug 18, 2015
@floehopper floehopper Make auto-activation of Test::Unit integration more resilient.
This change is specifically to cope with the nasty re-defining of
classes that is done by the `minitest-spec-rails` gem [1]. This
confuses the Test::Unit version detection logic in Mocha and it thinks
that the Ruby 1.8 standard library version of Test::Unit is being used
when in fact there is no Test::Unit version present.

I'm not completely sure why this has reared its head [2] with Rails 4
and/or Ruby 2, and haven't checked carefully that this change is
(a) completely logical; and (b) hasn't introduced any regressions,
but it does seem to fix the problem. I'm going to push it now and try to
track down any regressions separately - I'm pretty hopeful there won't
be any.

Note that it will be possible to avoid this problem in the future
when we introduce the ability to manually activate specific test library
integration and stop using the automatic activation.

[1] metaskills/minitest-spec-rails#17
[2] freerange#143
c516606
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment