Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split expect_with :stdlib into expect_with :test_unit and expect_with :minitest #1466

Merged
merged 8 commits into from Apr 24, 2014

Conversation

cupakromer
Copy link
Member

@cupakromer cupakromer commented Apr 18, 2014

Follow up to #1462.

@cupakromer
Copy link
Member

cupakromer commented Mar 31, 2014

I'm up for tackling this one this week if no one else wants it.

@myronmarston
Copy link
Member Author

myronmarston commented Mar 31, 2014

I'm up for tackling this one this week if no one else wants it.

All yours :).

@myronmarston
Copy link
Member Author

myronmarston commented Apr 18, 2014

Thanks for tackling this, @cupakromer. Taking a look now.

BTW, github didn't notify me you had this ready, so in the future feel free to ping us to let us know you've got something ready when you convert an issue to a PR.

* minitest assertions in ruby 1.9
* rspec/expectations _and_ stlib assertions
* test/unit assertions in ruby 1.8
* minitest assertions in ruby 1.9
Copy link
Member

@myronmarston myronmarston Apr 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use :test_unit or :minitest on any version of ruby. (The version was just about which was included in the standard library, but both are available as gems that can be installed on all versions of ruby).

Copy link
Member Author

@cupakromer cupakromer Apr 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll remove.

@myronmarston
Copy link
Member Author

myronmarston commented Apr 18, 2014

Solid work, @cupakromer!

@myronmarston
Copy link
Member Author

myronmarston commented Apr 18, 2014

The other thing this'll need is a deprecation warning in 2.99. Actually, to deprecate this properly I think you have to backport this (but keep :stdlib) so that people can switch from :stdlib to one of the other options in 2.99 when they get the warning, then continue upgrading. Hopefully that backport will be easy...

@cupakromer
Copy link
Member

cupakromer commented Apr 18, 2014

@myronmarston thanks for taking a look. I had a branch up for a bit, but never attached it until last night. It was late when I finished pushing and got busy with things this morning. So you're right on top of it.

@cupakromer
Copy link
Member

cupakromer commented Apr 18, 2014

I'll make a new PR for the backport to 2.99 later this afternoon.

end
rescue NameError => _ignored
# No-op. Minitest::Assertions isn't loaded
end
Copy link
Member

@myronmarston myronmarston Apr 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just needed to provide the assertions attribute for the test-unit cases that need it? If so, I think it's less hacky/simpler to just copy over this bit of the minitest adapter:

      attr_writer :assertions
      def assertions
        @assertions ||= 0
      end

It shouldn't cause any problems to have that attribute defined even if it's not used. Thoughts?

Copy link
Member Author

@cupakromer cupakromer Apr 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about doing this. I agree that it doesn't necessarily hurt to have it duplicated here too. My reasoning for not taking that route was:

  • this isn't coincidental duplication; we're duplicating these lines solely because minitest is being loaded
  • we don't always load minitest, so why have the extra API surface area
  • if more dependencies are required by minitest in the future we need to make sure to add them in two places; I know I personally would forget to add it to 'test_unit' if that were to happen
  • the code in this manner is fairly self documenting, I'd feel odd duplicating the comments about the minitest issue again here
  • I considered making a minitest_assertions_hooks module, which I could include in both; though part of me though this was slightly better documenting

With all that having been said. Neither of these modules have specs around them. I probably easily could just use a shared_examples_for "provides minitest hooks" to help add the documentation.

Honestly, I'm on the fence about my approach and figured I'd make sure it passed on all versions and then solicit feedback.

Copy link
Member

@myronmarston myronmarston Apr 21, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an alternate idea (that I'm just tossing out there but don't necessarily recommend): define a subclass of Module that provides the assertions accessor and accepts an argument for what additional assertion module to include:

module RSpec
  module Core
    class AssertionAdapter < Module
      def initialize(assertion_module)
        module_exec do
          include assertion_module

          attr_writer :assertions
          def assertions
            @assertions ||= 0
          end
        end
      end
    end
  end
end

Then the :test_unit option will be translated to RSpec::Core::AssertionAdapter.new(::Test::Unit::Assertions) and the :minitest option will be translated to RSpec::Core::AssertionAdapter.new(::Minitest::Assertions).

I'm on the fence about which solution is best, honestly. This idea is potentially confusing (subclassing Module is pretty non-standard) but it feels like the right kind of abstraction for this -- the adapter is it's own thing, not tied to test_unit or minitest and can receive any assertion module as an argument.

If you feel like switching to this feel free, otherwise I'm fine merging what you have. You clearly thought about this a lot and you're thinking about the right things :).

@myronmarston
Copy link
Member Author

myronmarston commented Apr 21, 2014

This LGTM. Left a couple comments but they aren't merge blockers. Thanks @cupakromer!

@myronmarston
Copy link
Member Author

myronmarston commented Apr 24, 2014

One other thing I just remembered...we need a 2.99 deprecation for expect_with :stdlib since it will no longer be supported in 3.0. Would you like me to take care of that or are you on it, @cupakromer?

@cupakromer
Copy link
Member

cupakromer commented Apr 24, 2014

@myronmarston yep, have it open right now in fact. Will make final changes soon then merge this, and should have the 2.99 push in an hour or so.

cupakromer added 7 commits Apr 24, 2014
We're not sure yet what minitest 6.x will entail. So we're locking the
gem to a 5.x version.
Mark feature as `@wip` so it doesn't fail the build while the feature is
being implemented.
- Remove support for the `stdlib` expectation framework
- Include `test_unit`
- Include `minitest`
- In specs treat previous `stdlib` as `minitest`
- Attempt to require the 'minitest' gem
  - Supports minitest with Ruby 1.8.7
  - Supports minitest 5.x which is not part of stdlib
The whole minitest and test/unit loading is a bit complex. It depends
greatly on which Ruby version is being used, and if either the
'test_unit' or 'minitest' gems are available.

The newer Ruby versions (1.9+) use test/unit as a light shim around
minitest. Due to this, they load minitest when test/unit is loaded. This
means we should add the new `assertions` accessors when `:test_unit` is
specified.

However, if either Ruby 1.8 is used or the 'test_unit' gem was loaded,
'minitest' will not be loaded. In these situations we should not be
adding the accessor.
RSpec 3.x does not provide explicit support for Minitest 4.x. However,
due to differences in `require` paths, we do provide a fallback option
in the case it is used. We are not in the business of dictating which
versions our users should or should not use, nor on how they setup their
load paths (i.e. bundler or other methods).

There are a lot of subtle issues with the different combinations of
versions of Ruby + test/unit + minitest. These can cause load and name
errors.

- Update the features to specifically check for our desired assertion
  errors; instead of allowing failures due to `NameError` or `LoadError`
- Update the test/unit and minitest adapters to include fallback logic
  when different combinations occur
cupakromer added a commit that referenced this issue Apr 24, 2014
Split `expect_with :stdlib` into `expect_with :test_unit` and `expect_with :minitest`
@cupakromer cupakromer merged commit 892c66a into master Apr 24, 2014
@myronmarston myronmarston deleted the split-expect_with-stdlib branch Apr 24, 2014
# RUBY_VERSION we need to check ancestors.
begin
# MiniTest is 4.x
# Minitest is 5.x
Copy link
Member

@JonRowe JonRowe Apr 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment makes no sense, it's also present in the 2.99 deprecation.

Copy link
Member Author

@cupakromer cupakromer Apr 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which part? There's a difference in the upcase on Test between minitest 4.x and 5.x. Would this be a bit more explicit?

Loading `test/unit/assertions` sometimes also loads a
version of minitest:

- Ruby 1.8 does not load any minitest files

- the test/unit gem does not include any minitest files

- Ruby core 1.9+ test/unit/assertions includes
  `MiniTest::Assertions`. Note the upcasing of 'Test' in
  'MiniTest'. It loads this by requiring 'minitest/unit'.

- If Minitest 5.x is loaded when Ruby core 1.9+ requires
  'minitest/unit', the gem sets 'MiniTest' to an alias of it's
  'Minitest'

Minitest 5.x added new dependencies to 'Minitest::Assertions';
note the lowercasing of 'test' in 'Minitest' now. Only if this
module is loaded do we need to add a shim for these new
dependencies.

Copy link
Member

@JonRowe JonRowe Apr 25, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh the the casing is different, prehaps just amending these two lines to:

# 4.x uses the `MiniTest` constant (note capitalisation) 
# 5.x uses the `Minitest` constant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants