make "spring test" support any number of arguments #102

Merged
merged 2 commits into from Apr 12, 2013

Projects

None yet

2 participants

aspiers commented Mar 17, 2013

This obsoletes #101.

spring test needs to accept multiple arguments if guard-minitest is to ever have a chance of supporting spring. Specs are automatically pruned from the command-line with a warning, since loading specs will stop the unit tests from running.

It's also useful for it to support zero arguments, in which case it does the sensible thing and defaults to running all unit tests. Additionally, this is consistent with the behaviour of spring rspec.

aspiers commented Mar 17, 2013

Hold tight - I'm working on tests for this ...

aspiers commented Mar 17, 2013

Weird, Ruby 2.0 doesn't like my use of Enumerable#partition :-( Looking into it ...

aspiers commented Mar 17, 2013

Ah, run_test was a bad choice of helper method :-) Fixing ...

aspiers commented Mar 17, 2013

guard/guard-minitest#57 adds support for spring based off this pull request.

@jonleighton jonleighton and 1 other commented on an outdated diff Mar 22, 2013
lib/spring/commands.rb
@@ -75,17 +75,27 @@ def setup
end
def call(args)
- if args.size > 0
- ARGV.replace args
- path = File.expand_path(args.first)
+ if args.empty?
+ args = [ 'test' ]
jonleighton
jonleighton Mar 22, 2013 Member

please use ['test'] to be consistent with the existing code.

aspiers
aspiers Mar 22, 2013

Sure, will adjust.

@jonleighton jonleighton and 1 other commented on an outdated diff Mar 22, 2013
lib/spring/commands.rb
@@ -75,17 +75,27 @@ def setup
end
def call(args)
- if args.size > 0
- ARGV.replace args
- path = File.expand_path(args.first)
+ if args.empty?
+ args = [ 'test' ]
+ end
+
+ # If a spec gets required here, it will stop all the test-unit
+ # tests running for some strange reason, so we need to watch
+ # out for that.
+ specs, ok_paths = *args.partition { |path| path =~ %r{(^|/)spec(/|$)} }
+ if ! specs.empty?
+ $stderr.puts "WARNING: spring test does not work on specs; skipping:"
+ specs.each { |path| $stderr.puts " #{path}" }
+ end
jonleighton
jonleighton Mar 22, 2013 Member

I don't want to have a hack like this in here. The test command shouldn't need to know anything about what "specs" are.

aspiers
aspiers Mar 22, 2013

It's not a hack; it's simple argument validation in order to improve usability. It saves users from the very easy mistake of including specs in their spring test invocation via an over-zealous glob or a misconfigured Guardfile. If that mistake wasn't so disruptive then this wouldn't be needed, but the fact that including even a single spec prevents all the test-unit tests from running caused me to waste about 2 hours trying to figure out why they weren't running. I really don't want other users to go through the same suffering.

jonleighton
jonleighton Mar 22, 2013 Member

Does your project use both specs and test/unit tests? Or did you try to use "spring test" to run specs? What happened? Did you find the root cause of the issue? How do you define a "spec"? I.e. does this issue affect "minitest/spec" specs or just rspec ones? If it doesn't affect minitest/spec specs, won't this prevent users who have minitest/spec specs in the spec/ directory from running them?

I'm in favour of preventing users hitting problems where possible, but I think this is very kludgy and could cause other problems. So I'm keen to explore alternatives first.

aspiers
aspiers Mar 22, 2013

Yes, I am working on two projects which use both specs and test/unit tests. I don't think this is uncommon. The root cause in my case was a misconfigured Guardfile. I didn't try it with minitest/spec specs (since I didn't even know they existed ...) but that's a valid concern you raise. I'm more than happy to change the check to something safer - any ideas?

jonleighton
jonleighton Mar 22, 2013 Member

I'd still like you to describe the issue first so I can understand it a bit better.

aspiers
aspiers Mar 23, 2013

This is a simple test case:

$ bin/spring test ./test/unit/person_test.rb ./spec/models/person_spec.rb
# Run options: --seed 28283

# Running tests:



Finished tests in 0.068785s, 334.3730 tests/s, 0.0000 assertions/s.

23 tests, 0 assertions, 0 failures, 23 errors, 0 skips

Total meltdown, with no explanation. This could be triggered by a buggy Guardfile, e.g.

# ... other definitions snipped ...

def all
  all_testunit + all_specs
end

def cucumber_guards
  watch(%r{^spec/support/(.+)\.rb$}) { all }
end

group :minitest do
  guard 'minitest', guard_opts do
    startup_guards { all_testunit }
    test_unit_guards
    all
  end
end

group :rspec do
  guard 'rspec', guard_opts do
    startup_guards { all_specs }
    rspec_guards
    all_specs
  end
end

If the bug isn't glaringly obvious at first sight then that reinforces my point ;-) Input validation is a good thing for users.

@jonleighton jonleighton and 1 other commented on an outdated diff Mar 22, 2013
test/acceptance/app_test.rb
@@ -156,6 +156,11 @@ def spring_test_command
assert_success spring, stdout: 'Usage: spring COMMAND [ARGS]'
end
+ test "test command defaults to test/" do
+ @test = ''
+ assert_success spring_test_command, stdout: "0 failures"
+ end
jonleighton
jonleighton Mar 22, 2013 Member

I don't think we need an acceptance test for this.

aspiers
aspiers Mar 22, 2013

I'll take it out then.

aspiers
aspiers Mar 22, 2013

Hmm, then it wouldn't be tested at all, which is not good. Can you suggest a good way to unit test this instead?

jonleighton
jonleighton Mar 22, 2013 Member

I guess it could be tested via mocking, but I don't really have a problem with it not being tested to be honest - this code is basically "glue", and doesn't involve lots of complex logic. What I'm trying to avoid is the app_test.rb file growing to such a size that it takes a really long time to run, as I think that will make general development much more painful. So there's a balance to be struck - I don't think this test is problematic in itself, but if we have an acceptance test for every little thing like this it will start to really drag down the test speed.

aspiers
aspiers Mar 22, 2013

Understood, although I noticed that the acceptance test suite is already extremely slow - ideally we could find some way to optimise that instead, but maybe you've already exhausted all possible avenues there?

jonleighton
jonleighton Mar 22, 2013 Member

Yeah, the tests are slow because they're booting up rails processes all the time, which is kinda the point of spring :)

aspiers
aspiers Mar 22, 2013

Right ;-) But maybe the initial boot-up could be made faster? Are all those gems needed?

aspiers commented Apr 6, 2013

If you're here at RubyManor maybe we could meet for a quick chat about this? I'll try to split the input validation commit out into a different pull request so that we can get the other (non-contentious) commits merged at least.

aspiers commented Apr 8, 2013

OK, the contentious change is no longer part of this pull request. I also rebased against latest master. Please can you review/merge this and then we can address the remaining issues separately? Cheers!

@jonleighton jonleighton and 1 other commented on an outdated diff Apr 9, 2013
test/acceptance/app_test.rb
@@ -148,7 +148,7 @@ def spring_test_command
teardown do
app_run "#{spring} stop"
- File.write(@test, @test_contents)
+ File.write(@test, @test_contents) unless @test.empty?
jonleighton
jonleighton Apr 9, 2013 Member

what's this for?

aspiers
aspiers Apr 10, 2013

It's a remnant from the test you asked to be dropped. I guess we can ditch it now, although it still makes sense for if we wanted to re-introduce a similar test in the future.

@jonleighton jonleighton and 1 other commented on an outdated diff Apr 9, 2013
test/unit/commands_test.rb
@@ -2,16 +2,18 @@
require "spring/commands"
class CommandsTest < ActiveSupport::TestCase
- test "test command needs a test name" do
+ def run_spring_testunit(args)
jonleighton
jonleighton Apr 9, 2013 Member

I'm confused about this - it's not called from anywhere as far as I can tell

aspiers
aspiers Apr 10, 2013

It's the result of a refactoring which is needed by the contentious changes which were originally part of this pull request. I removed them but didn't realise the refactoring wasn't used by anything else in this pull request, which is why it looks funny, but if you view the individual commits it makes slightly more sense.

Member

Sorry, I was at Ruby Manor but I didn't see your message

aspiers commented Apr 10, 2013

I'll tidy up this stuff soon and re-push.

Adam Spiers added some commits Apr 6, 2013
Adam Spiers make "spring test" support multiple arguments
"spring test" needs to accept multiple arguments if guard-minitest is
to ever have a chance of supporting spring.
da2413c
Adam Spiers make "spring test" run all unit tests when given no arguments
With no arguments, it now defaults to running all test-unit tests it
can find, i.e. files matching test/**/*_test.rb.  This is more useful
and arguably more intuitive too.

We need to remove test/performance/browsing_test.rb from the Rails app
being tested, otherwise "spring test" attempts to run this when it
iterates over test/**/*_test.rb, causing a failure due to ruby-prof
not being installed.

Signed-off-by: Adam Spiers <spring@adamspiers.org>
34b2ee8
aspiers commented Apr 11, 2013

OK, another attempt...

@jonleighton jonleighton merged commit 5e03447 into rails:master Apr 12, 2013

1 check passed

default The Travis build passed
Details
@aspiers aspiers deleted the aspiers:test-multi-args branch Apr 17, 2013
@pgaertig pgaertig referenced this pull request in guard/guard-test May 13, 2013
Merged

Spring support (fixes #40) #41

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