Deprecate uses of `expect { }.not_to change` that RSpec 3 won't support. #380

Merged
merged 1 commit into from Dec 5, 2013

Projects

None yet

3 participants

@myronmarston
Member

This is the follow up to #376, adding deprecation warnings to 2.99 for breaking changes in 3.0 added by that PR.

/cc @soulcutter

@soulcutter soulcutter commented on an outdated diff Dec 5, 2013
spec/rspec/matchers/change_spec.rb
+
+ context 'when the value starts at a different value' do
+ before { allow_deprecation }
+
+ it 'passes when the value does not change' do
+ k = 6
+ expect { }.not_to change { k }.from(5)
+ end
+
+ it 'passes when the value does change' do
+ k = 6
+ expect { k += 1 }.not_to change { k }.from(5)
+ end
+
+ it 'issues a deprecation warning' do
+ expect_deprecation_with_call_site(__FILE__, __LINE__ + 2, /expect \{ \}\.not_to change \{ \}.from()/)
@soulcutter
soulcutter Dec 5, 2013 Member

This should also probably use Regexp.escape

@soulcutter
Member

This looks really good to me. You can merge any time (whether you want to change that one thing I noted or not is up to you)

@myronmarston myronmarston merged commit 2c46bc7 into 2-99-maintenance Dec 5, 2013

1 check passed

default The Travis CI build passed
Details
@myronmarston myronmarston deleted the deprecate-expect-not-to-change-chained-methods branch Dec 5, 2013
@mhenrixon

Could someone explain why this deprecation is done? Not complaining just wondering what I am doing wrong trying to use it.

The following tests become very different and I'd like some input on how to make them simillar again.

    context "with a completed battle for current region" do
      context "when battle was won by hero" do
        let!(:completed_battle) { create :battle_won_by_hero, player: player, region: region }

        it "returns the previous battle" do
          expect { battle_starter.start }
            .to change { battle_starter.battle }.to completed_battle
        end
      end

      context "when battle was won by boss" do
        let!(:completed_battle) { create :battle_won_by_boss, player: player, region: region }

        it "returns a new battle" do
          # This is ugly, guess I could put it in a subject instead maybe.
          battle_starter.start
          expect(battle_starter.battle).not_to eq(completed_battle)
        end
      end
    end

The second version would be to write the spec like below and this is what I would like to do:

    context "with a completed battle for current region" do
      subject { ->{ battle_starter.start } }
      context "when battle was won by hero" do
        let(:completed_battle) { create :battle_won_by_hero, player: player, region: region }
        it { should change { battle_starter.battle }.to completed_battle }
      end

      context "when battle was won by boss" do
        let(:completed_battle) { create :battle_won_by_boss, player: player, region: region }
        it { should_not change { battle_starter.battle }.to completed_battle }
      end
    end

If that isn't possible then I could think of another thing to do and that is

it { should change { battle_starter.battle }.from(nil).to(a battle that isn't the completed_battle) }

Can I achieve the last part somehow?

@mhenrixon

Never mind the specs sucked, I changed some things to make the assertion a whole lot nicer.

@myronmarston
Member

This deprecation is only making explicit what the docs have always said (since RSpec 1):

358ce67#diff-708de39e3a9d2cfd7a3dab772aac107eR138

    # == WARNING
    # <tt>should_not change</tt> only supports the form with no
    # subsequent calls to <tt>by</tt>, <tt>by_at_least</tt>,
    # <tt>by_at_most</tt>, <tt>to</tt> or <tt>from</tt>.

I was doing some refactoring and cleanup as a result of implementing support for expect { }.not_to change { }.from() as discussed in #154, and I noticed that our docs explicitly state we don't supported negated change with those chained qualifiers, but there's nothing in the code that prevents them from being used in that fashion. In general, all of them (except for the recently supported from) weaken the expectation, as it changes the meaning from "this shouldn't change", to "this can change (or not) as long as it doesn't change by/to this value". And I'm not even sure that's how the logic was working before as we never put effort into providing support for the negative case, so it was likely buggy. I think it's odd to support expect { }.not_to change { }... expressions that pass if the value changes (which is what would happen if we supported these chained expressions according to how I read them).

If you think we should formally support these, open an issue and make your case. But for now with the fact that 3.0 affords us the opportunity to raise an error since this usage has never been intended/supported, I wanted to use that opportunity.

@mhenrixon

No I am totally fine with this. Actually forced me to clean up my tests so I ended up with the following after a minimal refactoring.

  context "when player has sufficient pp" do
    context "with a completed battle for current region" do
      context "when battle was won by hero" do
        let(:completed_battle) { create :battle_won_by_hero, player: player, region: region }
        it { should eq completed_battle }
      end

      context "when battle was won by boss" do
        let(:completed_battle) { create :battle_won_by_boss, player: player, region: region }

        it { should_not eq completed_battle }
      end
    end

    context "with no completed battle for current region" do
      context "with an active battle" do
        it { should eq active_battle }
      end

      context "with no active battle" do
        it "creates a new battle" do
          expect { BattleStarter.new(player, region).start }
            .to change{ Battle.count }.by(1)
        end
      end
    end
  end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment