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

don't require test/unit for compatibility with minitest 5.0 #456

Closed
wants to merge 1 commit into from

Conversation

levinalex
Copy link

cucumber required test/unit in disable_mini_and_test_unit_autorun.rb.
This breaks compatibility with minitest >= 5.0 and leads to the error described in minitest/minitest#283

there's an example that demonstrates the incompatibility at https://gist.github.com/levinalex/772817a7d453addf4b91

Why did the require "test/unit" exist in the fist place? Removing it does not seem to break any tests.

test/unit is not compatible with minitest ~> 5.0
@mattwynne
Copy link
Member

I don't think there are any tests for this file - it's too complicated to set up the different contexts for it, and quite low value / impact IMO.

The point of the require statement is so that those constant checks will work to figure out which version of Test::Unit is in play. If test/unit isn't required, we won't run any of that code, as far as I can see.

I'm not sure what's the right thing to do here. I'm tempted just to rip the file out entirely.

@kou @zenspider any suggestions?

@zenspider
Copy link

I don't really have an opinion on the mechanics of this, but you might as well remove the begin wrapper entirely.

@kou
Copy link

kou commented May 16, 2013

I think that the patch's approach is reasonable. It means that "If test/unit is already required, we disable auto runner of it. If is it not required yet, we do nothing".

It seems that elsif defined?(Test::Unit::Runner) clause should be updated for minitest >= 5.

@alindeman
Copy link

I'm interested in seeing Minitest 5.0 support in cucumber. Right now, we can't run rspec-rails' full suite against Rails master (Rails 4.1).

@tooky
Copy link
Member

tooky commented May 19, 2013

This patch still causes a warning when we load assertions in rb_support/rb_language.rb.

I think that cucumber is trying too hard to figure out the right thing to do for the various testing frameworks automagically.

I would prefer to isolate the code for each test framework into a separate require e.g.

require 'cucumber/rspec'
# or...
require 'cucumber/minitest'
# etc..

The user can then choose which framework they want to use. It also provides a simple way for others to add support for new test frameworks should they appear?

@mattwynne
Copy link
Member

@tooky the problem this particular file is trying to solve is where a (usually Ruby novice) user has accidentally required test/unit in their Cucumber test run, then is surprised to see the Test::Unit at_exit hook fire after their cukes have run and try to run unit tests (usually throwing an error about a lack of tests).

However plugins would help for the assertions behaviour specified here: https://github.com/cucumber/cucumber/blob/master/features/docs/defining_steps/assertions.feature

@mattwynne
Copy link
Member

@tooky I've been doing some testing on this today. It looks like things are simpler than we think, perhaps since we dropped Ruby 1.8.7.

See http://github.com/cucumber/multi_test

I'd like to have Cucumber delegate to MultiTest for this functionality. That way it's tested but we don't have to run those tests for Cucumber's main build. I'd also like to move the automatic assertions module loading into MultiTest.

WDYT?

@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants