semaperepelitsa / spork-minitest

MiniTest runner for Spork
MIT License
13 stars 25 forks source link

Respect minitest/pride if it was loaded. #5

Open nuxlli opened 12 years ago

nuxlli commented 12 years ago

Replicate finxing.

Issue: https://github.com/sporkrb/spork-testunit/pull/30 And fixing: https://github.com/semaperepelitsa/spork-testunit/commit/92495b3fa3eedb8fb42dd8e1be5158e741ad960f

rmm5t commented 11 years ago

+1 This is much needed, especially adding support for the command line arguments. A must if you also use guard-minitest.

semaperepelitsa commented 11 years ago

I want this project to be a very thin layer above Minitest and this is a key difference from spork-testunit. Guard-minitest originally targeted spork-testunit hence it used these options. But they are not really doing anything useful except configuring Growl. It can be set up in the test helper as fine.

I think guard-minitest should just omit these options if notification is disabled and the issue is gone.

if notify?
  cmd_parts << "-r #{File.expand_path('../runners/default_runner.rb', __FILE__)}"
  cmd_parts << "-e '::GUARD_NOTIFY=#{notify?}'"
end
semaperepelitsa commented 11 years ago

And I don't have a solution for minitest/pride. Certainly, I'm not bringing this mess here. (Yes, I'm aware that this was my pull request :-)

rmm5t commented 11 years ago

Disabling the ability to do notifications with guard seems to completely miss the point of using guard in the first place. Notifications are what make Guard so useful.

While, I'd prefer to see these testdrb arguments parsed by OptionParser (require "optparse"), @nuxill's first stab at this enables spork-minitest to be used with guard-minitest. While I understand the desire to keep a very thin layer above Minitest, I don't think it's such a bad thing to enable some parity with spork-testunit to ensure a level of functionality that allows spork-minitest to be used in cool ways.

Let's be honest too. While spork is a great thing, tools like guard are really what make spork shine.

Aside, what was wrong with @nuxlli's solution for supporting minitest/pride?

semaperepelitsa commented 11 years ago

I'm totally not against notifications, they are great. What I was trying to say is that you can setup them in the test_helper directly without those clunky options passed to testdrb:

require "guard/minitest/runners/default_runner"
GUARD_NOTIFY = true

Spork-testunit parsed all options itself and did not pass them to MiniTest. I don't want that. I want all options to be passed through to MiniTest Unit runner and let it interpret them. This is a feature.

You can write a custom MiniTest::Unit subclass to support any additional options but this does not belong here.

Having said that, guard-minitest integration would be very nice. But I think it should be done in a different way.

rmm5t commented 11 years ago

@semaperepelitsa Fair arguments. In the end, I'd really like to see guard-mintest and spork-minitest play nice together. Right now, they do not.

It would also be nice if guard-minitest and spork-minitest could co-exist without having to add a few lines of configuration to every project that use both of them, but getting the two working together at all would be an excellent first step. I think we agree that guard-minitest could use some cleanup.

sbleon commented 11 years ago

This pull request on guard-minitest makes it compatible with spork, but does break notifications. https://github.com/guard/guard-minitest/pull/42

Please +1 it if it solves this problem for you, or suggest some modifications to keep notifications working if that's important to you.

zenspider commented 11 years ago

"Spork-testunit parsed all options itself and did not pass them to MiniTest. I don't want that. I want all options to be passed through to MiniTest Unit runner and let it interpret them. This is a feature."

I think this is the crux of the issue and certainly needs to be addressed.

semaperepelitsa commented 11 years ago

@zenspider what do you suggest?

martco commented 11 years ago

@nuxlli thank you for this. it's a shame that @semaperepelitsa seems to be sucking his thumb on this one. i'm going to try it out locally regardless. cheers!