-
-
Notifications
You must be signed in to change notification settings - Fork 763
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
should include 'invert-tests' switch #598
Comments
A very interesting idea. It exploits the fact that matchers can be used in positive and negative modes, so you probably couldn't do it with any other lib. It would require a fair amount of additional complexity, so it's not something I'm willing to just say yes to right away. There would be some coordination between rspec-core and rspec-expectations, and could only ever work when both libs were being used together. Inverting the meaning of passing and failing is likely to require additional conditional logic in a few places, thus a significant refactoring would be required to keep things sane. Let's get some feedback from some other folks and see where this goes. |
Also - for the name - how about --invert-expectations? |
Very interesting idea. I usually change the value in the matchers, rather than should -> should_not, but I could see this being a better alternative. I like --invert-expectations |
Nice idea. This sounds a bit like mutation testing. It doesn't mutate implementation code but it does mutate the tests. 👍 |
kind of a nice idea |
Interesting idea indeed. Not sure if it is something that I would introduce into my daily coding routine (I always make sure my test fail like @coreyhaines does), but I would like to have it handy as a kind of code metric, a sanity check: measuring the quality of the tests by checking the coverage and the "reversability". So with that in mind, implementation-wise, couldn't it be something that is bolted on, like a gem, that temporarily reverses the meaning of Otherwise I think it could be horribly confusing that every true could be false when |
But I can't see where is useful in practice :. I know this increase a lot complexity and maybe will not be totally wow! |
@nathanvda an extension gem would be a great first step - it would swap should/should_not and include a formatter that reported passes as failures and failures as passes. I think we could make it more robust by incorporating into rspec proper, but that would give us a chance to validate the idea. |
👍 When working on anything substantial, there comes a time when enough of the code is written that sometimes expectations pass without any additional code changes. That makes me uneasy. At that point, I manually change the sense of the expectations to verify that, yes, we are indeed still making progress and not just fooling ourselves in a forest of doubles. |
Great idea. |
nice idea +1 |
👍 This would be especially useful when refactoring. If code is extracted into new methods and specs are reused on those new methods one does not get the chance to start with a failing test. Ensuring that the inverted specs fail will show us if we are testing what we should be. |
+1 for |
Genius idea. I give it a hearty +1 |
I +1 this idea as a separate gem to feel it out first. I have never heard of or seen anyone inverting |
I love this idea as well. I would probably not use it in my local dev routine regularly, but I'd incorporate it into my CI builds. One thing that the implementation would need to take into account is the fact that some matchers don't support being used with https://github.com/rspec/rspec-expectations/blob/v2.9.1/lib/rspec/matchers.rb#L442 So we'd probably need to add an (optional) method to the matcher protocol so that they can declare they are not invertible, and the tool that dose this can know not to bother trying with such matchers. |
+1 pretty nice idea although may be complex to implement 👍 |
I like the idea, but I find that the inverse in most cases is covered by the test itself - this seems like it probably isn't truly necessary. For example say I have the following simple example blocks: it "returns nil" do
something.should be_nil
end
it "returns 5" do
something.should eq(5)
end
it "includes the object" do
something.should include(object)
end If I inverted these to Given this, and also that I'm not keen on having rspec-expectations have a hard dependency on rspec itself, I'm inclined to say no. |
I came here to say exactly what @durran just said: "I'm struggling to find an example where this would not hold true for any matcher." This is something that would be pretty easy to add to minitest, but I'm failing to see its utility the more I think about it. We're basically talking about adding this transformation to every assertion/expectation: |
Owwww I am thinking @durran and @zenspider have a valid point. I was thinking they were simplyfying things, because I know I have written tests that actually tested the wrong thing, and still passed while my code was wrong. But, inverting those would not have worked: since the test passed, it did return TRUE or FALSE as expected, and just reverting it, will always return the opposite. It is just boolean logic, just inverting will not show us anything new indeed. It seems so obvious now, but if you have
and it passes, then there is no other way that
could fail. The Even if a matcher is incorrect, it will still return the same result. So while the idea would be incredibly interesting (could we have an automated sanity check?), just reverting the should-should_not is not going to cut it. What do you think? Who can think of an counter-example (proving that inverting the |
Thanks to all for the feedback. I agree with @durran, @zenspider, and @nathanvda that it's challenging to come up with a case where this would actually reveal any new information, and the additional complexity highlighted by @myronmarston's comments concern me as well. @rdpoor seeing as you agree that a separate gem is a sane way to go, I'm going to go ahead and close this issue. Please feel free to additional comments here as you experiment with it - especially if you find a case where this technique uncovers something that would have otherwise remained uncovered. |
Upon further consideration @zenspider is probably right. To really 'test the test' you'd want to change the subject, but that is probably not possible to do for anything but the simplest case. Anyway, I think you've made the right call @dchelimsky :) |
For people interested in 'testing your tests' I recommend taking a look at mutant |
Requesting reopen. I was just about to let this issue go to rest. But then I stumbled upon the exact reason I want --invert_expectations: BOTH of these routing tests pass. The only difference is the substitution of
There may be a perfectly sane reason that both versions pass (is there?) but --invert_expectations would have caught the error sooner. (As it stands, I have an entire file full of routing tests that are now suspect -- I have no idea if they're testing anything legit or not.) |
Just out of curiosity, what happens if you do this: it "recognizes GET index" do
assert_recognizes({ :controller => "premise_neighborhoods", :action => "index", :format => "json" },
"#{PREFIX}/premise_neighborhoods.json")
end |
It "fails as intended":
|
To anticipate your next question:
and
|
You anticipated my 3rd question :) The next question is: what does your routes file look like? |
Heh - I should have done a git commit when I first posted the question and I've been charging ahead on my code since then. Give me a day (for the tax man cometh) and I'll put together a freestanding example. Thanks. (P.S.: maybe when we're all done, --inverse-expectations will prove to useful as a debugging tool for RSpec itself.) |
PS: As for the routes file, in the spirit of TDD, I'd written the tests before adding the nested routes. But there were routes for the outer resources were routed. |
Okay - I have a nice smoking gun for you. It appears that line breaks are significant? With NO entries in routes.rb (or with a single Given a file /spec/routing/membership_spec.rb of the form
and running RSpec, I observe the following output
Note that one of each of the "fails both tests" and "passes both tests" ought to fail. But both of the former fail and both of the latter pass. The only difference between the two groups is the line break between the I've built a minimal Rails project from scratch that demonstrates the problem -- let me know if you want it as a gist or a github project or tarfile or whatever's appropriate. |
correction
then one of the test will fail (as expected):
|
OK. This does, unfortunately, make sense. Well, at least it is explainable. The I've submitted rspec/rspec-expectations#138 with a proposal to resolve. |
Joining the two lines into one was a serendipitous move -- I'm glad it yielded some insights about the issue, even if the solution is troublesome. But, as Peter Falk as Columbo might say, "I don't wanna cause any more trouble, but there's just one more thing I don't understand..." When there are NO routes, as in:
and routing tests that look like this:
both tests fail with an $SANDBOX/usr/bin/ruby -rrubygems -S '$SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/exe/rspec' --tty '~/clubbing/spec/routing/membership_spec.rb'
FF
Failures:
1) membership routing recognizes GET index
Failure/Error: { :get => "/memberships" }.should route_to( :controller => "memberships", :action => "index")
ActionController::RoutingError:
No route matches "/memberships"
# ./spec/routing/membership_spec.rb:5:in `block (2 levels) in <top (required)>'
2) membership routing does not recognize GET index
Failure/Error: { :get => "/memberships" }.should_not route_to( :controller => "memberships", :action => "index")
ActionController::RoutingError:
No route matches "/memberships"
# ./spec/routing/membership_spec.rb:8:in `block (2 levels) in <top (required)>'
Finished in 0.00584 seconds
2 examples, 2 failures
Failed examples:
rspec ./spec/routing/membership_spec.rb:4 # membership routing recognizes GET index
rspec ./spec/routing/membership_spec.rb:7 # membership routing does not recognize GET index If the tests are improperly written, what's the right way to write them? |
Would you please run that last example with the --backtrace flag and post the full backtraces for both examples? |
Why certainly... $ rspec --backtrace ~/clubbing/spec/
FF..
Failures:
1) membership routing with expectations on one line recognizes GET index
Failure/Error: { :get => "/memberships" }.should route_to( :controller => "memberships", :action => "index")
ActionController::RoutingError:
No route matches "/memberships"
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/actionpack-3.2.1/lib/action_dispatch/routing/route_set.rb:622:in `recognize_path'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/actionpack-3.2.1/lib/action_dispatch/testing/assertions/routing.rb:210:in `recognized_request_for'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/actionpack-3.2.1/lib/action_dispatch/testing/assertions/routing.rb:42:in `assert_recognizes'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-expectations-2.9.0/lib/rspec/matchers/built_in/base_matcher.rb:29:in `match_unless_raises'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-expectations-2.9.0/lib/rspec/expectations/handler.rb:9:in `handle_matcher'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-expectations-2.9.0/lib/rspec/expectations/extensions/kernel.rb:12:in `should'
# ./spec/routing/membership_spec.rb:5:in `block (3 levels) in <top (required)>'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:80:in `instance_eval'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:80:in `block in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:173:in `with_around_hooks'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:77:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:355:in `block in run_examples'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:351:in `map'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:351:in `run_examples'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:337:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:338:in `block in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:338:in `map'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:338:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:28:in `map'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:28:in `block in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/reporter.rb:34:in `report'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:25:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/runner.rb:69:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/runner.rb:10:in `block in autorun'
2) membership routing with expectations on one line does not recognize GET index
Failure/Error: { :get => "/memberships" }.should_not route_to( :controller => "memberships", :action => "index")
ActionController::RoutingError:
No route matches "/memberships"
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/actionpack-3.2.1/lib/action_dispatch/routing/route_set.rb:622:in `recognize_path'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/actionpack-3.2.1/lib/action_dispatch/testing/assertions/routing.rb:210:in `recognized_request_for'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/actionpack-3.2.1/lib/action_dispatch/testing/assertions/routing.rb:42:in `assert_recognizes'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-expectations-2.9.0/lib/rspec/matchers/built_in/base_matcher.rb:29:in `match_unless_raises'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-expectations-2.9.0/lib/rspec/expectations/handler.rb:32:in `handle_matcher'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-expectations-2.9.0/lib/rspec/expectations/extensions/kernel.rb:24:in `should_not'
# ./spec/routing/membership_spec.rb:8:in `block (3 levels) in <top (required)>'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:80:in `instance_eval'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:80:in `block in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:173:in `with_around_hooks'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example.rb:77:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:355:in `block in run_examples'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:351:in `map'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:351:in `run_examples'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:337:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:338:in `block in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:338:in `map'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/example_group.rb:338:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:28:in `block (2 levels) in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:28:in `map'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:28:in `block in run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/reporter.rb:34:in `report'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/command_line.rb:25:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/runner.rb:69:in `run'
# $SANDBOX/usr/lib/ruby/gems/1.9.1/gems/rspec-core-2.9.0/lib/rspec/core/runner.rb:10:in `block in autorun'
Finished in 0.00691 seconds
4 examples, 2 failures
Failed examples:
rspec ./spec/routing/membership_spec.rb:4 # membership routing with expectations on one line recognizes GET index
rspec ./spec/routing/membership_spec.rb:7 # membership routing with expectations on one line does not recognize GET index |
@dchelimsky: it seems that the " |
@rdpoor rspec-rails, please |
@dchelimsky: done. thanks. |
I find myself in a situation needing to double check that both the expectation is ok and the not expectation must fail, because sometimes both pass.... For whatever reason (broken test code after refactoring tests, we did not notice that because tests still pass...) My trick is currently to patch the expectations lib (comments removed):
def _to(matcher=nil, message=nil, &block)
prevent_operator_matchers(:to) unless matcher
RSpec::Expectations::PositiveExpectationHandler.handle_matcher(target, matcher, message, &block)
end
def _not_to(matcher=nil, message=nil, &block)
prevent_operator_matchers(:not_to) unless matcher
RSpec::Expectations::NegativeExpectationHandler.handle_matcher(target, matcher, message, &block)
end
# alias not_to _not_to
# alias to _to
# alias to_not _not_to
alias to _not_to
alias not_to _to
alias to_not _to I activate either the one or other aliases. Wouldn't that do the trick in case of a global flag as well? Do I miss something? I read about a high complexity to implement the feature, I do not understand from a functional point of view why a negation of tests can be so complicated to implement... That is why I would like to get some more info and adjust my checks to solve my current problems with the test code correctly. |
Sorry again I know this ticket is closed but this functionality would be really good to have. I now slowly realize that integrating it is indeed challenging at some points, once it comes to reporting something adequate for example. But inverting could be considered as "meta" testing = testing the tests and instead of aliasing define a new method that gets the result of the actual test and expects it to be a failure... I am not familiar with the code at all, I am just throwing ideas... If there is a wish to get such a feature implemented from the community side, if I get some hints on how this could/should roughly be done, I can invest some time in digging into it. At the moment we really suffer from tests that are wrong (both the test and its negation pass) and difficult to detect in the mass. |
As you've found it's relatively easy to integrate a temporary negation of assertions, the issue becomes in integrating this feature across And then there is the point that for most, if not all of RSpec's matchers it would always pass, I can't think of any examples of our matchers that would pass in the positive and then not fail when negated... I feel that broken test code is a different class of problem and there is no guarantee that if this feature existed it would catch it any better. You could wrap Can you provide some examples of where a matcher passes for both |
You are fully right, the main reason for such situations is that the test code itself was too complicated and ended up in situations where no expect at all was called. Thanks for your feedback, we have a pretty big code base here maybe I can see some other possibility to improve our situation. I now can fully agree on that this is a different class of problem. Thanks for taking the time to answer, I'll check your hints as well to see if that helps me any further. |
Theres a long standing suggestion to implement expectation counting, but no ones ever had time to do it, its the sort of thing that needs to happen before we can enforce that an expectation happens (currently we never know really unless theres a failure) but there are some not so easy bits to that too (mostly surrounding how you deal with complex assertions and composed matchers). |
Oh dear. Still the goal "counting expects" sounds definitively reasonable and useful. I'll make myself a picture of it. |
@jdehaan Are you up to come up with a PoC that would make sure that at least one expectation per example has been made, and warn in case |
@pirj that is what I would like to do but I am not ready (in terms of knowledge of the framework) for that. I read several other related tickets (with similar goals like #404, #591, #740, #759) and need to get a good picture of what is going on. I think the first thing in the way is that a test that checks something does not raise an exception is valid (as current state of the implementation following things work well: #404 (comment)) and changing that to forced explicit expectations is a breaking change. As of my current research one thing that could also solve the dilemma is to change the initial state of an example to be pending by default and toggle to success if there is a successful expectation (with any framework used for that) and stick to failure on the first failed expectation. But that is just one idea among many. I thought initially that must not be a big deal but the further you get into it the more complicated it gets. My feeling is that there is something in the design that makes this more complicated than needed. I try to get a first clear mind on what are the design goals of RSpec and how an option could make sense in an universal way. At the moment I solved my issue with a manual quirk and it's not something that makes sense in a general way because RSpec is so much more flexible and modular allowing all kinds of combinations of frameworks. I try to see if one can make things fit from the example runner perspective without touching the rest (probably at the cost of an option that changes the behaviour of the runner in a breaking way, what is not a big deal if you want/need this feature... Maybe it could be made configurable at example group level?) |
As best I can understand, a properly written test should ALWAYS fail when its "should" is replaced with "should_not" and vice versa.
I often interchange "should" and "should_not" to make sure my tests fail as expected. If a test "fails to fail", I know it isn't really testing what I think it's testing -- a very important bit of information. But this requires a lot of manual work.
It would be useful if RSpec offered an
--invert-tests
command-line switch which has the following effects:(I was tempted to call this the
--null-hypothesis
switch, but that may simply be too cute. What do you think?)The text was updated successfully, but these errors were encountered: