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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a warning when the should syntax is used. #339

Merged
merged 1 commit into from Sep 10, 2013

Conversation

Projects
None yet
5 participants
@samphippen
Member

samphippen commented Jul 4, 2013

This is for RSpec 3 with a view to letting users know that this syntax will be off by default in 4. I seem to recall that there was some discussion of this, but I couldn't find the exact issue/mailing list post. I'll bake one for rspec-expectations whilst I'm on the plane 馃惐

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 4, 2013

Coverage Status

Coverage remained the same when pulling 44f35a5 on samphippen:warn_about_the_should_syntax into 6ef6f11 on rspec:master.

coveralls commented Jul 4, 2013

Coverage Status

Coverage remained the same when pulling 44f35a5 on samphippen:warn_about_the_should_syntax into 6ef6f11 on rspec:master.

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

Somehow I missed the decision. I thought since the should syntax doesn't have much maintenance cost, we had no plans to deprecate or remove it, except to start enabling it only in generated spec_helper.rb files.

Contributor

alindeman commented Jul 4, 2013

Somehow I missed the decision. I thought since the should syntax doesn't have much maintenance cost, we had no plans to deprecate or remove it, except to start enabling it only in generated spec_helper.rb files.

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

(If it's not obvious, I'm 馃憥 this change because this will spew warnings for all but the very newest test suites, and it's not entirely automatable to change the syntax)

Contributor

alindeman commented Jul 4, 2013

(If it's not obvious, I'm 馃憥 this change because this will spew warnings for all but the very newest test suites, and it's not entirely automatable to change the syntax)

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jul 4, 2013

Member

It warns precisely once for the entire suite as opposed to on every should.
It emits the warning once on the first should and then Not again. Hardly
"spewing warnings".

Sent from my phone please excuse my brevity.

On 4 Jul 2013, at 18:03, Andy Lindeman notifications@github.com wrote:

(If it's not obvious, I'm [image: 馃憥] this change because this will spew
warnings for all but the very newest test suites, and it's not entirely
automatable to change the syntax)


Reply to this email directly or view it on
GitHubhttps://github.com//pull/339#issuecomment-20484300
.

Member

samphippen commented Jul 4, 2013

It warns precisely once for the entire suite as opposed to on every should.
It emits the warning once on the first should and then Not again. Hardly
"spewing warnings".

Sent from my phone please excuse my brevity.

On 4 Jul 2013, at 18:03, Andy Lindeman notifications@github.com wrote:

(If it's not obvious, I'm [image: 馃憥] this change because this will spew
warnings for all but the very newest test suites, and it's not entirely
automatable to change the syntax)


Reply to this email directly or view it on
GitHubhttps://github.com//pull/339#issuecomment-20484300
.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Jul 4, 2013

Member

I just worked out where the discussion was. It's on the 3.0 release post
gist. Won't link on case @myronmarston wants to keep it under wraps for
now. @alindeman you should have access to it :).

Sent from my phone please excuse my brevity.

On 4 Jul 2013, at 18:03, Andy Lindeman notifications@github.com wrote:

(If it's not obvious, I'm [image: 馃憥] this change because this will spew
warnings for all but the very newest test suites, and it's not entirely
automatable to change the syntax)


Reply to this email directly or view it on
GitHubhttps://github.com//pull/339#issuecomment-20484300
.

Member

samphippen commented Jul 4, 2013

I just worked out where the discussion was. It's on the 3.0 release post
gist. Won't link on case @myronmarston wants to keep it under wraps for
now. @alindeman you should have access to it :).

Sent from my phone please excuse my brevity.

On 4 Jul 2013, at 18:03, Andy Lindeman notifications@github.com wrote:

(If it's not obvious, I'm [image: 馃憥] this change because this will spew
warnings for all but the very newest test suites, and it's not entirely
automatable to change the syntax)


Reply to this email directly or view it on
GitHubhttps://github.com//pull/339#issuecomment-20484300
.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 4, 2013

Coverage Status

Coverage remained the same when pulling f14c98f on samphippen:warn_about_the_should_syntax into 6ef6f11 on rspec:master.

coveralls commented Jul 4, 2013

Coverage Status

Coverage remained the same when pulling f14c98f on samphippen:warn_about_the_should_syntax into 6ef6f11 on rspec:master.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 4, 2013

Member

Somehow I missed the decision. I thought since the should syntax doesn't have much maintenance cost, we had no plans to deprecate or remove it, except to start enabling it only in generated spec_helper.rb files.

We have no plans to remove it. Given that we think the expect syntax is superior, and that we'd like to move in the direction of making rspec do zero monkey patching by default, the plan we had discussed was for should to still be enabled by default in RSpec 3 but to have it issue a "you should explicitly enable this" warning, and then in RSpec 4 it will be disabled by default. That'll help make our preference clear, and help nudge folks towards explicitly configuring the syntax if they want to continue using should.

Anyhow, let's hold off on this for now. I want to get community feedback on it first (based on the RSpec 3 blog post). There are plenty of other rspec 3 things we can work on in the meantime (e.g. removing stuff that's been deprecated for a long time).

Member

myronmarston commented Jul 4, 2013

Somehow I missed the decision. I thought since the should syntax doesn't have much maintenance cost, we had no plans to deprecate or remove it, except to start enabling it only in generated spec_helper.rb files.

We have no plans to remove it. Given that we think the expect syntax is superior, and that we'd like to move in the direction of making rspec do zero monkey patching by default, the plan we had discussed was for should to still be enabled by default in RSpec 3 but to have it issue a "you should explicitly enable this" warning, and then in RSpec 4 it will be disabled by default. That'll help make our preference clear, and help nudge folks towards explicitly configuring the syntax if they want to continue using should.

Anyhow, let's hold off on this for now. I want to get community feedback on it first (based on the RSpec 3 blog post). There are plenty of other rspec 3 things we can work on in the meantime (e.g. removing stuff that's been deprecated for a long time).

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Jul 4, 2013

Contributor

It warns precisely once for the entire suite as opposed to on every should. It emits the warning once on the first should and then Not again. Hardly "spewing warnings".

It's true. I didn't look closely enough. Sorry @samphippen.

We have no plans to remove it. Given that we think the expect syntax is superior, and that we'd like to move in the direction of making rspec do zero monkey patching by default, the plan we had discussed was for should to still be enabled by default in RSpec 3 but to have it issue a "you should explicitly enable this" warning, and then in RSpec 4 it will be disabled by default. That'll help make our preference clear, and help nudge folks towards explicitly configuring the syntax if they want to continue using should.

Ah, OK. Basically I misread the code and misinterpreted the text here. If it's possible to squash the warning by enabling the syntax explicitly, I'm supportive.

Sorry for the knee jerk reaction.

Contributor

alindeman commented Jul 4, 2013

It warns precisely once for the entire suite as opposed to on every should. It emits the warning once on the first should and then Not again. Hardly "spewing warnings".

It's true. I didn't look closely enough. Sorry @samphippen.

We have no plans to remove it. Given that we think the expect syntax is superior, and that we'd like to move in the direction of making rspec do zero monkey patching by default, the plan we had discussed was for should to still be enabled by default in RSpec 3 but to have it issue a "you should explicitly enable this" warning, and then in RSpec 4 it will be disabled by default. That'll help make our preference clear, and help nudge folks towards explicitly configuring the syntax if they want to continue using should.

Ah, OK. Basically I misread the code and misinterpreted the text here. If it's possible to squash the warning by enabling the syntax explicitly, I'm supportive.

Sorry for the knee jerk reaction.

@JonRowe

This comment has been minimized.

Show comment
Hide comment
@JonRowe

JonRowe Jul 5, 2013

Member

馃憤 but also we need to hold off merging this until we get community feedback

Member

JonRowe commented Jul 5, 2013

馃憤 but also we need to hold off merging this until we get community feedback

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Jul 5, 2013

Member

but also we need to hold off merging this until we get community feedback

Also, I have some code review feedback but don't have the time to write it up at the moment...

Member

myronmarston commented Jul 5, 2013

but also we need to hold off merging this until we get community feedback

Also, I have some code review feedback but don't have the time to write it up at the moment...

Show outdated Hide outdated lib/rspec/mocks/configuration.rb Outdated
@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Aug 23, 2013

Member

Should I close this but leave the branch around, I feel like we should maybe start with a community community poll first.

Member

samphippen commented Aug 23, 2013

Should I close this but leave the branch around, I feel like we should maybe start with a community community poll first.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 23, 2013

Member

@samphippen - The feedback to my RSpec 3 blog post (where I mentioned we would emit a warning like this) was universally positive. I consider that to be a sufficient poll.

Member

myronmarston commented Aug 23, 2013

@samphippen - The feedback to my RSpec 3 blog post (where I mentioned we would emit a warning like this) was universally positive. I consider that to be a sufficient poll.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 23, 2013

Member

Thanks for reminding me to review this, though...I'll try to review it this weekend. If you want to rebase before then, feel free.

Member

myronmarston commented Aug 23, 2013

Thanks for reminding me to review this, though...I'll try to review it this weekend. If you want to rebase before then, feel free.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen
Member

samphippen commented Aug 23, 2013

@myronmarston go at it :D

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 23, 2013

Coverage Status

Coverage decreased (-0.09%) when pulling ff83d8a on samphippen:warn_about_the_should_syntax into 80d70b4 on rspec:master.

coveralls commented Aug 23, 2013

Coverage Status

Coverage decreased (-0.09%) when pulling ff83d8a on samphippen:warn_about_the_should_syntax into 80d70b4 on rspec:master.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Aug 26, 2013

Member

@myronmarston plz review when you can 鉂わ笍

Member

samphippen commented Aug 26, 2013

@myronmarston plz review when you can 鉂わ笍

Show outdated Hide outdated lib/rspec/mocks/syntax.rb Outdated
Show outdated Hide outdated lib/rspec/mocks/syntax.rb Outdated
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Aug 27, 2013

Member

@samphippen - code review feedback left.

Member

myronmarston commented Aug 27, 2013

@samphippen - code review feedback left.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 3, 2013

Coverage Status

Coverage increased (+0.38%) when pulling 23eb526 on samphippen:warn_about_the_should_syntax into 80d70b4 on rspec:master.

coveralls commented Sep 3, 2013

Coverage Status

Coverage increased (+0.38%) when pulling 23eb526 on samphippen:warn_about_the_should_syntax into 80d70b4 on rspec:master.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Sep 3, 2013

Member

@myronmarston I took a quick look through your comments, I think I got them all, I added a spec for the default then should case you asked about, you were right it wasn't doing the right thing, but it does now. More review welcomed. (Also got some help from @JonRowe on this one when we bumped into each other today in london)

Member

samphippen commented Sep 3, 2013

@myronmarston I took a quick look through your comments, I think I got them all, I added a spec for the default then should case you asked about, you were right it wasn't doing the right thing, but it does now. More review welcomed. (Also got some help from @JonRowe on this one when we bumped into each other today in london)

o2 = Object.new
o.stub(:faces)
o2.stub(:faces)
end

This comment has been minimized.

@myronmarston

myronmarston Sep 4, 2013

Member

Given that we've had some trouble with this, I'd like a spec that asserts on the correct call site being included in the deprecation warning.

@myronmarston

myronmarston Sep 4, 2013

Member

Given that we've had some trouble with this, I'd like a spec that asserts on the correct call site being included in the deprecation warning.

This comment has been minimized.

@samphippen

samphippen Sep 8, 2013

Member

you mean add the call site to the expected_arguments list, slightly confused because you've put this next to the negative case that doesn't filter by arguments. I assume you only want this to happen on the ones that do args matching?

@samphippen

samphippen Sep 8, 2013

Member

you mean add the call site to the expected_arguments list, slightly confused because you've put this next to the negative case that doesn't filter by arguments. I assume you only want this to happen on the ones that do args matching?

This comment has been minimized.

@samphippen

samphippen Sep 8, 2013

Member

I just had another look at this, it'd change the expect to kernel#warn instead of rspec#deprecate. Based on looking at this I'd assume we have tests elsewhere for call sites from CallerFilter? if not would you go for something like expect(CallerFilter).to receive(:first_non_rspec_line).and_return(the correct things here)?

@samphippen

samphippen Sep 8, 2013

Member

I just had another look at this, it'd change the expect to kernel#warn instead of rspec#deprecate. Based on looking at this I'd assume we have tests elsewhere for call sites from CallerFilter? if not would you go for something like expect(CallerFilter).to receive(:first_non_rspec_line).and_return(the correct things here)?

This comment has been minimized.

@myronmarston

myronmarston Sep 8, 2013

Member

you mean add the call site to the expected_arguments list, slightly confused because you've put this next to the negative case that doesn't filter by arguments. I assume you only want this to happen on the ones that do args matching?

No, I would like an additional spec added specifically for this. Having a spec for it calls it out as important (which it is).

Based on looking at this I'd assume we have tests elsewhere for call sites from CallerFilter

No, setting a mock expectation on CallerFilter doesn't really help us here; consider that a few weeks back, we had two different deprecation warnings that were printing the wrong call site, and both were using the CallerFilter (or the equivalent, before that got added):

  • in rspec-expectations, the be_true deprecation was printing a line from rspec/matchers.rb, because the caller-searching logic considered rspec/matchers/*.rb to be an rspec file but not rspec/matchers.rb. Luckily, CallerFilter as added to rspec-mocks already handled this correctly, so I ported it to rspec-core to solve the bug.
  • in rspec-mocks, the deprecation for the any_instance receiver yielding was printing a line from the user's code but the wrong line, because we wanted it to print the line where the implementation block was defined, but the warning was triggered when the mocked/stubbed method was called, and thus CallerFilter was being given the wrong backtrace to begin with.

To really have confidence that the right line is being printed, we need a spec that actually asserts on the file and line number (typically with __FILE__ and __LINE__ so that it won't fail as new code is added to the file or if the file is renamed). We have a helper method in rspec-core that assists with testing this. I recommend porting that to here (eventually, I suspect it'll go into the rspec-support gem we've discussed) and then you can test this like so:

it "includes the call site in the deprecation warning" do
  obj = Object.new
  expect_deprecation_with_call_site(__FILE__, __LINE__ + 1)
  obj.stub(:faces)
end

Does that make sense?

@myronmarston

myronmarston Sep 8, 2013

Member

you mean add the call site to the expected_arguments list, slightly confused because you've put this next to the negative case that doesn't filter by arguments. I assume you only want this to happen on the ones that do args matching?

No, I would like an additional spec added specifically for this. Having a spec for it calls it out as important (which it is).

Based on looking at this I'd assume we have tests elsewhere for call sites from CallerFilter

No, setting a mock expectation on CallerFilter doesn't really help us here; consider that a few weeks back, we had two different deprecation warnings that were printing the wrong call site, and both were using the CallerFilter (or the equivalent, before that got added):

  • in rspec-expectations, the be_true deprecation was printing a line from rspec/matchers.rb, because the caller-searching logic considered rspec/matchers/*.rb to be an rspec file but not rspec/matchers.rb. Luckily, CallerFilter as added to rspec-mocks already handled this correctly, so I ported it to rspec-core to solve the bug.
  • in rspec-mocks, the deprecation for the any_instance receiver yielding was printing a line from the user's code but the wrong line, because we wanted it to print the line where the implementation block was defined, but the warning was triggered when the mocked/stubbed method was called, and thus CallerFilter was being given the wrong backtrace to begin with.

To really have confidence that the right line is being printed, we need a spec that actually asserts on the file and line number (typically with __FILE__ and __LINE__ so that it won't fail as new code is added to the file or if the file is renamed). We have a helper method in rspec-core that assists with testing this. I recommend porting that to here (eventually, I suspect it'll go into the rspec-support gem we've discussed) and then you can test this like so:

it "includes the call site in the deprecation warning" do
  obj = Object.new
  expect_deprecation_with_call_site(__FILE__, __LINE__ + 1)
  obj.stub(:faces)
end

Does that make sense?

This comment has been minimized.

@samphippen

samphippen Sep 9, 2013

Member

I think so, I'll have an experiment and work it out.

@samphippen

samphippen Sep 9, 2013

Member

I think so, I'll have an experiment and work it out.

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 4, 2013

Member

Looking good. Very close to being ready to merge :). Left a couple final comments. (Sorry about noticing some of this on the last review...).

Also, Update should warnings for myron's changes. is kinda a weird commit message. I haven't made any changes here.

Member

myronmarston commented Sep 4, 2013

Looking good. Very close to being ready to merge :). Left a couple final comments. (Sorry about noticing some of this on the last review...).

Also, Update should warnings for myron's changes. is kinda a weird commit message. I haven't made any changes here.

@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Sep 8, 2013

Member

Also, Update should warnings for myron's changes. is kinda a weird commit message. I haven't made any changes here.

you're totally right. tired git driving leads to bad commit messages. I'll squash everything together once I'm done here.

Member

samphippen commented Sep 8, 2013

Also, Update should warnings for myron's changes. is kinda a weird commit message. I haven't made any changes here.

you're totally right. tired git driving leads to bad commit messages. I'll squash everything together once I'm done here.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 8, 2013

Coverage Status

Coverage decreased (-0.16%) when pulling 75f9f60 on samphippen:warn_about_the_should_syntax into 3ac6f4e on rspec:master.

coveralls commented Sep 8, 2013

Coverage Status

Coverage decreased (-0.16%) when pulling 75f9f60 on samphippen:warn_about_the_should_syntax into 3ac6f4e on rspec:master.

Add a warning when the should syntax is used.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
@samphippen

This comment has been minimized.

Show comment
Hide comment
@samphippen

samphippen Sep 9, 2013

Member

@myronmarston please rereview when you can. I took most of your notes. Good catches

Member

samphippen commented Sep 9, 2013

@myronmarston please rereview when you can. I took most of your notes. Good catches

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 9, 2013

Coverage Status

Coverage remained the same when pulling cf4d4c9 on samphippen:warn_about_the_should_syntax into 3ac6f4e on rspec:master.

coveralls commented Sep 9, 2013

Coverage Status

Coverage remained the same when pulling cf4d4c9 on samphippen:warn_about_the_should_syntax into 3ac6f4e on rspec:master.

myronmarston added a commit that referenced this pull request Sep 10, 2013

Merge pull request #339 from samphippen/warn_about_the_should_syntax
Add a warning when the should syntax is used.

@myronmarston myronmarston merged commit 44b464e into rspec:master Sep 10, 2013

1 check passed

default The Travis CI build passed
Details
@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 10, 2013

Member

Thanks, @samphippen! I just merged it. I also added some additional doc comments in f96edc3.

Can you take care of adding this kind of thing to rspec-expectations as well?

Member

myronmarston commented Sep 10, 2013

Thanks, @samphippen! I just merged it. I also added some additional doc comments in f96edc3.

Can you take care of adding this kind of thing to rspec-expectations as well?

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 10, 2013

Member

Also, BTW: while it's nice to not a ton of tiny commits to merge, in the future please don't continually amend your commits as we do the back-and-forth of a code review. It makes it harder to see what changed since I last looked at the PR when you've amended commits or squashed them together.

Member

myronmarston commented Sep 10, 2013

Also, BTW: while it's nice to not a ton of tiny commits to merge, in the future please don't continually amend your commits as we do the back-and-forth of a code review. It makes it harder to see what changed since I last looked at the PR when you've amended commits or squashed them together.

myronmarston added a commit that referenced this pull request Sep 10, 2013

@myronmarston

This comment has been minimized.

Show comment
Hide comment
@myronmarston

myronmarston Sep 10, 2013

Member

One other thing...we forgot a changelog entry. I added one in b34488c

Member

myronmarston commented Sep 10, 2013

One other thing...we forgot a changelog entry. I added one in b34488c

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