Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Show number of assertions in output. #740

Open
utkarshkukreti opened this Issue · 36 comments

9 participants

@utkarshkukreti

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

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

@eturk

: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

@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
Owner

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

Is this issue resolved?

@eturk

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

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

@eturk

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
Collaborator

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.

@eturk

1 example, 0 failures, 3 assertions looks good.

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

@dchelimsky
Owner

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

@myronmarston

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

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
Collaborator

@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
@eturk

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

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
Collaborator

@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.

@eturk

@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

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
Collaborator

@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.

@eturk

@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
Collaborator

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

@JonRowe
Owner

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

@samphippen
Collaborator

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
Owner

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

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).

@eturk

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
Owner

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
Owner

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

@eturk

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
Owner

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...

@eturk

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

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 #740 (comment)

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
Owner

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
Owner

Multiple formatters are already supported.

@soulcutter
Collaborator

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
Owner

@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

@myronmarston myronmarston added this to the Post 3.0 milestone
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.