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

expect {E}.to_not change{X}.from(F) should fail with "should have initially been F but was f" #154

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
7 participants

I could be a minority here, but I keep finding myself expecting from() to work the same with a should_not change as it does with a should change.

The problem is that there are 2 very different ways of interpreting should_not change{X}.from(F)...

It could either mean:

(1) X should initially have a value of F; and this block should NOT cause X to change from this initial value (to any other value)

Or it could be interpreted as:

(2) X MAY change its value AS LONG AS it does not change from this specific forbidden "from" value F (in other words, the "from" value must be anything _other_ than F)

I believe both interpretations are valid. The change matcher currently implements the (2) interpretation... and this could occasionally be what you want, but IMHO I believe option (1) makes more intuitive sense, is a stronger test/expectation, and is more useful more often than is option (2).

(I believe the potential for confusion is probably a problem in general with should_not expectations, but for now just dealing with should_not change...)

(2) X MAY change its value AS LONG AS it does NOT change from this specific forbidden "from" value F)

This interpretation may be described with the following examples:

  before(:each) do
    @instance = SomethingExpected.new
    @instance.some_value = 'initial'
  end

  it "passes because it was changed from an initial value of 'initial' and we merely specified that it must not change from an initial value of 'dog'; changing from *any* non-'dog' value is still allowed" do
    expect { @instance.some_value = "cat" }.to_not change(@instance, :some_value).from("dog")
  end

  it "fails because it was changed from an initial value of 'initial' and we explicitly specified that it must *not* change from that specific value" do
    expect do
      expect { @instance.some_value = "cat" }.to_not change(@instance, :some_value).from("initial")
    end.to fail_with(%(some_value should not have changed, but did change from "initial" to "cat"))
  end

While this makes sense on some level, I have yet to see a practical example of when it would actually be useful to express an expectation like this.

If you really are okay with the value changing from some value F but aren't okay with it changing from some value G, then it seems like you should be writing this is as a (positive) should expectation about the change you do expect to see, rather than as a (negative) should_not expectation:

  it "should change from 'F' to 'cat'" do
    expect { @instance.some_value = "cat" }.to change(@instance, :some_value).from("F").to("cat")
  end

On the one hand, a should_not change().from() expectation seems too specific to be useful. It would be like testing a bunch of specific unexpected values:

x.should_not == 1
x.should_not == 2
...
x.should_not == 99

instead of simply expressing in the positive what the expected value is:

x.should == 100

On the other hand, this interpretation does more closely parallel the difference in meaning between, say:

x.should == 99       # equality with a very specific value

and

x.should_not == 99   # inequality with the same specific value

So to be fully consistent with that behavior I suppose it might make sense to implement the way it is currently implemented.

But that might be a poor comparison since should == is not a complex matcher with parameters (from(), etc.) like change is...

Besides, the failure_message_for_should_not message "should not have changed, but did change" makes it sound more like we're broadly testing that there was NO change at all than it sounds like we're testing that we didn't change in one very specific way (but may have changed in some other specific way that we didn't consider).

It seems like as a tester it's all too easy to fail to test every single way in which something should not change or behave... and end up shooting ourselves in the foot down the road when some change produces some unwanted behavior that we forgot to specify as disallowed... This is probably the case with a lot of different matchers when using should_not, not just should_not change . But now I'm rambling, sorry...

"Strengthening" tests by passing additional parameters to the matcher...

Currently when you add a from() parameter to a should_not change expectation, you actually "weaken" the test. So instead of "X should not change at all", when you add a from(F), you change it to mean "X can change practically any way it wants to (!) AS LONG AS it didn't start out with a value of F".

With proposed behavior (2), on the other hand, the meaning continues to be "X should not change at all" when you add a from(F), and then it adds additional expectations on top of that: "X should have an initial value of F".

(1) X should initially have a value of F; and this block should NOT cause X to change from this initial value

Here's a more real-world example of when I'd want it to behave like interpretation (1)...

    describe "when we attempt to re-import the file" do
      context "the file's timestamp is newer than the existing item" do
        it('should update the existing item') {
          expect { expect {
            do_import
          }.to change(Import, :count).from(1).to(2)
          }.to change { Item.last.name }.from('Test Item').to('Updated Item')
        }
      end
      context "the file's timestamp is older than the existing item" do
        it('should not update the existing item') {
          expect { expect {
            do_import
          }.to_not change(Import, :count).from(1)
          }.to_not change { Item.last.name }.from('Test Item')
        }
      end

When I discovered that the 2nd test here was passing even though Item.last.name was not initially 'Test Item', I was pretty confused. It felt like RSpec was silently ignoring my from() parameter!

And if it's just going to ignore it, I decided, then it would be better to remove the from() altogether so that future test readers wouldn't think that it's testing something that it's not!

Of course, I could always just add a separate expectation to test the "before" value, like this (and ended up doing so):

      context "the file's timestamp is older than the existing item" do
        it('should not update the existing item') {
          Item.count.should == 1
          Item.last.name.should == 'Test Item'
          expect { expect {
            do_import
          }.to_not change(Import, :count)
          }.to_not change { Item.last.name }
        }
      end

but it seems like it would be cleaner to just roll all that into my "not change" expectation... (After all, you could unroll any "change" expectation into a series of expectations before and after the code in event_proc... but the whole reason I want to use the nice rspec matchers is to express expected behavior more cleanly and concisely and, in this case, without repeating myself.)

failure_message_for_should

Another possible argument for this special "should_not change from" behavior is that the "should have initially been" failure message within failure_message_for_should is already a bit of a special case. All of the other failure messages start with "should have been changed" and take into account both what the value was before and what the value changed to, whereas "before" message is the only one that looks only at the state of things before the proc was called.

Conclusion

So there you have it — my case for changing this behavior. I'd love to hear what you think (and, if possible, point out a real-world example of when the current behavior works well)...

Even if we don't change the behavior, perhaps the docs and specs could be expanded to be clearer about how the "should_not" case is intended to behave...

Tyler Rick added some commits Jul 10, 2012

Expanded 2 examples into 4:
* fails when #after and #to are different (#from not supplied)
* fails when #after and #to are different (#from supplied but matching #before)
and
* fails when #before and #from are different (#to not supplied)
* fails when #before and #from are different (#to supplied but not used)
Changed behavior of "should_not change from" so that it tests the
initial (#from, @expected_before) value *in addition* to testing that
the value did not change at all, instead of merely testing that "it did
not change from this specific initial value".
Contributor

justinko commented Jul 11, 2012

This change makes sense to me. Will definitely need feedback from @dchelimsky @myronmarston @alindeman @patmaddox

Owner

myronmarston commented Jul 12, 2012

I think interpretation 1 makes more sense, too.

Contributor

patmaddox commented Sep 11, 2012

Interesting. I would think that expectation would pass. subject was not F to begin with, so the block certainly did not change it from F. If there's any question to the value of F or subject, that should be covered in a prior expectation.

Owner

myronmarston commented Dec 19, 2012

Looping back around on this...I agree strongly that the current behavior is a bit weird, and your proposed change is an improvement. I'm a bit worried, however, about the possibility of this being a breaking change, especially given that the right behavior is open to interpretation.

So...I think I'm going to hold off merging this now, with the plan to include it in 3.0.

@alindeman -- thoughts?

Contributor

alindeman commented Dec 20, 2012

So...I think I'm going to hold off merging this now, with the plan to include it in 3.0.

Concur!

Contributor

alindeman commented Dec 20, 2012

I added the 3.0 label.

Owner

myronmarston commented Dec 21, 2012

I added the 3.0 label.

I'm not noticing a 3.0 label on this....and actually, I was going to do that but it looks like you can't tag pull requests with labels like you can for an issue. At least, as far as I can tell. What did you do?

Contributor

alindeman commented Dec 21, 2012

Doh! It seems like it's only visible in the issues view.

Sounds good to me!

Member

cupakromer commented Jun 19, 2013

Is this still planned for 3.0?

👍 on interpretation 1. It took me a while to warp my brain into seeing how interpretation 2 would be correct; it's just not intuitive.

Owner

JonRowe commented Jul 10, 2013

I also agree with interpretation 1, so I guess this is still planned for 3

@ghost ghost assigned myronmarston Dec 3, 2013

@myronmarston myronmarston referenced this pull request Dec 4, 2013

Merged

Fixup change matcher #376

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