rspec / rspec-core

RSpec runner and formatters
http://rspec.info
MIT License
1.22k stars 763 forks source link

Show number of assertions in output. #740

Open utkarshkukreti opened 11 years ago

utkarshkukreti commented 11 years ago

I searched around, but couldn't find a way to do it. Something like:

1 example, 0 failures, 3 assertions

In this question, @dchelimsky suggested to open a feature request, so here I am :smiley_cat:

Edit: @xaviershay's summary of comments below as of Jan 2017:

ethnt commented 11 years ago

:thumbsup: It would be good information to have, even if it is best practice to have only one assertion per test (see Better Specs).

utkarshkukreti commented 11 years ago

@eturk, interesting link.

If I were to follow that guideline, how should I rewrite this block?

it "should parse chained monads/dyads" do
  eval1("1 + 2 + 3 4 + 4 5").should eq [10, 12]
  eval1("1 - 2 * 4 + 4 5").should eq [-15, -17]
  eval1("1 - 2 * 4 + 4 5").should eq [-15, -17]
  eval1("1 - 1 - 1 2 3 + 3 4 5 - 6 7 8").should eq [-2, -1, 0]
end

(eval1 is just a helper to parse, transform, and evaluate the string).

dchelimsky commented 11 years ago

One expectation per example is a useful guideline, not a moral imperative. @utkarshkukreti the risk you run with your example is that if the first statement fails and you get it to pass, you might find that the 2nd is now failing. Then you get that passing and the third one might be failing, etc.

Having each statement in separate examples gives you clearer insight into what the underlying problem is.

You could rewrite your example with a custom matcher. Something like

context "parsing chained modads/dyads" do
  it { should parse("1 + 2 + 3 4 + 4 5").as([10, 12])}
  it { should parse("1 - 2 * 4 + 4 5").as([-15, -17])}
  # etc
end
vanstee commented 11 years ago

Is this issue resolved?

ethnt commented 11 years ago

Would it be best to wait and see if anyone else wants to give their opinion? I'm for it, even if it is a best practice to write one assertion per example.

Shall I begin writing an implementation?

myronmarston commented 11 years ago

I'm moderately in favor of the idea, so sure....@eturk, feel free to take a stab at this :).

ethnt commented 11 years ago

The output will look something like:

1 example (1 assertion), 0 failures

Is this going to be an optional configuration option or on by default?

soulcutter commented 11 years ago

How about 1 example, 0 failures, 3 assertions ? Or at least 1 example, 0 failures (3 assertions). It seems a bit weird to break up the output in the middle.

I feel like this should be an optional configuration option that is off by default. Maybe I'm being over-conservative, but I feel that changes to output format should be introduced gradually.

ethnt commented 11 years ago

1 example, 0 failures, 3 assertions looks good.

I agree that it should be optional. How does the --assertions flag sound?

dchelimsky commented 11 years ago

It's called rspec-expectations for a reason: assertion != expectation. Please use "expectation/s" instead of "assertion/s".

myronmarston commented 11 years ago

Is this going to be an optional configuration option or on by default?

I like making RSpec configurable, but with a feature like this that gives the user more info in the output, I have a hard time coming up with a reason why a user would want it off, or coming up with a way this could be a breaking change for anyone. So I lean towards not making it a config option. We don't want the complexity of a config option for each bit of output the formatter spit out.

@soulcutter -- what problems do you think it could cause someone by adding this bit of output to the formatters?

That said, there is one thing that probably should be configurable: what exception classes are considered to be expectations. We don't want rspec-core to be coupled to rspec-expectations in this regard, and we should make this feature usable by folks who use other libraries (e.g. minitest's assertions or wrong). Something like:

RSpec.configure do |c|
  c.expectation_failure_errors = [SomeErrorClass, SomeOtherErrorClass]
end

rspec-expectations can use this API if RSpec.respond_to?(:configure) so that it auto-configures this correctly.

Actually, I guess this config option would provide a simple way to disable this behavior: if you assigned the config option to an empty array (or nil, if we want to support that), then it would effectively turn it off as rspec-core would have no way to distinguish exception types.

myronmarston commented 11 years ago

Actually, ignore most of what I just wrote. I just realized that this feature is to list the number of expectations, not simply to distinguish between expectation failures and other types of errors. (Sorry, I was thinking about this all wrong!)

So the difficulty here is that rspec-expectations should be the place that keeps track of the # of expectations, but we don't to couple rspec to it. So...maybe the config should be something like this?

RSpec.configure do |c|
  c.fetch_expectation_count_from do
    RSpec::Expectations.expectation_count
  end
end

(I'm not happy with the name, but you get the idea: it's a block that's called to fetch the number of expectations when the formatter is printing the numbers. rspec-expectations can still self-configure this when loaded).

soulcutter commented 11 years ago

@myronmarston I am hard pressed to think of what changing the output would break. I think you are right, and I should have paid attention to my instinct that I was being over-conservative.

As far as the block syntax goes, maybe it would make more sense to register objects which are called for summation? e.g.

RSpec.configure do |c|
  # this would be a default, so you wouldn't really do this, but for other libs or something... 
  c.expectation_counters << RSpec::Expectations.public_method(:expectation_count) 
end
ethnt commented 11 years ago

Isn't RSpec::Expectations included along with RSpec::Core when you install rspec? If that's the case, you wouldn't need to call RSpec::Expecations.public_ethod(:expectation_count) explicitly, correct? Then it would make sense to just do c.expectation_counts, true.

myronmarston commented 11 years ago

Isn't RSpec::Expectations included along with RSpec::Core when you install rspec ?

rspec-core mixes RSpec::Matchers into RSpec::Core::ExampleGroup (so the matchers are available in the example context).

If that's the case, you wouldn't need to call RSpec::Expecations.public_ethod(:expectation_count) explicitly, correct? Then it would make sense to just do c.expectation_counts, true.

We don't want rspec-core to be directly coupled to rspec-expectations, and thus it shouldn't be hardcoded to get the expectation count using any particular method name. If we expose a config option for this, rspec-expectations can easily detect that RSpec.configure is available, and, if so, set the config option so that rspec-core gets it's expectation count from rspec-expectations.

Does that make sense?

@soulcutter -- On the surface I like how that reads a bit better, but the semantics of supporting multiple expectation counters seems a bit weird...would it just sum all of them? Also, the call idea makes sense so people can pass lambdas, but I feel like blocks are much more idiomatic...and you can always do &object.method(:some_method) to pass a method on as a block.

soulcutter commented 11 years ago

@myronmarston I am imagining a large test suite that may use both RSpec expectations and wrong (or some other lib). I feel like this would be a fairly common use case - I've dealt with several heterogenous test suites in my experience.

So yes, multiple expectation counters would be summed together for the output (something like config.expectation_counters.inject(0) { |sum, x| sum + x.call } ). I'm not sure I understand how blocks would work with multiple expectation counters. I don't think it would be more idiomatic to take a block inside which you did the summing of multiple libs yourself... I'm not sure I understand your point there.

ethnt commented 11 years ago

@myronmarston Ah, okay โ€” I wasn't quite sure, I've never developed on RSpec before. Thanks for clarifying and your patience with me! :smiley:

myronmarston commented 11 years ago

Random thought I just had: if we come up with the right API, rspec-mocks (or any mocking framework, really) could use this to provide end-of-run reporting of the number of mock expectations. Maybe something like:

RSpec.configure do |c|
  c.add_end_of_run_count("expectations") do
    RSpec::Expectation.expectation_count
  end

  c.add_end_of_run_count("mock expectations") do
    RSpec::Mocks.mock_expectation_count
  end
end

This would produce output like:

20 examples, 2 failures, 25 expectations, 3 mock expectations

Taking this a step further, the existing example and failure counts could be refactored to use this more flexible API (i.e. rspec-core could itself register end-of-run counts for examples and failures). Really, anything could use this to report counts at the end. WebMock could use this to report the number of HTTP mock expectations, for example.

If we go this route, we also have to decide how to handle pluralization (e.g. 1 expectation vs 2 expectations). We could do something like:

RSpec.configure do |c|
  c.add_end_of_run_count("expectation", "expectations") do
    RSpec::Expectation.expectation_count
  end
end

...where the first arg is the singular description, and the second arg is the plural description. We could also easily support add_end_of_run_count("widget(s)")--that is, if only one arg is given, use it for both singular and plural.

Thoughts?

soulcutter commented 11 years ago

@myronmarston I like where you are going with that. Generalizing the behavior makes a lot of sense...

I may try to make a pass at a more general system. Don't let me stop you, @eturk , from the more specific originally requested feature - I think that's a good step to take regardless.

ethnt commented 11 years ago

@soulcutter @myronmarston I'll try to work on it, but to be perfectly honest, I won't iterate that quickly. I'll definitely try to make a (very) simplistic version of it, but I wouldn't count on me to have it done in a timely manner. I should've been more clear about that to begin with.

soulcutter commented 11 years ago

@eturk Of course! I didn't want you to have development paralysis because of this discussion. No pressure, carry on :)

JonRowe commented 11 years ago

Made any progress @eturk? ( No pressure just a friendly triage reminder :) )

fables-tales commented 11 years ago

Random thought: why not make it count expect and should style expectations differently. By default we just show a count of both, but we can then have a flag that shows users how many should style things they've got left in their specs. This could be used for people trying to port to the new, preferred, expect syntax, when they hit should zero, they know they've done it.

JonRowe commented 11 years ago

I can see how that would be useful, but we would need the more flexible count api @myronmarston suggested, (as in my mind it should be agnostic)

myronmarston commented 11 years ago

Random thought: why not make it count expect and should style expectations differently. By default we just show a count of both, but we can then have a flag that shows users how many should style things they've got left in their specs. This could be used for people trying to port to the new, preferred, expect syntax, when they hit should zero, they know they've done it.

I can see the usefulness of this during a transition period, but in general, this sounds like added noise and complexity that wouldn't usually be useful -- so I don't think I'm in favor of this. (I could be convinced otherwise, though).

ethnt commented 11 years ago

I haven't actually looked at this in a while. When I get the time I'll take a look at it. Just to be clear, here's what we're implementing (eventually).

RSpec.configure do |c|
  c.add_end_of_run_count("expectations") do
    RSpec::Expectation.expectation_count
  end
end

This should be pretty easy to do in rspec-core: a method that takes a String and a Block, and just append the contents of that Block (hopefully an Integer) along with the String.

Would it be better to have the current example count output adhere to this as system as well?

myronmarston commented 11 years ago

Would it be better to have the current example count output adhere to this as system as well?

Yep, I like the idea of providing a generalized way of registering end-of-run counters, and then have rspec-core simply use these APIs for its end-of-run counters.

JonRowe commented 11 years ago

How would this hook into the Reporter? As the current implementation is eventually passed into this...?

ethnt commented 11 years ago

We could rewrite Reporter like so:

def add_count(count)
  @counts << count
end

def finish(seed)
  begin
    stop
    notify :start_dump
    notify :dump_pending
    notify :dump_failures
    notify :dump_summary, @duration, @counts
    notify :seed, seed if seed
  ensure
    notify :close
  end
end

Then we have Configuration#add_end_of_run_count pass it's contents to Reporter#add_count.

Again, I'm not that familiar with the way RSpec is structured, but this is my understanding.

JonRowe commented 11 years ago

Agreed, we'd have to do something along those lines, but as I pointed out elsewhere, this would break all the formatters. So again it'd be a breaking change which we'd have to leave till 3...

ethnt commented 11 years ago

I suppose a temporary fix would be using @example_count, @failure_count, and @pending_count if they aren't empty, otherwise using the new implementation. This is quite hackety-hax though... I'm all ears for any non-breaking alternatives.

williscool commented 11 years ago

after being frustrated with this for a while (having come to expect an assertion count from coming test unit) and thinking about it ...

@dchelimsky 's comment https://github.com/rspec/rspec-core/issues/740#issuecomment-10715691

is technically a pretty good work around.

using a context and wrapping the individual assertions in it calls would give you better context on what actually broke.

Its a different mental model;

Lots of small tests, vs one big one with many smaller assertions.

Don't know which one is better. Sounds like a philosophical debate to me.

But that will work until our fearless leaders decide on this feature

JonRowe commented 11 years ago

IMO lots of small tests is better, especially as if you need one big test because of setup cost that's probably a sign that your code needs some refactoring anyway, but that's just my 2ยข.

This being a breaking change isn't blocker, it just means it won't go into 2.14, as an aside @myronmarston @samphippen @soulcutter I think we should look to refactor reporter to allow changes like this to go through, I mean not all formatters need to listen to all notifications? I have an idea how to do this, maybe look at registering formatters as listening to certain notifications, and then only sending the notifications to those registered formatters. Would also allow multiple formatters (which would admittedly only be useful if you had a series of smaller reports, e.g. one doing code stats, whilst another is the cmd line output) etc...

dchelimsky commented 11 years ago

Multiple formatters are already supported.

soulcutter commented 11 years ago

What if dump_summary could accept some sort of SummaryStats class instance (or even just a hash, though that wouldn't give us as good of an opportunity to document/deprecate) so that we can add stats without breaking existing formatters? There are a few ways we could transition to this... but in any case, I'd hesitate to just add yet another argument to dump_summary

I'm not sure adding formatter registration for different notifications is worth doing since extending from BaseFormatter makes most events no-ops, and also serves as documentation of the available events. I could be convinced, but I think it's somewhat orthogonal to adding a new run statistic.

JonRowe commented 11 years ago

@dchelimsky Ah ok, I wasn't aware that was the case.

@soulcutter It would give us the freedom to make changes around this without making breaking changes or requiring people to extend BaseFormatter

bootstraponline commented 8 years ago

I needed this for work so I came up with a gem. It'd be nice to have a proper API.

One issue with counting at the rspec level is that there's no way to distinguish between retried expectations and regular one time rspec core expectations.

rosenfeld commented 8 years ago

:+1: I'd love to get such report from RSpec.

adrianthedev commented 6 years ago

+1 for this feature. Wuould love to have more feedback about the tests I write.

pirj commented 4 years ago

Since there's a gem for it (https://github.com/rspec/rspec-core/issues/740#issuecomment-155091855), does this still stand?

Schwad commented 2 years ago

This would be incredibly helpful to have! I'd love to see how many assertions I'm including in my test (and help spot tests missing assertions!

Since there's a gem for it (https://github.com/rspec/rspec-core/issues/740#issuecomment-155091855), does this still stand?

My :twocents: is it still stands. It'd make a great candidate for core and I'm unsure the current state of that gem as the build is failing ๐Ÿค”

n-rodriguez commented 3 weeks ago

Hi there! Any news?

n-rodriguez commented 3 weeks ago
module RSpecPatch
  module Expectations
    def expectation_count
      @expectation_count ||= 0
    end

    def update_expectation_count
      @expectation_count = expectation_count + 1
    end
  end

  module Matchers
    def expect(*args, &block)
      RSpec::Expectations.update_expectation_count
      super
    end
  end

  module SummaryNotification
    def totals_line(*args)
      text = super
      count = RSpec::Expectations.expectation_count
      message = RSpec::Core::Formatters::Helpers.pluralize(count, "expectation")
      "#{text.chomp}, #{message}\n"
    end
  end
end

unless RSpec::Expectations.respond_to?(:expectation_count)
  RSpec::Expectations.extend(RSpecPatch::Expectations)
  RSpec::Matchers.prepend(RSpecPatch::Matchers)
  RSpec::Core::Notifications::SummaryNotification.prepend(RSpecPatch::SummaryNotification)
end
JonRowe commented 3 weeks ago

No news, as far as I know no one is actively working on it

n-rodriguez commented 3 weeks ago

The patch I've posted above works very well ๐Ÿ˜ƒ