#88: Do not force minitest loading when test-unit is available #181

Closed
wants to merge 3 commits into from

3 participants

@os97673

It looks like fix for #88 was removed by the very next change in assertion_error.rb (27b8b78)
So, here is a re-fix for the problem described in #88 (we should not load minitest if test-unit is already available)

@drapergeek
thoughtbot, inc. member

Wouldn't this be a better solution?

    if defined?(Test::Unit::AssertionFailedError)
      AssertionError = Test::Unit::AssertionFailedError
    elsif Gem.ruby_version >= Gem::Version.new('1.8') && Gem.ruby_version < Gem::Version.new('1.9')
      require 'test/unit'
      AssertionError = Test::Unit::AssertionFailedError
    elsif Gem.ruby_version >= Gem::Version.new("1.9")
      require 'minitest/unit'
      AssertionError = MiniTest::Assertion
    else
      raise "No unit test library available"
    end

This way if TestUnit is already loaded, we just use it, otherwise we load it and use it.

@os97673

@drapergeek
Personally, I do not see big difference :)

I've placed condition in the order which corresponds to "for 1.8 always use test-unit, for 1.9 use test-unit if it is already loaded, and use minitest otherwise", the order you suggest implements different wording/logic and has the same result.
So, if you believe that your wording is better or we will get significant performance improvement with the order you suggest I'm ready to change the order, but I, personally, see no reasons to do this.

@os97673

Just to clarify current situation: do I need to change something in my pull request or it is ok and it is not merged yet because of lack of committers' time?

@drapergeek
thoughtbot, inc. member

I would like to get a second set of eyes on this before making a call, @gabebw, thoughts?

@gabebw gabebw commented on an outdated diff Nov 21, 2012
lib/shoulda/matchers/assertion_error.rb
@@ -3,6 +3,9 @@ module Matchers
if Gem.ruby_version >= Gem::Version.new('1.8') && Gem.ruby_version < Gem::Version.new('1.9')
require 'test/unit'
AssertionError = Test::Unit::AssertionFailedError
+ elsif defined?(Test::Unit::AssertionFailedError)
+ # It looks like we already have what we need ;)
@gabebw
thoughtbot, inc. member
gabebw added a note Nov 21, 2012

Please change this to something more descriptive like "Something already loaded Test::Unit"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gabebw
thoughtbot, inc. member

1 comment - otherwise looks good. We should extract this logic to a class (AssertionErrorFinder?) since it's getting very complicated.

@os97673

Comment has been changed.

@os97673

@drapergeek @gabebw

do I need to do anything else to make the request acceptable?

@os97673

@drapergeek @gabebw
any chance to get the request merged?

@gabebw
thoughtbot, inc. member

Just merged this in 68ae500, thanks for reminding me.

@gabebw gabebw closed this Jan 3, 2013
@os97673

Thank you

@os97673 os97673 deleted the unknown repository branch Jan 3, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment