Fix rails console --sandbox test for Mac OS #9761

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Contributor

Using Process#kill was triggering ReportCrash on Mac OS Moutain Lion and
the test would not complete. This happened whether run from rake test
or as ruby -w -Itest ...

/cc @jonleighton @carlosantoniodasilva

Hm yeah, I was having problems but couldn't look yet. Will try your patch later, thanks @macksmind!

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Mar 17, 2013
railties/test/application/console_test.rb
@@ -120,15 +120,19 @@ def assert_output(expected, timeout = 1)
end
def write_prompt(command, expected_output = nil)
+ expected_output ||= "> "
carlosantoniodasilva
carlosantoniodasilva Mar 17, 2013 Owner

Can you move this default to the arguments definition instead?

Confirme, tests pass for me with this change.

@jonleighton thoughts?

@MacksMind MacksMind Fix rails console --sandbox test for Mac OS
Using Process#kill was triggering ReportCrash on Mac OS Moutain Lion and
the test would not complete. This happened whether run from `rake test`
or as ruby -w -Itest ...
c7a48b9
Contributor

@carlosantoniodasilva not passed the tests...

@rrmartins sorry, what? It seems to be passing here.

Contributor

@carlosantoniodasilva here in my machine had a failure, below is the log:

1) Failure:
FullStackConsoleTest#test_sandbox [test/application/console_test.rb:125]:
"Post.count" expected, but got:

quit

@rrmartins crap.. which version is your OS?

Contributor

@carlosantoniodasilva here is my version 10.8.3

Contributor

@rrmartins Pulling that down now.

Weird, I'm already on OSX 10.8.3, and it worked.

Contributor

Works for me on 10.8.3 too. @rrmartins, call you try increasing the timeout at line 109 and see if anything changes. Grasping at straws, but it seems like that quit should have already been consumed.

Member

Sorry for my slow reply folks, I've been moving house :( :( :( I'll look at this today.

Member

Hi guys, I hope that the above commit resolves this? Please let me know if not.

Contributor

No, it doesn't.

$ bundle exec ruby -wItest test/application/console_test.rb
Run options: --seed 41388

# Running tests:

.....Terminated: 15
Contributor

Based on the Process#kill rdoc, what you did looks good. But I played with it for a while on my Mac and I couldn't figure out how to make it do what I wanted. My patch isn't ideal, but it's the best I could think of. Do we need to bring this up at http://bugs.ruby-lang.org/ ?

Member

Hmm, weird. Maybe some native extension is objecting to the signal. Anyway, how about this?

diff --git a/railties/test/application/console_test.rb b/railties/test/application/console_test.rb
index d586822..80700a1 100644
--- a/railties/test/application/console_test.rb
+++ b/railties/test/application/console_test.rb
@@ -126,12 +126,6 @@ class FullStackConsoleTest < ActiveSupport::TestCase
     assert_output "> "
   end

-  def kill(pid)
-    Process.kill('TERM', pid)
-    Process.wait(pid)
-  rescue Errno::ESRCH
-  end
-
   def spawn_console
     pid = Process.spawn(
       "#{app_path}/bin/rails console --sandbox",
@@ -148,15 +142,13 @@ class FullStackConsoleTest < ActiveSupport::TestCase
     write_prompt "Post.count", "=> 0"
     write_prompt "Post.create"
     write_prompt "Post.count", "=> 1"
-
-    kill pid
+    @master.puts "quit"

     pid = spawn_console

     write_prompt "Post.count", "=> 0"
     write_prompt "Post.transaction { Post.create; raise }"
     write_prompt "Post.count", "=> 0"
-  ensure
-    kill pid if pid
+    @master.puts "quit"
   end
 end

I basically want to avoid having special cases for different OSes.

Contributor

Yes!

@jonleighton jonleighton added a commit that referenced this pull request Mar 22, 2013
@jonleighton jonleighton Don't kill the console
Use the "quit" command instead. This seems to prevents some weirdness on
OS X. See #9761.
c91789c
Member

Ok, I've committed that then

@jonleighton jonleighton reopened this Mar 22, 2013
Contributor

@jonleighton Thanks

@DanOlson DanOlson added a commit to DanOlson/rails that referenced this pull request Mar 24, 2013
@jonleighton @DanOlson jonleighton + DanOlson Send SIGTERM, not SIGQUIT.
SIGTERM is the correct signal for a graceful exit.

This will hopefully resolve #9761.
988a117
@DanOlson DanOlson added a commit to DanOlson/rails that referenced this pull request Mar 24, 2013
@jonleighton @DanOlson jonleighton + DanOlson Don't kill the console
Use the "quit" command instead. This seems to prevents some weirdness on
OS X. See #9761.
8b0b595
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment