sj26 / rspec_junit_formatter

RSpec results that your CI can read
http://rubygems.org/gems/rspec_junit_formatter
MIT License
302 stars 122 forks source link

Skip STDOUT/STDERR capture when reports are not generated #80

Closed ianfixes closed 4 years ago

ianfixes commented 4 years ago

Using the example config.around(:each) behavior listed here has a strange side effect with tools like pry (and possibly others) in which invoking binding.pry seems to simply be ignored. This is probably to do with pry's expectation of working with a terminal and finding none.

But in any case, this code checks whether the RspecJunitFormatter has been requested and captures output conditionally on that setting.

ianfixes commented 4 years ago

Would you accept a pull request to package this code into the library itself? It seems cleaner, especially if you maintain a lot of ruby repositories...

sj26 commented 4 years ago

Capturing stdout is nor a core responsibility of this formatter. This example code is provided as a launching point for what are likely to be more specific requirements for stdout capture. For example, another approach would be to add a tag to the each in this configure block then tag each spec unit requiring stdout capture explicitly. But this is an exercise to the user, and will be unique for each use.

It'd be neat if this was extracting into an rspec_stdout_capture gem which uses the right meta-data keys, though!

ianfixes commented 4 years ago

Your comment about not putting the example code into your library makes sense.

However, I don't see what that has to do with rejecting this update to your documentation. At the very least, your README.md should acknowledge the potential consequences of the code it is suggesting. (It took us a quite a long while to figure out why binding.pry wasn't working when debugging an rspec test.)

ianfixes commented 4 years ago

Are we in disagreement about the need to add any sort of warning to the documentation?

sj26 commented 4 years ago

Sorry, you're right that a note in the docs is a good idea, I just hadn't got to this notification yet! I'll update the README now.

sj26 commented 4 years ago

I've added a note in 3738c28.

ianfixes commented 4 years ago

Thanks!