New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a configuration option to cause tests to fail if they write stderr or stdout #2460

Open
samphippen opened this Issue Aug 28, 2017 · 8 comments

Comments

Projects
None yet
8 participants
@samphippen
Member

samphippen commented Aug 28, 2017

One of the things that many RSpec users really love is clean output. At the moment, we don't have a great story for enforcing this at a suite level. I propose that we add a configuration option that makes this possible, and a special method in examples to opt out for debugging.

Details

A new configuration option would be introduced fail_when_examples_writing_to_stderr_or_stdout which is a boolean. Configuration would look like:

RSpec.configure do |rspec|
  rspec.fail_when_examples_writing_to_stderr_or_stdout = true
end

Adding this option would cause any example to fail if during its execution it writes to stderr or stdout, with an error message something akin to:

This example wrote to [stderr/stdout] during its execution, which is explicitly disabled in your RSpec suite's configuration. If you are using this to debug, please invoke `allow_writing_to_stderr_and_stdout`

the allow_writing_to_stderr_and_stdout method would then enable the user to write, temporarily for debugging, to stdout.

Open questions

One might imagine that it is desirable to prevent any use of allow_writing_to_stderr_and_stdout in CI, and to do this we could consider adding a configuration option: disable_allow_writing_to_stderr_and_stdout which would make the method a noop. What do we think about this?

@myronmarston @JonRowe WDYT about this feature?

@nateberkopec

This comment has been minimized.

Show comment
Hide comment
@nateberkopec

nateberkopec Aug 28, 2017

Shit in your green dots is one of those things that happens slowly, over time. It's easy to fix if it's just one spec that does it, but when it's a whole suite...oh boy. So this option is most useful for new, small suites. It would be great to get this in a default or the "suggested configuration options" for this reason.

nateberkopec commented Aug 28, 2017

Shit in your green dots is one of those things that happens slowly, over time. It's easy to fix if it's just one spec that does it, but when it's a whole suite...oh boy. So this option is most useful for new, small suites. It would be great to get this in a default or the "suggested configuration options" for this reason.

@Ymbirtt

This comment has been minimized.

Show comment
Hide comment
@Ymbirtt

Ymbirtt Aug 28, 2017

Providing there was a stack trace to show me exactly what was writing to STDOUT, this'd be really useful. I'd likely have this on in most of my tests, but have allow_writing_to_stderr_and_stdout option fixed to true in my config file, so that it fails in my CI rather than on my dev machine, so both options would be useful.

Ymbirtt commented Aug 28, 2017

Providing there was a stack trace to show me exactly what was writing to STDOUT, this'd be really useful. I'd likely have this on in most of my tests, but have allow_writing_to_stderr_and_stdout option fixed to true in my config file, so that it fails in my CI rather than on my dev machine, so both options would be useful.

@chrisarcand

This comment has been minimized.

Show comment
Hide comment
@chrisarcand

chrisarcand Aug 28, 2017

Love the idea.

Given the plethora of ways to get output on the screen, I wonder if all pathways could be realistically monitored. Specifically, I've experienced some pain with C extensions outputting to the screen and haven't really come up with a good way to find or suppress them, so I'm curious how one does that.

chrisarcand commented Aug 28, 2017

Love the idea.

Given the plethora of ways to get output on the screen, I wonder if all pathways could be realistically monitored. Specifically, I've experienced some pain with C extensions outputting to the screen and haven't really come up with a good way to find or suppress them, so I'm curious how one does that.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 28, 2017

Member

I like the idea, but I'd suggest using metadata instead of a global flag to control it. e.g.:

RSpec.describe SomeClass, fail_on_output: true do
  # ...
end

A global flag is globally on or off, whereas metadata is per example, and has a robust system for setting it globally (via config.define_derived_metadata) and inheriting it. Making it more granular via metadata has the advantage of allowing people to migrate to it piecemeal if they have an existing suite that has this problem, and also makes it easy to temporarily allow output on an example group or individual example for the purposes of debugging.

Member

myronmarston commented Aug 28, 2017

I like the idea, but I'd suggest using metadata instead of a global flag to control it. e.g.:

RSpec.describe SomeClass, fail_on_output: true do
  # ...
end

A global flag is globally on or off, whereas metadata is per example, and has a robust system for setting it globally (via config.define_derived_metadata) and inheriting it. Making it more granular via metadata has the advantage of allowing people to migrate to it piecemeal if they have an existing suite that has this problem, and also makes it easy to temporarily allow output on an example group or individual example for the purposes of debugging.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Aug 29, 2017

I'm a big fan of clean output... I am a bit worried this would encourage/lead to people turning off deprecation warnings in Rails apps, though. We already have a fail-immediately mode; if it's not turned on, it seems undesirable for something else to give the same effect.

@samphippen do you envision rspec-rails working around this, say by registering a custom deprecation reporter that [writes back to the real stdout || separately collects deprecations and summarizes them in a style more like the 'pending specs' block]? Or is this exactly the sort of pollution you want to stop dead upon?

matthewd commented Aug 29, 2017

I'm a big fan of clean output... I am a bit worried this would encourage/lead to people turning off deprecation warnings in Rails apps, though. We already have a fail-immediately mode; if it's not turned on, it seems undesirable for something else to give the same effect.

@samphippen do you envision rspec-rails working around this, say by registering a custom deprecation reporter that [writes back to the real stdout || separately collects deprecations and summarizes them in a style more like the 'pending specs' block]? Or is this exactly the sort of pollution you want to stop dead upon?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Aug 29, 2017

Member

I'd favour the metadata option too, and make it configurable to stdout, stderr or both, it could then be overridden on an per example basis as needed for debugging.

Member

JonRowe commented Aug 29, 2017

I'd favour the metadata option too, and make it configurable to stdout, stderr or both, it could then be overridden on an per example basis as needed for debugging.

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Sep 18, 2018

Contributor

How would one go about this? Replace $stdout and $stderr with proxies when a metadata is applied to a spec?

Contributor

kaiwren commented Sep 18, 2018

How would one go about this? Replace $stdout and $stderr with proxies when a metadata is applied to a spec?

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Sep 18, 2018

Member

We already have a matcher that can introspect output, input, an easy way to implement this at the mount is to use an around hook and that matcher...

Member

JonRowe commented Sep 18, 2018

We already have a matcher that can introspect output, input, an easy way to implement this at the mount is to use an around hook and that matcher...

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