Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Window command line generation patches #762

Merged
merged 3 commits into from

2 participants

@jojje

Two commits.
1. The fix for quoting and its associated test updates.
2. A regression test to avoid single-quoted arguments from creeping in again, in the future (as requested).

spec/autotest/rspec_spec.rb
@@ -28,9 +28,15 @@
@to_test = files.map { |f| File.expand_path(f) }.join ' '
end
+ it "uses double quotes for windows compatibility" do
+ actual_command = rspec_autotest.make_test_cmd(@files_to_test)
+ expected_pattern = /^[^\']+$/
+ actual_command.should match(expected_pattern)
+ end
@myronmarston Owner

I think this could be expressed in a simpler fashion:

command = rspec_autotest.make_test_cmd(@files_to_test)
command.should include('"')
command.should_not include("'")

The regex seems more convoluted and doesn't actually verify that double quotes are included....which seems kinda important.

@jojje
jojje added a note

I agree, proper use of matches is key to expressiveness and when you see people not exploiting the full vocabulary of your test framework it only indicates one thing; lack of insight :)

My reasoning for not checking for double quotes was that I was thinking a bit non-TDD, namely "what if he decided in the future to no longer need quoting, then the test would break at that point", instead of the proper notion: "how is the code supposed to work NOW". I'll remedy it as suggested.

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

BTW, please don't open a separate pull request for additional changes...it makes more work for us to have to track multiple pull requests than just the one. You can always force push if you want to rebase and squash things down into fewer commits.

@jojje

Ok, thanks for that useful advice, appreciate it. Still in a learning mode for proper collaboration on this site.

@myronmarston

Ok, thanks for that useful advice, appreciate it. Still in a learning mode for proper collaboration on this site.

No worries...I'm glad to see you took a stab at diving right in and contributing :).

@jojje

On a side note: Did a test on the force update approach as you mentioned by amending the commit message, to see how github and travis react, and indeed, Github allows rewriting the history (not that I doubted you), but Travis seems to regard commit amendment (changing commit message only) as a change that requires a complete rebuild and re-test.

Perhaps something I should raise to github as an enhancement request, esp. since projects such as yours take 18 minutes to re-test (.. jruby :< ..).

@myronmarston myronmarston merged commit 34577db into rspec:master
@myronmarston myronmarston referenced this pull request from a commit
@myronmarston myronmarston Add changelog entry for #762.
[ci skip]
cc2e56f
@myronmarston

Thanks @jojje!

Travis seems to regard commit amendment (changing commit message only) as a change that requires a complete rebuild and re-test.

Well, a new commit is a new commit. I suppose travis could look at the SHA of the git tree itself (not taking into account the commit sha) but I've never thought of this as a problem.

Perhaps something I should raise to github as an enhancement request, esp. since projects such as yours take 18 minutes to re-test (.. jruby :< ..).

You can certainly bring it up, but I'd recommend bringing it up with the travis folks. It would be a change on their side if it got implemented.

@jojje jojje deleted the unknown repository branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 10 additions and 4 deletions.
  1. +1 −1  lib/autotest/rspec2.rb
  2. +9 −3 spec/autotest/rspec_spec.rb
View
2  lib/autotest/rspec2.rb
@@ -47,7 +47,7 @@ def consolidate_failures(failed)
# Overrides Autotest's implementation to generate the rspec command to run
def make_test_cmd(files_to_test)
files_to_test.empty? ? '' :
- "#{prefix}'#{ruby}'#{suffix} -S '#{RSPEC_EXECUTABLE}' --tty #{normalize(files_to_test).keys.flatten.map { |f| "'#{f}'"}.join(' ')}"
+ %|#{prefix}"#{ruby}"#{suffix} -S "#{RSPEC_EXECUTABLE}" --tty #{normalize(files_to_test).keys.flatten.map { |f| %|"#{f}"|}.join(' ')}|
end
# Generates a map of filenames to Arrays for Autotest
View
12 spec/autotest/rspec_spec.rb
@@ -28,9 +28,15 @@
@to_test = files.map { |f| File.expand_path(f) }.join ' '
end
+ it "uses double quotes for windows compatibility" do
+ command = rspec_autotest.make_test_cmd(@files_to_test)
+ command.should include('"')
+ command.should_not include("'")
+ end
+
it "makes the appropriate test command" do
actual_command = rspec_autotest.make_test_cmd(@files_to_test)
- expected_command = /#{ruby_cmd}.*'#{spec_cmd}' (.*)/
+ expected_command = /#{ruby_cmd}.*"#{spec_cmd}" (.*)/
actual_command.should match(expected_command)
@@ -46,13 +52,13 @@
it "quotes the paths of files to test" do
cmd = rspec_autotest.make_test_cmd(@files_to_test)
@files_to_test.keys.each do |file_to_test|
- cmd.should match(/'#{File.expand_path(file_to_test)}'/)
+ cmd.should match(/"#{File.expand_path(file_to_test)}"/)
end
end
it "quotes the path of the ruby executable" do
cmd = rspec_autotest.make_test_cmd(@files_to_test)
- cmd.should match(%r('/path/to/ruby'))
+ cmd.should match(%r("/path/to/ruby"))
end
it "gives '--tty' to #{Autotest::Rspec2::RSPEC_EXECUTABLE}, not '--autotest'" do
Something went wrong with that request. Please try again.