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

Favor ActiveSupport::TestCase over Minitest::Test #1385

Merged
merged 1 commit into from
Dec 30, 2015

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Dec 20, 2015

  • Better minitest 4/5 support
  • Better DSL
  • Already available with no changes
  • Consistent interface

- Better minitest 4/5 support
- Better DSL
- Already available with no changes
- Consistent interface
@beauby
Copy link
Contributor

beauby commented Dec 27, 2015

What would be the drawbacks?

@bf4
Copy link
Member Author

bf4 commented Dec 28, 2015

@beauby I have no idea what the drawback would be. I do know that at one point AS::TC was removed in favor of raw minitest. Perhaps speed?

@beauby
Copy link
Contributor

beauby commented Dec 29, 2015

After investigating a bit, it appears ActiveSupport::TestCase is just an extension of Minitest::Test, so this PR should not be an issue. I'll merge it tomorrow unless somebody raises their voice (cc @spastorino).

@spastorino
Copy link
Contributor

It's ok to merge this, when I was building AMS 0.10 I used Minitest::Test just because I didn't need AS::TC and wanted to depend as minimum as possible on Rails stuff.

@beauby
Copy link
Contributor

beauby commented Dec 30, 2015

Great, thanks for the heads up @spastorino. As 0.10 is already depending on quite a bit of rails stuff, I don't believe this PR will hurt much, so I'm merging.

beauby added a commit that referenced this pull request Dec 30, 2015
Favor ActiveSupport::TestCase over Minitest::Test
@beauby beauby merged commit 7bc66c5 into rails-api:master Dec 30, 2015
@bf4 bf4 deleted the remove_raw_minitest_test branch August 31, 2016 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants