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

Already on GitHub? Sign in to your account

Master minitest5 #901

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants

voxik commented Jul 21, 2014

These are changes necessary to migrate the testsuite to Minitest 5.x (related to #865 and #867).

The only remaining caveat is Test::Unit::TestSuite in test/contest.rb. I am not sure what was it good for, but the test suite is passing without it, so I left it commented out. May be it should be removed entirely?

Member

kytrinyx commented Jul 21, 2014

All of context becomes irrelevant if we move to minitest, I believe.

We should definitely not be requiring test/unit anymore, and an empty minitest suite doesn't fail by default. The remaining setup/context/teardown etc would only be necessary if we were using test unit.

I think the whole file should be deleted.

@rkh, @zzak - sanity check me on this?

Member

kytrinyx commented Jul 21, 2014

Oh, uhm. Failing build. I think it's actually failing because of the test/contest.rb file.

voxik commented Jul 21, 2014

Hmm, you are right, it fails on older Rubies. I was testing just against 2.1.2 and that worked just fine.

voxik commented Jul 21, 2014

Simple replacement of require 'contest' by require 'minitest/autorun' does not work. It complains for missing context and missing setup. This is probably above knowledge I'd like to have about Sinatra's test suite :-(

Member

kytrinyx commented Jul 21, 2014

I'll pull down your branch after work and muck around with it.

Minitest has setup. I'm not sure where we're using context.

Owner

zzak commented Aug 11, 2014

Rebuilding but LGTM: https://travis-ci.org/sinatra/sinatra/builds/30458494

If we can get it green I will merge.

@zzak zzak commented on the diff Aug 11, 2014

@@ -33,7 +33,7 @@ if RUBY_ENGINE == 'jruby'
end
if RUBY_ENGINE == "ruby" and RUBY_VERSION > '1.9.2'
@zzak

zzak Aug 11, 2014

Owner

I think we have to change this and require minitest on 1.8 as well, does minitest 5 run on ruby 1.8?

Member

kytrinyx commented Aug 11, 2014

Minitest does run on ruby 1.8, as long as you require "rubygems" first.:

require 'rubygems'
require 'minitest/autorun'

class TinyTest < Minitest::Test
  def test_something
    assert_equal 1, 2
  end
end
$ ruby -v
ruby 1.8.7 (2012-02-08 patchlevel 358) [universal-darwin12.0]
$ ruby tiny_test.rb
# Running:

F

Finished in 0.136810s, 7.3094 runs/s, 7.3094 assertions/s.

  1) Failure:
TinyTest#test_something [tini_test.rb:6]:
Expected: 1
  Actual: 2

mikesea commented Aug 27, 2014

I was able to get the test suite passing locally on jruby, 1.8.7, and 2.1.2 by moving gem "minitest", "~> 5.0" outside the RUBY_ENGINE conditionals. You can see my patch here: mikesea/sinatra@bee981a

Happy to submit this as a separate PR if you'd like.

voxik commented Aug 27, 2014

@mikesea So if I can guess, you ran the test suite using minitest 4.x at the end ...

mikesea commented Aug 28, 2014

@voxik no, I ran it with minitest 5.x for each ruby version, with a regenerated Gemfile.lock each time.

voxik commented Aug 28, 2014

@mikesea Ah sorry, I read wrongly your patch :)

Contributor

vipulnsward commented Sep 21, 2014

@voxik I was able to make this work making these change - https://gist.github.com/vipulnsward/45b2f8d2e50c254dc390

Could you update this PR. Would love to see it get merged.

Contributor

vipulnsward commented Sep 21, 2014

Oh, also it needs a rebase.

Contributor

vipulnsward commented Jan 10, 2015

@voxik are you still interested to pursue this? I would love to wrap this up with your commits.

voxik commented Jan 10, 2015

I am still interested in this ... unfortunately I am currently busy with different tasks :/

Owner

zzak commented Jan 21, 2015

@voxik Thank you for the patch. I've merged in @vipulnsward's work from #960 since it was rebased and passing. Sorry for the duplicate work but I really appreciate both of your contributions!!! <3 <3 <3

@zzak zzak closed this Jan 21, 2015

voxik commented Jan 21, 2015

Thanks. I'm happy to see this resolved :)

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