Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Show warning for examples that don't include an expectation #759

Open
sorah opened this Issue Dec 29, 2012 · 15 comments

Comments

Projects
None yet
10 participants
Contributor

sorah commented Dec 29, 2012

I'm doing the following hack to do this in our repository.

I want to introduce this to rspec-core, but I can't decide which repository (rspec-{mocks,expectations, etc}) to write a patch and send pull-request.

In that patch, it crosses rspec-mocks and expectations, which should I send a patch? rspec-core?

(I know that patch is little bit ugly (optimized as monkey patch), I'll fix for pull request.)

Owner

dchelimsky commented Dec 29, 2012

This is a duplicate (ish) of #591. My opinion is we shouldn't support this for reasons explained in that issue, but @myronmarston may have a different opinion so I'll leave it open for his comment.

Contributor

sorah commented Dec 29, 2012

Oops, I missed that issue by simple researching.

Now I've read that issue, how about make this optional stuff?

(Don't you like adding configuration like this?)

Owner

myronmarston commented Dec 29, 2012

There's also been a lot of discussion about this same idea in #404.

I had a few concerns at the time of that issue:

  • It didn't take into account examples that used only a mock expectation. (But it looks like your implementation has addressed that).
  • At the time I favored using no expectation over expect { }.not_to raise_error due to wanting the original error's backtrace. (But now that we've solved rspec/rspec-expectations#59, that's not a concern anymore (for me, anyway).
  • I didn't want any added coupling between rspec-core, rspec-expectations and rspec-mocks. (This is still a concern of mine.)

I'd say I'm more on board with the idea now than I was before, as long as it's an optional config, it takes into account mock expectations, is not too complex, and doesn't introduce additional coupling between the rspec gems.

One other thing to consider is #740. For that one we are discussing an API that rspec-mocks and rspec-expectations can use to register callbacks rspec-core can use to get the counts of expectations/mock-expectations that have been set. I wonder if that could be used here? It's easy to imagine the logic for this implemented by comparing before and after counts of expectations/mock-expectations, and such an API would provide an appropriate level of de-coupling.

Double plus on the optional setting. Making it required would break "natural assertions" provided by RSpec/Given (https://github.com/jimweirich/rspec-given/tree/natural#natural-assertions)

Contributor

sorah commented Dec 31, 2012

I didn't want any added coupling between rspec-core, rspec-expectations and rspec-mocks. (This is still a concern of mine.)

Yes, I had same concern.

#740 looks good solution to de-couple. (see counter to judge there was no expectations or not)

Owner

JonRowe commented Apr 18, 2013

Closing because of the reasons outlined in #591, #404. I hope we can create something usable for you in #740, feel free to chime in with help there.

@JonRowe JonRowe closed this Apr 18, 2013

Contributor

mvz commented Nov 11, 2013

+1:

As someone who just came across a whole spec file full of 'expectations' looking like some_array =~ [foo, bar], I would really appreciate this feature.

Of course one should always write a failing spec first, but sometimes people don't, and this feature would help in making the resulting mess a bit less.

In the mean time, I will be trying the method used in http://blog.sorah.jp/2012/12/17/rspec-warn-for-no-expectations

Owner

myronmarston commented Feb 18, 2014

I think we should reconsider doing this. There's clearly demand for it since it's come up a number of times (and yet again on the mailing list) and I've just thought of an implementation that's really simple:

  • Have rspec-core define an expect method that increments :expectation_counter in the example metadata and uses super to delegate to the rspec-expectations and rspec-mocks.
  • Add a config option that adds an after hook that makes the example fail or be marked pending if the example metadata has an :expectation_counter of zero.

This could also be used for #740 as an alternative implementation to having rspec-expectations and rspec-mocks expose counts. One downside to this implementation idea is that it only works with the expect syntax. Personally, I'm fine with that since it's the direction we're going (and the fact that expect provides an extension point for rspec-core is one additional benefit of it), but it could possibly be confusing for users.

@myronmarston myronmarston reopened this Feb 18, 2014

Hopefully this can be done in a way that won't break natural assertions in
RSpec-Given (which uses neither the should or expect syntax). All I would
need is a public API that can signal "Hey, an assertion has been made,
don't complain about a missing expect/should").

On Tue, Feb 18, 2014 at 10:54 AM, Myron Marston notifications@github.comwrote:

Reopened #759 #759.

Reply to this email directly or view it on GitHubhttps://github.com/rspec/rspec-core/issues/759
.

Owner

myronmarston commented Feb 19, 2014

All I would
need is a public API that can signal "Hey, an assertion has been made,
don't complain about a missing expect/should").

I'm imagining this would be a config option that would default to do nothing, so rspec-given users would only be affected if they enabled the config option.

Beyond that, are you satisfied with expect(true).to eq(true) (or some such always passing expectation) as a sufficient public API?

@myronmarston thanks for re-opening this after my post on the forum, & thanks for the link to sorah.jp - I'll give that a try for now.

All I would
need is a public API that can signal "Hey, an assertion has been made,
don't complain about a missing expect/should").

If that was a hook to rspec-core, it would let others develop other extensions like RSpec-Given, and also (potentially) enable reporting like this:

10 examples, 23 assertions, 0 failures, 3 pending

(I know many consider >1 assertions per example a bad practice, but it can be useful.)

@cupakromer cupakromer referenced this issue Mar 16, 2014

Merged

Warn on pending block #1394

3 of 3 tasks complete

@myronmarston myronmarston added this to the Post 3.0 milestone Mar 24, 2014

@myronmarston myronmarston changed the title from Show warning for exceptions that forgot to write `should` to Show warning for exceptions that forgot to write an expectation May 20, 2015

I would love to work on this. I talked to @myronmarston and @samphippen a few days ago about how to go about implementing this as a plugin. But I was pointed towards this thread instead since it is a requested feature in RSpec.

@samphippen samphippen changed the title from Show warning for exceptions that forgot to write an expectation to Show warning for examples that don't include an expectation Apr 17, 2016

ojab commented Sep 21, 2016

AFAIU it can be done by checking if RSpec::Matchers.last_expectation_handler is not nil in run_after_example before assign_generated_description is called (which calls RSpec::Matchers.clear_generated_description on line 525 and sets last_expectation_handler to nil).

What I'm missing here?

ojab commented Sep 21, 2016

(and hacked up rspec-core that basically do $stderr.puts if RSpec::Matchers.last_expectation_handler.nil? passes smoke testing: warning if there is no expect/should in example, no warning if expectation exists)

Owner

xaviershay commented Jan 15, 2017

I think if we do #740 as a prerequisite, this feature would be pretty straightforward to do in a third-party formatter.

Leaving this open for now since it does describe a different feature, but I'm pretty sure we'd go the third party route (at least initially) so may close it once #740 is done.

@xaviershay xaviershay added the Feature label Jan 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment