Report to stream

[kbrock]

reopening #89 - you can set this up to output to stdout or a string.

Sorry to jump the gun and close this. The introduction of these extra methods in the interface just never felt quite right to me. If you have some feedback, I can hopefully update to something that feels better to both of us.

[nateberkopec]

I think this is super solid as-is.

Evan made a comment in the original PR re: compare!

As for how to deal with compare!, I’d prefer to delegate it to the report itself.

@kbrock do you have the bandwidth to do that or would you prefer someone else take it from here?

[kbrock]

Think I like the way #113 uses a no-op object to avoid the nil checks. That one makes fewer changes to the interface of those objects. Which is good.

It doesn't address the ambiguous output that ends up on stdout. Thinking after that one is merged, this approach may change.

[nateberkopec]

@kbrock Definitely, I too noticed the overlap between the two PRs

[nateberkopec]

@kbrock So, where does this leave us after the suite-accessor PR?

[kbrock]

I rebased it and have a question around approaches

I like adding the stdout. that is basically done and I can add a spec to test that

I have never been a fan of the double output, the stdout and report output together. But having an output aggregator feels contrived and like I'm trying too hard to use a design pattern. I also don't like adding to the report interface just so I can make the 2 similar.

Any thoughts?

[nateberkopec]

@kbrock Take a look at how Minitest manages basically the same problem, maybe it will spark some inspiration:

https://github.com/seattlerb/minitest/blob/3c6576a51f4e266996e3459d7a0dd054eb4c87f7/lib/minitest.rb#L125 https://github.com/seattlerb/minitest/blob/3c6576a51f4e266996e3459d7a0dd054eb4c87f7/lib/minitest.rb#L822

Otherwise, hard to say without seeing more of the implementation you have in mind.

[kbrock]

Sorry for the long long lag on this one.

Ok, so the pattern that I said may be contrived is basically the one that you pointed in my direction. Coverage reports, minitest, and others seem to use the same mechanism for output to multiple sources. So I'm good to go on this.

---

I put quite a few comments in that last commit. It seems the last refactoring (that introduced no op output) and this one do change the interface a tad bit. Most of those changed seemed so minor, but I want to be clear that they happened.

I've never seen the point of using a suite to be honest. But since it seemed so close to the standard output case, and supporting the redirection of output, it seemed fine.

the handling of quiet is probably overkill.
Allowing the user to change their mind. Especially since it not per
test but for the whole quite run.

querying quite was fixed, but suite is now broken. For no suite it
used to return a noop (originally it returned nil) but now it returns
basically all output, including standard out. This is mostly because there
is no longer the distinction between a report and a suite. It will just
output of any kind.

Setting a benchmark to quiet and then adding a suite that is really a
standard output report directed to a specific output seems like a good use case.
This currently works, but quiet? will return false (when technically it is true)

This will work with existing benchmark suites, that do not have start_{warming,running} or footnote defined.
But will also call those methods if they are defined. This is a deviation from
the previous implementation.

Also, the previous case would replace the suite when assigned and this one will add more.
I would be difficult to add this detection (especially if someone chose to use a standard report
for the suite) and the use case doesn't seem as practical

previously suite returns a noop suite (when originally it would be nil)

---

I'm happy enough with this to say merge it. The only thing that could be better is a better way of redirecting the standard output.

The mechanism to manipulate:

new_output = StringIO.new
Benchmark.ips do |x|
  # A
  x.quiet = true
  x.suite = StdoutReport(new_output)

  # B
  x.suite.quiet!
  x.suite = StdoutReport(new_output)
end

[kbrock]

ok, rebased. Gosh, this has been languishing for far too long. Sorry about that.

• The change to stdout report feels like a no brainer.
• Removing the Noop{Report,Suite} feels good. and just using a stdout w/ /dev/null or something simplifies quite a bit

This does the multiple outputs the same way that Mini-test does it. This has been my sticking point for years, but since it is good enough for mini test, it should be good enough for me.

Let me know if/where you have apprehensions. I'm more than happy to peel anything out of here into a simpler PR if you want.

[nateberkopec]

Thanks for your work, @kbrock 🙇

[kbrock]

@nateberkopec thanks for picking up so many gems and really rocking it. Let me know if you need anything related to this or tangental.

[ioquatix]

Nice work.