output matcher: add as_tty method to make the simulated stream behave as a TTY

[porras]

The problem

I have an app which prints some output only if the standard output is a TTY. I'd say it's a relative standard practice:

puts "Whatever" if $stdout.tty?

Currently, this can't be tested with expect { ... }.to output(...).to_stdout because $stdout is replaced with a StringIO, which always returns false for .tty?, so the code never prints.

If this is a problem RSpec wants to fix (I guess you can argue that it's an antipattern and should be implemented in a different way), this PR does it.

The fix

$stdout is still replaced with a StringIO and the matcher works the same, but some specific methods are called on the original stream. I only added .tty? because that's my usecase, but I guess there are other methods for which this could make sense (now or in the future).

I haven't contributed to RSpec before so I'm not familiar with coding standards, style, etc., so I appreciate any feedback, particularly on the following problem: I had to add an exception for the libraries allowed to be loaded for delegate (in fact there is another one for stringio for the same reason). Is this acceptable? If not, there are at least two alternatives I could implement:

• Do some trick to require delegate and define the class on runtime only once output has been called. This is already done for the tempfile require, although it would be a bit trickier than that because the class needs to be defined to.
• Not use SimpleDelegator and implement the behavior using method_missing.

Is any of these approaches preferred?

Update: the description above no longer mathes the implementation, which I've changed according to the feedback received. It's simpler now and doesn't need such a long explanation 😅

[JonRowe]

I think its reasonable to respect the original tty behaviour as we're just capturing the output, another option would be to explicitly define this with something like expect { ... }.to output(...).to_stdout.as_tty which sets it explicitly in the test

[porras]

Thanks for such a fast feedback 😄

@pirj: Will there be cases when it will be false? I guess, something like running or CI with a redirect to file? But won’t this undermine the whole fix?

Of course 🤦 (see failed build). I had only ran the tests locally and hadn't even thought of that.

--

Putting it all together, I will:

• Inherit from StringIO instead of delegating. Much simpler implementation.
• Hardcode tty? to true
  • I'll give a try to make it only when explicitly asked to, as @JonRowe suggested. I currently don't know how to implement such an .as_tty method but I'm sure I'll find good examples in the codebase
• While at it, I'll remove the let mentioned by @pirj (see comment in the relevant thread)

Once again, thank you!

[porras]

What I just pushed is without implementing the .as_tty method, it just hardcodes it to true. I'll give it a try to it.

[porras]

I just pushed an attempt at implementing as_tty. It works but it's admittedly ugly. Feedback on suggested refactoring or a different way to do it welcome 😄 I guess the ugliest thing is the duplication between CaptureStdout and CaptureStderr (which was already there, but it's more evident now), but it's difficult to refactor them together without resorting to some metaprogramming tricks, because they need to reassign the global variables.

[pirj]

Nice! Thank you.

Can you add a couple of examples covering those parts where it raises when as_tty is not compatible with “in any process”?

‘output’ calls can be chained, too? output('foo').to_stdout.and output('bar').to_stdout.as_tty. Will it work as expected? Should as_tty be specified for both?
I am sorry, Iam from my phone, does it even chain properly, or just my wishful thinking?

[JonRowe]

Nice, a few suggestions

[JonRowe]

Great! Thanks!