Added another realization for capturing output #161

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@route

Original topic https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-core/iJ6VvC5igPk

Guys I decided to open PR because here we can discuss my code and these tests together.

require 'minitest/spec'
require 'minitest/autorun'

describe "Capture" do
  it "should capture all output in the same process" do
    out, err = capture_io do
      puts "STDOUT"
      warn "STDERR"
    end

    assert_equal "STDOUT\n", out
    assert_equal "STDERR\n", err
  end

  # This test isn't capture output and get failure
  it "should capture all output in subprocess" do
    out, err = capture_io do
      system("echo 'STDOUT'")
      system("echo 'STDERR' 1>&2")
    end

    assert_equal "STDOUT\n", out
    assert_equal "STDERR\n", err
  end

  # This works fine
  it "should capture all output in subprocess with new method" do
    out, err = full_capture_io do
      system("echo 'STDOUT'")
      system("echo 'STDERR' 1>&2")
    end

    assert_equal "STDOUT\n", out
    assert_equal "STDERR\n", err
  end
end
@route

You can use StringIO instead of Tempfile to avoid disk IO.
@sobrinho I have the same thoughts as and you, I tried to do it, but reopen require IO with fd.
And also that's the cause why I hesitate to replace older version.

@sobrinho

@route I think you don't need to call reopen, you can just do something like that:

begin
  stdout, stderr = $stdout, $stderr
  $stdout, $stderr = StringIO.new, StringIO.new

  ...

  $stdout.rewind
  $stderr.rewind

  [$stdout.read, $stderr.read]
ensure
  $stdout, $stderr = stdout, stderr
end

I think this will work, didn't tested.

@route

It's exactly old realization, and it won't work if you're going to use subprocesses, because old descriptors are inherited.

@sobrinho

I see.

I think the capture_io should work with subprocesses instead of using full_capture_io for one case and capture_io for other case.

WDYT, @seattlerb?

@zenspider
Seattle Ruby Brigade member

1) needs a test.

2) I think you should be reopening on $std, not STD... but I need a test to know for sure.

@zenspider zenspider was assigned Sep 13, 2012
@route

@zenspider I've done what you said and squashed two methods into one as @sobrinho proposed

@zenspider
Seattle Ruby Brigade member

I don't think Tempfile is the way to go. I get nothing but errors now with this patch:

...
Class: <Errno::EBADF>
Message: <"Bad file descriptor - /var/folders/7z/kz0n494d16j6zxqd46stxtrc0000gn/T/20120917-9987-b2ygkm-0">
...
228 tests, 706 assertions, 18 failures, 13 errors, 0 skips

Also, the comment on the impl is stale now that you squashed.

I'll poke at it and see what I can do.

@zenspider
Seattle Ruby Brigade member

And please set up your editor to point out trailing whitespace...

@zenspider
Seattle Ruby Brigade member

OK. The problem is the return line. You should be using the tempfile vars instead. The ensure block gets in the way and messes up the results. I've cleaned up the code and the comment and am committing after a second polish.

@zenspider
Seattle Ruby Brigade member

Hrm... turns out that this code is almost 11x slower than the previous... so I'm splitting it out to capture_subprocess_io.

@route

Yep two methods it was what I thought at first, because I don't want to use Tempfile either

@route

I understood the problem with ensure, sorry about that, I didn't have failures:

228 tests, 737 assertions, 0 failures, 0 errors, 0 skips
@zenspider
Seattle Ruby Brigade member

http://pastie.org/private/yuweun8zuadpzbe76tbota is my current version. I'd use gist, but github's oauth mandate bullshit has broken every editor/cmdline interface I have for it. So fuck em.

@zenspider
Seattle Ruby Brigade member

@tenderlove pls glom your orbs on this

@route

I think that on the line 39 in your pastie code should be system("") too

@zenspider
Seattle Ruby Brigade member

good catch. I've also removed the superfluous quotes.

@zenspider
Seattle Ruby Brigade member

no eyeballs from @tenderlove ... :(

committed!

@zenspider zenspider closed this Sep 18, 2012
@tenderlove
Seattle Ruby Brigade member
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment