Skip to content
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

OutputToStdout and OutputToStderr matchers #410

Merged
merged 25 commits into from Jan 16, 2014
Merged

Conversation

@lucapette
Copy link

@lucapette lucapette commented Jan 5, 2014

This pull-request is a very raw draft of how I would like to implement two new built-in matchers that have been discussed in #399, since @matthias-guenther said he wouldn't have the time to move on with it I thought it would have been great to try to contribute to my favourite Ruby open-source project.

What I did so far is implementing the matchers following the specs suggested by @myronmarston to a point all the specs I have are green on MRI 2.1.0 (not sure about other implementations but travis-ci will answer this question soon).

Since it's the first time I try to contribute to RSpec please bare with me because I have some questions:

  • If I understood it correctly we need alias methods (and tests for them) for output_to_stdout and output_to_stderr like output_from_stdout in order to support composition of the matchers.
  • We need cukes because all the matchers have them, for documentation reasons if I understood it.
  • I'm not sure you like the fact we have one OutputToStream class handling both $stdout and stderr. I did it in order to fight a clear duplication. What I don't like about this approach is the way we find out which stream is requested (@stream == $stdout for example).
  • I can't say I like the naming of all the things. But this is a problem I always have, I'm never satisfied with names. Never. It's always a trade-off for me and I'd like to know if you like the one we currently have.
  • In the current implementation we have the shared examples for both matchers in the existing file we have for a more general shared example. I can live with this but I can't say if it's OK with you.
  • A problem connected with the previous is that now we have two different spec files for the matchers but we have one file (with a different name) with the actual implementation.
  • I think we're covering all the basic cases but since it's the first time I'm trying to test a testing framework (that's so nicely meta btw) that I can't say the coverage we have it's enough. I added some tests to the one suggests in #399 and changed the wording a bit.

Sorry for asking so many questions :)

end
end

private

This comment has been minimized.

@JonRowe

JonRowe Jan 5, 2014
Member

We like to indent private / public inline with class/end so this should be dedented two spaces.

This comment has been minimized.

@lucapette

lucapette Jan 6, 2014
Author

Will do. Would you mind telling me the reasoning you have for it? Fortunately vim-ruby has a nice feature to help not getting wrong it again.

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Semantically, private affects method definitions below it, so it's nice to have the method defs indented below them. However, I find the rails convention to be ugly:

class Foo
  def public_method
  end

  private

    def private_method
    end
end

We like putting private at the same indentation level as the class to be the best of both worlds: it keeps all method defs at the same level of indentation, while visually showing that the methods below private are affected by it:

class Foo
  def public_method
  end

private

  def private_method
  end
end

This comment has been minimized.

@lucapette

lucapette Jan 6, 2014
Author

I see, pretty interesting position about this. Thank you for the explanation.

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Our of curiosity, what is your preferred convention?

This comment has been minimized.

@lucapette

lucapette Jan 6, 2014
Author

I don't really have a favourite one. But I do dislike Rails convention. The extra-level of indentation drives me crazy :)

end

def formatted_stream
@stream == $stdout ? "stdout" : "stderr"

This comment has been minimized.

@JonRowe

JonRowe Jan 5, 2014
Member

This doesn't cater for when @stream is neither $stdout or $stderr. How about using a case statement with the default just being inspect?

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

I'd rather this just be a stream_name attribute that is initialized by a value passed from output_to_stdout and output_to_stderr:

def output_to_stdout(expected=nil)
  BuiltIn::OutputToStream.new($stdout, :stdout, expected)
end

def output_to_stderr(expected=nil)
  BuiltIn::OutputToStream.new($stderr, :stderr, expected)
end

Then there's no need for this implicit inference logic.

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Nevermind on that idea: I came up with a better one (see my comment below capture_stream...): make a base class and two subclasses -- one for stdout, one for stderr.

This comment has been minimized.

@lucapette

lucapette Jan 6, 2014
Author

I thought of having two sub-classes at first but then I went for a simpler (raw) version because I wasn't convinced enough you would like it. Now I am.

else
"expected block to not output to #{formatted_stream}, but did"
end
end

This comment has been minimized.

@JonRowe

JonRowe Jan 5, 2014
Member

It seems like failuare_message, failure_message_when_negated could be DRYed up a bit, mostly thinking the conditional, how about wrapping the conditional into a method:

"... #{formatted_expected} ... to #{formatted_stream}"

def formatted_expected
  "#{@expected.inspect} " unless @expected
end

This comment has been minimized.

end
end
end
end

This comment has been minimized.

@JonRowe

JonRowe Jan 5, 2014
Member

For clarity it might be better to combine the two spec files into output_to_stream_spec.rb and include the shared examples there...

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Yep, that would improve things, I think, given there's one common implementation file. We've done that for others (e.g. yield_spec.rb).

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jan 5, 2014

Hey thanks! This is a good start, I left a few minor comments :)

@@ -0,0 +1,64 @@
module RSpec

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

This file needs a require 'stringio' since it relies upon that piece of stdlib.

This comment has been minimized.

end

def formatted_actual
captured? ? @actual.inspect : "nothing"

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

This is formatted funny: two spaces of indention, please.

This comment has been minimized.

@lucapette

lucapette Jan 6, 2014
Author

it's funny indeed. I'll be more careful.


context "expect { ... }.to #{matcher_method}('string')" do
it "passes if the block outputs that string to #{stream_name}" do
expect { stream.puts 'foo' }.to matcher("foo\n")

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

The extra \n is a little jarring at first but I see why it's needed. What do you think about changing stream.puts to stream.print? Then it's not needed.

expect { stream.puts 'foo' }.to matcher("foo\n")
end

it "fails if the block does not outputs to #{stream_name}" do

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

s/outputs/output/

ensure
$stdout = orig_stdout
$stderr = orig_stderr
end

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Your implementation here always captures both stdout and stderr. Besides the extra work this does, it feels like a bug to me that in a spec like this:

expect {
  puts "calling the method"
  the_method
}.to output_to_stderr("foo")

...my debugging output of "calling the method" is silenced. I've only told RSpec only to look at stderr, so why should it mess with stdout?

So, if you make a base class and two subclasses (one for stdout, one for stderr), you can implement the capture_stream method to capture only the appropriate stream.

This comment has been minimized.

@JonRowe

JonRowe Jan 6, 2014
Member

I had this concern initially too, I'd like to see this work for any stream, and then be setup for the individual $stdout / $stderr.

}.to fail_with("expected block to not output \"foo\\n\" to #{stream_name}, but did")
end
end
end

This comment has been minimized.

@myronmarston

myronmarston Jan 6, 2014
Member

Some additional specs I'd like to see passing (I'll leave the implementation as an exercise to you!):

  context "expect { ... }.to #{matcher_method}(/regex/)" do
    it "passes if the block outputs a string to #{stream_name} that matches the regex" do
      expect { stream.puts 'food' }.to matcher(/foo/)
    end
    it "fails if the block does not output to #{stream_name}"
    it "fails if the block outputs a string to #{stream_name} that does not match"
  end

  context "expect { ... }.to #{matcher_method}(/regex/)" do
    # similar specs needed
  end

  context "expect { ... }.to #{matcher_method}(matcher)" do
    it "passes if the block outputs a string to #{stream_name} that passes the given matcher" do
      expect { stream.puts 'foo' }.to matcher(a_string_starting_with("f"))
    end
    # more specs needed (including failing examples)
  end

  context "expect { ... }.not_to #{matcher_method}(matcher)" do
    # similar specs needed
  end

To get these to pass you'll need to leverage the Composable mixin (already included in BaseMatcher):

  • Use values_match? to do the string matching (it'll match exact strings, against a regex or against any matcher).
  • Use description_of in failure messages to get good failure output when matchers are passed.
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jan 6, 2014

If I understood it correctly we need alias methods (and tests for them) for output_to_stdout and output_to_stderr like output_from_stdout in order to support composition of the matchers.

To be consistent with the other matcher aliases, I think the alias we want is a_block_outputting_to_stdout and a_block_outputting_to_stderr. That will allow it to be used in matcher expressions like:

expect(manager.callback_blocks).to include( a_block_outputting_to_stdout("foo") )

The matcher also needs to be able to accept a matchers as an argument (rather than just a string) -- I left some more detail about that in an inline comment.

This also needs the it_behaves_like "an RSpec matcher" shared example group applied to these two matchers -- we use that on all built in matchers to ensure some consistent things about all matchers.

BTW, I think that there's some potential gotchas around usage of these matchers. There are ways to output to stdout and stderr that this matcher won't be able to intercept:

  • Using STDOUT.puts or STDERR.puts.
  • Storing a reference to $stdout and $stderr before the matcher is used:
class MyClass
  def initialize(log_to=$stdout)
    @log_to = log_to
  end

  def do_it
    @log_to.print "I did it"
  end
end

describe MyClass do
  it "logs to stdout" do
    instance = MyClass.new
    expect { instance.do_it }.to output_to_stdout("I did it")
  end
end

I'm not sure that we can or should do anything about these gotchas...but it would be good to document them with specs (e.g. demonstrating known, documented cases where this matcher cannot work properly) and by adding a @note to the YARD docs for the matcher methods and the cuke file. (Speaking of which, yes, please do add that cuke!).

Thanks for your hard work on this @lucapette -- this is going to be a nice addition to rspec-expectations :).

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 6, 2014

Thank you the fantastic feedback! I'll happily do the changes you asked for. Everything makes sense to me. I'll come back as soon as possible with code/questions!

@wikimatze
Copy link

@wikimatze wikimatze commented Jan 6, 2014

Thanks to @lucapette, @myronmarston, and @JonRowe, it's such a pleasure to read about your interesting discussions and thoughts about how to solve this problem.

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 12, 2014

@myronmarston @JonRowe I finally found some hours to add more code/docs based on your feedback. The only thing I'm not sure is my English in documentation and in the cukes, being a non-native speaker doesn't help. I tried to clean up the history of the branch a bit but I'm not sure you want all those commits or you prefer me to squash them in one commit, please let me know what you prefer.

So far I like the output but I honestly can't say if the branch can be considered ready. I'll gladly follow your feedback!

lucapette added 6 commits Jan 14, 2014
- Add backticks where needed
- Use correct reference for the stream
- Mention matchers together with regexps
- Reword the description of the feature
@lucapette
Copy link
Author

@lucapette lucapette commented Jan 14, 2014

OK, I found some time during the week, that's rare. I think I applied all the changes you requested. I love this PR/code review cycle.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jan 14, 2014

Thanks, @lucapette!

I would probably do it in a separate PR if you're OK with it.

Normally that would be fine, but we're trying to get 3.0.0.beta2 out real soon, and I don't want to merge this and release with an API we plan to change before 3.0 final. So I'll leave this PR as-is for now, and if we release before you get around to changing it, I can merge this as-is; otherwise, if you start working on changing it before we release you can keep adding it to this PR. Does that sound fine?

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 14, 2014

It sounds very reasonable to me. So to move forward I have a couple of questions.

Just to be sure we move in the right direction, what we want is something like:

        specify { expect { print('foo') }.to output_to_stdout }
        specify { expect { print('foo') }.to output('foo').to_stdout }
        specify { expect { print('foo') }.to output(/foo/).to_stdout }
        specify { expect { }.to_not output_to_stdout }
        specify { expect { print('foo') }.to_not output('bar').to_stdout }
        specify { expect { print('foo') }.to_not output(/bar/).to_stdout }

Do you have a rough idea of when the release is coming? Just curious, maybe I can try to find a bit more time to work on this.

@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jan 14, 2014

It's a little weird to have output_to_stdout and output(...).to_stdout. For the case where you aren't specifying what is output (and just that something is), what do you think about this? expect { ... }.to output.to_stdout. That way it keeps the output(...).to_stdout form, but you can choose not to pass an arg to output, which indicates you aren't bothering to specify what is output (which makes sense, since that's what the arg represents).

Do you have a rough idea of when the release is coming? Just curious, maybe I can try to find a bit more time to work on this.

We were hoping to get beta2 out by the end of 2013. There are some pending items left to do, though. @JonRowe has a bunch of those in-flight in rspec-core, so it largely depends on his schedule.

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 14, 2014

it's weird and actually that's why I asked :) I wasn't sure output.to_stdout would be exactly what we want. I'll try to come back as soon as possible but I can't promise it will be really fast :(. Thank you for everything!

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 14, 2014

OK, I guess I was in the zone or something and I couldn't stop so I updated the PR to the cleaner API we discussed. Not sure this is exactly what you wanted from the implementation/documentation point of view but at least the API looks pretty good now.

Feature: output to stream matchers

The `output` matcher provides a way to assert that the block-under-test
has emitted content to either `to_stdout` or `to_stderr`.

This comment has been minimized.

@JonRowe

JonRowe Jan 14, 2014
Member

This has been indented three spaces rather than two...

This comment has been minimized.

@JonRowe

JonRowe Jan 14, 2014
Member

You could also just say 'block under test" without hyphens, but it might be even cleared to say "code in the block"

module BuiltIn
class OutputToStream < BaseMatcher
def initialize(expected)
@expected = expected

This comment has been minimized.

@JonRowe

JonRowe Jan 14, 2014
Member

You should set @stream here to a null object, to prevent 'nasty' errors when a developer makes a mistake with the matcher (e.g. just uses output)

This comment has been minimized.

@myronmarston

myronmarston Jan 15, 2014
Member

Actually, IMO, given that we don't intend output to be used on its own w/o to_xyz being chained off of it, we should make matches? raise an error if they didn't call one of the to_ methods.

This is similar to be_within:

raise needs_expected unless defined? @expected

expect(x).to be_within(0.1) will raise an error because you didn't chain of off of it.

This comment has been minimized.

@JonRowe

JonRowe Jan 15, 2014
Member

We should at least set @stream = nil to avoid any potential warnings, also will raising in matches? then trigger an error for @stream.name in the description?

This comment has been minimized.

@myronmarston

myronmarston Jan 15, 2014
Member

Good point, it's probably worth using a null object for that.

@JonRowe
Copy link
Member

@JonRowe JonRowe commented Jan 14, 2014

Looks good, I just have a concern about @stream not being setup upon initialise, which opens the door for unexpected errors / obtuse error messages. I'd add a null object that implements message and raises an appropriate error in capture (ExpectationNotMet with a message explaining what they did wrong maybe?). I'd also be good to have a spec covering that eventuality.

* `output.to_stdout` matches if the block-under-test outputs to
$stdout.

* `output.to_stderr` matchets if the block-under-test outputs to

This comment has been minimized.

@myronmarston

myronmarston Jan 15, 2014
Member

s/matchets/matches/

end
alias_matcher :a_block_outputting, :output do |desc|
desc.sub('output', 'a block outputting')
end

This comment has been minimized.

@myronmarston

myronmarston Jan 15, 2014
Member

Is the block still necessary here? It seems like the description more closely matches the method name and maybe the block isn't needed anymore.

@@ -26,6 +26,7 @@ module BuiltIn
autoload :Match, 'rspec/matchers/built_in/match'
autoload :NegativeOperatorMatcher, 'rspec/matchers/built_in/operators'
autoload :OperatorMatcher, 'rspec/matchers/built_in/operators'
autoload :OutputToStream, 'rspec/matchers/built_in/output_to_stream'

This comment has been minimized.

@myronmarston

myronmarston Jan 15, 2014
Member

Given the RSpec::Matchers method is now output, I think I'd like the class name to be BuiltIn::Output and the file name to build_in/output to match. For all the other matchers I think they match exactly and it's nice to be consistent. The cuke and spec file should be updated to match as well.

def to_stderr
@stream = CaptureStderr.new
self
end

This comment has been minimized.

@myronmarston

myronmarston Jan 15, 2014
Member

I'm really happy with how this decomposed nicely into a composition-based solution :). This is really clean and well done!

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 15, 2014

Here I'm again :) I think I applied all the feedback you gave me. I'm just not sure about the name of the null object, it's currently NullCapture (I'd go for something like CaptureNothing I think but I see we have a NullSolution somewhere so I kept the prefix).
On the same topic, aka naming is hard, there is the message error for when the user doesn't set any stream expectation. Not sure that's good enough.
Anyway I'm pretty sure that if there is room for improvement you'll point me into the right direction! Thank you for taking care of RSpec.

myronmarston added a commit that referenced this pull request Jan 16, 2014
OutputToStdout and OutputToStderr matchers
@myronmarston myronmarston merged commit ffd25a8 into rspec:master Jan 16, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
myronmarston added a commit that referenced this pull request Jan 16, 2014
- Add changelog entry.
- Copy the explanation from the YARD comments to
  the cuke as I think it was worded a bit better.
- Rephrase the note a bit to explain why.
- Since the the stream capturers are stateless,
  use singleton modules for them. Less garbage for the GC!
- Rename ivar to `@stream_capturer` since it's not a stream
  itself.
- Improve error message when the user forgets to chain
  `to_stdout` or `to_stderr` off of it.
- Still provide a description in this case.
@myronmarston
Copy link
Member

@myronmarston myronmarston commented Jan 16, 2014

Thanks, @lucapette! I merged and I also applied a few final tweaks in 4a8bb9f if you want to take a look. This is going to be a nice new feature in RSpec 3 :).

@lucapette
Copy link
Author

@lucapette lucapette commented Jan 16, 2014

thank you @myronmarston and @JonRowe for helping me writing this feature, and thanks to @matthias-guenther for the original idea. It was really a pleasure to contribute to RSpec! I hope I'll find a way to help more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.