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

Simplifying the DRb version of the runner #41

Merged
merged 3 commits into from Feb 21, 2013

Conversation

chadoh
Copy link
Contributor

@chadoh chadoh commented May 1, 2012

We're using guard-minitest with spork-minitest. Spork works when running just spork and then using testdrb to run individual files, but guard-minitest was broken. It kept saying

Running tests with args ["-r", "/Users/costro0001/.rvm/gems/ruby-1.9.2-p180-patched/gems/guard-minitest-0.5.0/lib/guard/minitest/runners/default_runner.rb", "-e
", "::GUARD_NOTIFY=true", "test/test_helper.rb", "./test/unit/raw_site_test.rb"]...
Exception encountered: #<LoadError: no such file to load -- -r>

It turns out that the implementation of testdrb in spork-minitest does not allow the -r or -e options in the way guard-minitest had been passing them in. Additionally, I found that the inclusion of test_helper/spec_helper was also unnecessary, as these files were already being included in the individual test that was being run.

I'm not sure why all of this extra machinery was in place. It seems strange that I just removed it. Let me know what you think.

…estdrb implementation in spork-minitest (not sure what the other one was set up for)
@chadoh
Copy link
Contributor Author

chadoh commented May 1, 2012

Oh, and yes, I'm aware that I probably "broke" the Growl notifications as a consequence of this. My team had not been using that, anyhow, so we don't mind. I don't really have a whole lot of time to play around with it, so I just wanted to get it working quickly.

@rmm5t
Copy link
Contributor

rmm5t commented Sep 22, 2012

I think this pull-request over on spork-minitest might be a better solution to the underlying problem.

semaperepelitsa/spork-minitest#5 specifically nuxlli/spork-minitest@ac59be9

@rking
Copy link

rking commented Dec 6, 2012

@rmm5t Why do we need those options supported?

The testrunner works fine without being explicitly listed, and the notification is configurable just fine through other means.

I vote 👍 on 5a7f3c1

@rmm5t
Copy link
Contributor

rmm5t commented Dec 6, 2012

@rking Originally, I felt this way, because this pull-request breaks notification support, but after the discussion over at semaperepelitsa/spork-minitest#5, I agree now that it would be better to just simplify guard-minitest as described in the pull-request here.

@zenspider
Copy link

I agree and I like this PR.

@sbleon
Copy link
Member

sbleon commented Dec 12, 2012

+1

@rmm5t
Copy link
Contributor

rmm5t commented Jan 4, 2013

Any chance of this PR getting pulled in anytime soon? I've confirmed that this change combined with spork-minitest 1.0.0.beta2 both make things a little more friendly between the two gems.

@rking
Copy link

rking commented Jan 4, 2013

I just got bored and pushed the guard-sporkminitest gem.

It doesn't even have a 'drb' option, or any other cruft.

I use it indirectly, by just saying:

Gemfile

group :development do gem 'working' end

Guardfile

require 'working/guard'

test/test_helper.rb

require 'working/test_helper'
# Spork.prefork doesn't like when this line is missing 

I'm ready to support this 100%, so let me know with Github Issues or whatever.

@rking
Copy link

rking commented Jan 4, 2013

Also I released working-rails, that uses guard-minispork for Rails stuff.

All three of these gems need polish, but they currently work for me.

@rymai
Copy link
Member

rymai commented Feb 20, 2013

Hi guys!

Thanks for the discussion, strangely I didn't receive any notification about it (not sure how notifications are received for organization's repos...)!

@chadoh Could you rebase on master so I can merge it in?

Thanks!

@rymai rymai mentioned this pull request Feb 21, 2013
@chadoh
Copy link
Contributor Author

chadoh commented Feb 21, 2013

Haha, I'm pleased that my 10-month-old pull request will finally be merged in! Unfortunately, I no longer work at the organization in which I used this, and I no longer have access to the repo. (@chrisledet! You might be the most likely of all my @itrcdevs comrades to want to help. These folks need my pull request rebased in order to accept it.)

@rymai
Copy link
Member

rymai commented Feb 21, 2013

Ok cool! Or I can merge it locally (the downside it that the commit will be from me instead of you...)?

Conflicts:
	lib/guard/minitest/runner.rb
	spec/guard/minitest/runner_spec.rb
@chadoh
Copy link
Contributor Author

chadoh commented Feb 21, 2013

Haha, @chrisledet's merge undid all my changes. 😦 He's given me access to the repo again; I'll fix it up at some point today.

@chadoh
Copy link
Contributor Author

chadoh commented Feb 21, 2013

Ok, it's obviously been a while and I'm no longer using this, but it looks like I redid all the original changes after @chrisledet helpfully merged master in.

@rymai
Copy link
Member

rymai commented Feb 21, 2013

Awesome!

rymai added a commit that referenced this pull request Feb 21, 2013
Simplifying the DRb version of the runner
@rymai rymai merged commit 3dbc1ce into guard:master Feb 21, 2013
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

6 participants