Fix ExpectationNotMetError superclass and update T::U/minitest integration #426

Merged
merged 3 commits into from Jan 22, 2014

Conversation

Projects
None yet
2 participants
@myronmarston
Member

myronmarston commented Jan 17, 2014

This fixes #425.

I still need to add deprecation warnings to 2.99 for the removal of T::U/minitest 4.x integration.

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jan 18, 2014

Member

So I'm trying to decide what deprecation warnings, if any, to add to 2.99. Considerations:

  • I can't figure out how to provide a warning that is silenced by the user integrating themselves. We mix RSpec::Matchers in at load time here. For the user to integrate themselves, they need to manually mix RSpec::Matchers in to their test base class...but they have to load RSpec::Matchers first in order to do that,
    at which time we detect the presence of the T::U/minitest constants and perform the deprecated integration.
  • The presence of the minitest constant may not be an indication that the user is actually using rspec-expectations with minitest. Consider the case of using rspec to test a rails app: modern rails versions load minitest, right? But the user may not be defining or running any tests via minitest, and issuing a deprecation warning in this case would be confusing.
  • Is anyone actually using rspec-expectations with T::U or minitest? (I've never heard of anyone using it). Given that, I'm not sure it's worth the effort to figure out a solution. Also, I care much more about providing all the deprecations for users who use all of rspec than for users who use just one of the gems.

So...anyone have thoughts on how we could do the deprecations or if it would even be worth the effort?

/cc @alindeman @JonRowe @xaviershay @samphippen @soulcutter

Member

myronmarston commented Jan 18, 2014

So I'm trying to decide what deprecation warnings, if any, to add to 2.99. Considerations:

  • I can't figure out how to provide a warning that is silenced by the user integrating themselves. We mix RSpec::Matchers in at load time here. For the user to integrate themselves, they need to manually mix RSpec::Matchers in to their test base class...but they have to load RSpec::Matchers first in order to do that,
    at which time we detect the presence of the T::U/minitest constants and perform the deprecated integration.
  • The presence of the minitest constant may not be an indication that the user is actually using rspec-expectations with minitest. Consider the case of using rspec to test a rails app: modern rails versions load minitest, right? But the user may not be defining or running any tests via minitest, and issuing a deprecation warning in this case would be confusing.
  • Is anyone actually using rspec-expectations with T::U or minitest? (I've never heard of anyone using it). Given that, I'm not sure it's worth the effort to figure out a solution. Also, I care much more about providing all the deprecations for users who use all of rspec than for users who use just one of the gems.

So...anyone have thoughts on how we could do the deprecations or if it would even be worth the effort?

/cc @alindeman @JonRowe @xaviershay @samphippen @soulcutter

@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Jan 19, 2014

Member

Could we not issue a deprecation when the old file is required or the module included? I'm in favour of adding a deprecation but I don't think we should stress over it too much...

Member

JonRowe commented Jan 19, 2014

Could we not issue a deprecation when the old file is required or the module included? I'm in favour of adding a deprecation but I don't think we should stress over it too much...

@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jan 19, 2014

Member

Could we not issue a deprecation when the old file is required or the module included? I'm in favour of adding a deprecation but I don't think we should stress over it too much...

The old file is required automatically by rspec-expectations when you load the gem:

require 'rspec/matchers/test_unit_integration'

That means that:

  • We can't issue a deprecation if the old file is required (as it is always required)
  • There's no way for the user to include the module before that file is loaded, because to include the module they have to load rspec-expectations so the module is defined.
Member

myronmarston commented Jan 19, 2014

Could we not issue a deprecation when the old file is required or the module included? I'm in favour of adding a deprecation but I don't think we should stress over it too much...

The old file is required automatically by rspec-expectations when you load the gem:

require 'rspec/matchers/test_unit_integration'

That means that:

  • We can't issue a deprecation if the old file is required (as it is always required)
  • There's no way for the user to include the module before that file is loaded, because to include the module they have to load rspec-expectations so the module is defined.
@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Jan 19, 2014

Member

What about using the setup/teardown hooks to issue the deprecation?

Member

JonRowe commented Jan 19, 2014

What about using the setup/teardown hooks to issue the deprecation?

myronmarston added some commits Jan 15, 2014

Improve integration with minitest.
- Support minitest 5.x rather than 4.x.
- Make expectations count as assertions in minitest's runner.
- Make expectation failures report as failures, not errors.
- Stop auto-integrating; users can require a file instead.
  I think this fits better with minitest's philosophy, and
  there are situations (such as in a rails app) where minitest
  is loaded, but the runner is not being used.
- Stop integrating with test/unit. T::U doesn't appear to be
  used much these days. Users can still integrate it themselves
  (by manually mixing in `RSpec::Matchers`). Stdlib T::U is not
  usable while minitest 5.x is installed, so there's not an easy
  way to run builds against T::U while we also run builds against
  minitest 5.x.
@myronmarston

This comment has been minimized.

Show comment Hide comment
@myronmarston

myronmarston Jan 22, 2014

Member

What about using the setup/teardown hooks to issue the deprecation?

Thanks for the idea. This does indeed work; see #426.

Do you have any feedback about this PR or is it good to merge?

Member

myronmarston commented Jan 22, 2014

What about using the setup/teardown hooks to issue the deprecation?

Thanks for the idea. This does indeed work; see #426.

Do you have any feedback about this PR or is it good to merge?

JonRowe added a commit that referenced this pull request Jan 22, 2014

Merge pull request #426 from rspec/fix-errors
Fix ExpectationNotMetError superclass and update T::U/minitest integration

@JonRowe JonRowe merged commit 2b0efe7 into master Jan 22, 2014

1 check passed

default The Travis CI build passed
Details
@JonRowe

This comment has been minimized.

Show comment Hide comment
@JonRowe

JonRowe Jan 22, 2014

Member

LGTM :)

Member

JonRowe commented Jan 22, 2014

LGTM :)

@JonRowe JonRowe deleted the fix-errors branch Jan 22, 2014

@adsteel adsteel referenced this pull request in mojotech/capybara-ui Jul 20, 2015

Closed

Eventually does not work with RSpec expect statements #102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment