Require the "message" param passed to a should[_not] call to be a string #173

Closed
wants to merge 14 commits into
from

Projects

None yet

6 participants

@samphippen
Member

Hi, this is related to #142.

I've added errors that will get thrown when message parameters aren't strings. I couldn't find any specs dedicated to the should method so I didn't add any specs. If someone can suggest a place where they should go I'll totally add them.

@samphippen samphippen Require the "message" param passed to a should to be a string
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
7ecdc64
@dchelimsky dchelimsky and 1 other commented on an outdated diff Sep 24, 2012
lib/rspec/expectations/syntax.rb
@@ -43,6 +43,8 @@ def default_should_host
@default_should_host ||= ::Object.ancestors.last
end
+ MESSAGE_ERR = "The value passed as the message for the expectation was not a string"
@dchelimsky
dchelimsky Sep 24, 2012 Member

Other parts of rspec share error messages by redirecting to another method. Something like raise message_must_be_string unless String === message.

@samphippen
samphippen Sep 24, 2012 Member

cool, I'll take a look at it.

@dchelimsky
Member

Needs specs. Otherwise I like the idea.

@samphippen
Member

@dchelimsky is there a specs file that exists that I should group these things with?

samphippen added some commits Sep 24, 2012
@samphippen samphippen Make the error message a method rather than a constant
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
b93d26b
@samphippen samphippen Move the message_must_be_string method into the right place
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
2e5f02b
@samphippen samphippen Add some specs for the should[_not] exception throwing
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
fe0e8e8
@samphippen
Member

now with specs.

@alindeman
Contributor

@dchelimsky, @samphippen , I think it might be worse to add more methods to the syntax_host (i.e., message_must_be_string). Maybe even worse than a bit of string duplication. Thoughts?

@myronmarston
Member

@alindeman -- good catch...that'd be a pretty serious problem to add message_must_be_string to every object in the system. It's easy to imagine user's defining that on some of their objects.

Also, what you have here only works for the should syntax, not the expect syntax.

What do you think about moving it to here instead? You could define a module that gets extended onto both handler classes with the helper method. Everything goes through the handler classes (including both syntaxes and rspec-core one-liners), so it'll handle more cases for free :).

@samphippen
Member

@alindeman this was originally a string constant, I'm not sure if that's better or worse. I moved it to a method on @dchelimsky's recommendation. A duplicated string is also fine.

@myronmarston I could do, I like that idea because we get more cases handled for free. @dchelimsky @alindeman what do you think?

@soulcutter
Member

Is enforcing types like this rubyish? I don't know what's wrong with passing any object that has a to_s (so... any object).

@dchelimsky
Member

@soulcutter see #142 for background

@soulcutter
Member

@dchelimsky I did see that. I understand the motivation, however I still feel this is something of a code smell. I would say 'maybe this can be a warning?' however that is probably a bigger feature.

@samphippen
Member

@soulcutter I think this is preferable, it means that the user will get warnings when they make what is probably a mistake.

@alindeman
Contributor

@myronmarston's suggestion sounds good to me. Anyone fancy implementing it?

@dchelimsky
Member

@soulcutter I like the idea of a warning, and I like @myronmarston's location suggestion.

@samphippen
Member

I'll do this.

samphippen added some commits Sep 27, 2012
@samphippen samphippen Move message must be string code into expectation handlers.
Instead of putting this in the should syntax, we can put it in the
positive and negative expectation handlers. This handles more cases in
terms of which expecations are covered by the message needing to be a
string.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
baeaeae
@samphippen samphippen Remove extraneous whitespace.
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
d77ccc4
@myronmarston myronmarston commented on an outdated diff Oct 1, 2012
spec/rspec/expectations/syntax_spec.rb
@@ -0,0 +1,29 @@
+require 'spec_helper'
+
+module RSpec
+ module Expectations
+ module Syntax
+ describe "the should and should_not expectations" do
+ describe "#should" do
+ it "raises an error when message isn't a string" do
+ lambda {3.should eq(3), :not_a_string}.should raise_error
@myronmarston
myronmarston Oct 1, 2012 Member

We generally use expect { }.to raise_error rather than lambda { }.should raise_error since the expect syntax reads nicer and is the syntax we recommend now for all expectations.

Also, it's best to use raise_error with an argument; it's prone to false positives on its own, since it passes for any error, even if you fat fingered something and got a different error than you expect. I tend to use raise_error /some sub-expression I expect/.

@myronmarston myronmarston commented on an outdated diff Oct 1, 2012
lib/rspec/expectations/handler.rb
def self.handle_matcher(actual, matcher, message=nil, &block)
+ raise message_must_be_string unless message.is_a? String or message == nil
@myronmarston
myronmarston Oct 1, 2012 Member

You've defined message_must_be_a_string on RSpec::Expectations but here, self is RSpec::Expectations::PositiveExpectationHandler -- so I would expect this to raise a NoMethodError (note that your specs could still pass below due to the fact that it just specifies that an error will be raised, but says nothing about which error).

I would recommend defining this method in a module and extending it onto PositiveExpectationHandler and NegativeExpectationHandler.

@myronmarston myronmarston commented on an outdated diff Oct 1, 2012
lib/rspec/expectations/handler.rb
@@ -1,7 +1,13 @@
module RSpec
module Expectations
+ def message_must_be_string
+ "The value passed as the message for the expectation was not a string"
+ end
@myronmarston
myronmarston Oct 1, 2012 Member

It'd be nice for this to include the message object in the failure so the developer can get more detail.

Also, there's duplicated logic in message.is_a?(String) or message == nil.

So, I think it might be best to change this to a method like:

def verify_message_is_a_string(message)
  return if message.is_a? String
  raise ArgumentError, "The passed expectation message was not a string, " +
                       "but only strings are supported. Message: #{message.inspect}"
end

Then call it with:

verify_message_is_a_string(message) if message
samphippen added some commits Oct 2, 2012
@samphippen samphippen Fix specs for the new message string requirement
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
f9106dd
@samphippen samphippen Fix message string handling code to pass new specs
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
06643c2
@samphippen samphippen Replace lamba and should with expect and to in message string specs
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
590acf8
@alindeman
Contributor

@soulcutter I like the idea of a warning, and I like @myronmarston's location suggestion.

@dchelimsky, do you think a warning is better than an error here? @samphippen's latest code is implemented as an error (through it uses throw .. did you mean raise?)

@samphippen
Member

@alindeman yes. I did mean raise. Let me patch that now.

@samphippen samphippen Replace the "throw" with a "raise" in the message should be string code
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
a448040
@samphippen
Member

Regarding this being a warning, my thought is that execution will continue and it might be missed in a sea of red. If we use an exception the user will be forcibly interrupted.

@alindeman
Contributor

@soulcutter, would checking for an object responding to #to_str be a little more Ruby-like?

@soulcutter
Member

The more I've thought about it the more ok I am with type checking here. Chances are non-strings are mistakes, and if not the user can call to_s

Apologies for hollow voice of dissent

@alindeman
Contributor

Apologies for hollow voice of dissent

Dissent is good if it makes everyone express their points better. I am thinking that a check for respond_to?(:to_str) is most idiomatic, even better than String ===. Thoughts?

@soulcutter
Member

@alindeman It's to_s and no I don't think that is better because every Object defines a to_s

@myronmarston
Member

@soulcutter -- there's also to_str, which is implemented on objects that have a direct 1-to-1 string representation, such as File, Pathname, and URI.

@soulcutter
Member

Huh, I did not realize that. I don't believe String responds to that, does it? (From my phone or I would check). I guess using to_str could make sense in any case

@myronmarston
Member

Huh, I did not realize that. I don't believe String responds to that, does it? (From my phone or I would check). I guess using to_str could make sense in any case

Yep:

1.9.3p194 :002 > "a".to_str
 => "a" 

#to_str is essentially the "you can treat me like a string" protocol in ruby.

#to_s is meant to produce a human readable string on every object, but you can't treat every object like a string.

@samphippen
Member

I think this should accept objects that implement to_str as well. Any significant disagreement? Otherwise I'll patch it in.

@samphippen samphippen Make message in expectations accept objects that implement to_str
this commit also contains some specs for that behaviour

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
9f9672c
@samphippen
Member

@alindeman and I were talking about this in irc, in the entire standard library only two objects implement to_str

>> ObjectSpace.each_object(Class).select { |k| k.method_defined?(:to_str) } 
=> [NameError::message, String]
@myronmarston
Member

That's surprising -- I thought that URI, File and Pathname did as well? Seems like all of those can be treated as strings since that's essentially what they are. I guess I was off...

@alindeman
Contributor

That's actually not with any of the non-core stdlib loaded.

@soulcutter
Member

Pathname definitely does.

@samphippen
Member

It appears pathname doesn't :O

 >> p = Pathname.new("/")
 => #<Pathname:/>
 >> p.to_str
 NoMethodError: undefined method `to_str' for #<Pathname:/>
    from (irb):20
    from /Users/sam/.rvm/rubies/ruby-1.9.3-p194/bin/irb:16:in `<main>'
 >> p.to_s
 => "/"

I also checked file:

 >> file = File.new("guardfile")
 => #<File:guardfile>
 >> file.to_str
 NoMethodError: undefined method `to_str' for #<File:guardfile>
      from (irb):2
      from /Users/sam/.rvm/rubies/ruby-1.9.3-p194/bin/irb:16:in `<main>'
 >> 
@samphippen
Member

URI doesn't either:

>> uri = URI("http://google.com/")
=> #<URI::HTTP:0x007f94ee0c0708 URL:http://google.com/>
>> uri.to_s
=> "http://google.com/"
>> uri.to_str
NoMethodError: undefined method `to_str' for #<URI::HTTP:0x007f94ee0c0708 URL:http://google.com/>
    from (irb):9
    from /Users/sam/.rvm/rubies/ruby-1.9.3-p194/bin/irb:16:in `<main>'

@soulcutter
Member

Took this to IRC, but for posterity we determined there are differences in responding to to_str between ruby 1.8 and 1.9

@myronmarston
Member

Took this to IRC, but for posterity we determined there are differences in responding to to_str between ruby 1.8 and 1.9

Interesting...care to share what you discovered?

@soulcutter
Member
1.8.7 :001 > ObjectSpace.each_object(Class).select { |k| k.method_defined?(:to_str) } 
 => [IRB::UndefinedPromptMode, IRB::CantChangeBinding, IRB::CantShiftToMultiIrbMode, IRB::NoSuchJob, IRB::IrbSwitchedToCurrentThread, IRB::IrbAlreadyDead, IRB::IllegalParameter, IRB::CantReturnToNormalMode, IRB::NotImplementedError, IRB::UnrecognizedSwitch, Exception2MessageMapper::ErrNotRegisteredException, IRB::Abort, StopIteration, SystemStackError, LocalJumpError, EOFError, IOError, RegexpError, Errno::EDQUOT, Errno::ESTALE, Errno::EINPROGRESS, Errno::EALREADY, Errno::EHOSTUNREACH, Errno::EHOSTDOWN, Errno::ECONNREFUSED, Errno::ETIMEDOUT, Errno::ETOOMANYREFS, Errno::ESHUTDOWN, Errno::ENOTCONN, Errno::EISCONN, Errno::ENOBUFS, Errno::ECONNRESET, Errno::ECONNABORTED, Errno::ENETRESET, Errno::ENETUNREACH, Errno::ENETDOWN, Errno::EADDRNOTAVAIL, Errno::EADDRINUSE, Errno::EAFNOSUPPORT, Errno::EPFNOSUPPORT, Errno::EOPNOTSUPP, Errno::ESOCKTNOSUPPORT, Errno::EPROTONOSUPPORT, Errno::ENOPROTOOPT, Errno::EPROTOTYPE, Errno::EMSGSIZE, Errno::EDESTADDRREQ, Errno::ENOTSOCK, Errno::EUSERS, Errno::EILSEQ, Errno::EOVERFLOW, Errno::EBADMSG, Errno::EMULTIHOP, Errno::EPROTO, Errno::ENOLINK, Errno::EREMOTE, Errno::ENOSR, Errno::ETIME, Errno::ENODATA, Errno::ENOSTR, Errno::EIDRM, Errno::ENOMSG, Errno::ELOOP, Errno::ENOTEMPTY, Errno::ENOSYS, Errno::ENOLCK, Errno::ENAMETOOLONG, Errno::EDEADLK, Errno::ERANGE, Errno::EDOM, Errno::EPIPE, Errno::EMLINK, Errno::EROFS, Errno::ESPIPE, Errno::ENOSPC, Errno::EFBIG, Errno::ETXTBSY, Errno::ENOTTY, Errno::EMFILE, Errno::ENFILE, Errno::EINVAL, Errno::EISDIR, Errno::ENOTDIR, Errno::ENODEV, Errno::EXDEV, Errno::EEXIST, Errno::EBUSY, Errno::ENOTBLK, Errno::EFAULT, Errno::EACCES, Errno::ENOMEM, Errno::EAGAIN, Errno::ECHILD, Errno::EBADF, Errno::ENOEXEC, Errno::E2BIG, Errno::ENXIO, Errno::EIO, Errno::EINTR, Errno::ESRCH, Errno::ENOENT, Errno::EPERM, FloatDomainError, ZeroDivisionError, ThreadError, SystemCallError, NoMemoryError, SecurityError, RuntimeError, NotImplementedError, LoadError, SyntaxError, ScriptError, NoMethodError, NameError::message, NameError, RangeError, IndexError, ArgumentError, TypeError, StandardError, Interrupt, SignalException, fatal, SystemExit, Exception, String, RubyLex::TerminateLineInput, RubyLex::SyntaxError, RubyLex::TkReading2TokenDuplicateError, RubyLex::TkSymbol2TokenNoKey, RubyLex::TkReading2TokenNoKey, RubyLex::AlreadyDefinedToken, IRB::SLex::ErrNodeAlreadyExists, IRB::SLex::ErrNodeNothing, IRB::Notifier::ErrUnrecognizedLevel, IRB::Notifier::ErrUndefinedNotifier] 
1.8.7 :002 > exit
➜  temp  rvm use 1.9.3
Using /Users/soulcutter/.rvm/gems/ruby-1.9.3-p194
Running /Users/soulcutter/.rvm/hooks/after_use
➜  temp  irb
1.9.3p194 :001 > ObjectSpace.each_object(Class).select { |k| k.method_defined?(:to_str) } 
 => [NameError::message, String] 
@alindeman
Contributor

Maybe it's just better to stick with String ===? I guess my only reasoning for #to_str was to make it seemingly more idiomatic, but if there are hidden complications, let's just nix it?

@dchelimsky
Member

@alindeman agree - String is simpler, and clearer.

@samphippen
Member

Patching now.

@samphippen samphippen Update the message parameter in the handlers to use `String ===`
this commit also removes the specs expecting objects that use `to_str`
to pass.

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
f91ea52
@samphippen
Member

there was some discussion of making this a warning, do we want to do that? I think we've concluded that String === was the best solution to string matching.

@dchelimsky

I think these should be reversed (check for non-nil first) because most of the time ppl don't use the message.

@dchelimsky
Member

I've already said a warning is fine w/ me. @myronmarston, @alindeman, @patmaddox?

@patmaddox
Contributor

I don't have a preference. I've never run into this issue and don't think I ever will. I've never heard of anyone else having it, and the examples cited in #142 don't convince me - there's a whole slew of problems that people can run into when they inadvertently write Ruby code that does something different than they anticipate! So...I abstain.

@dchelimsky
Member

@patmaddox didn't figure you for a pro-abstinence guy, but OK.

@myronmarston
Member

I'm fine with a warning.

@samphippen samphippen Flip the order on the conditional for message string/nil checking
this is done because message will be nil most of the time so we can
shortcut

Signed-off-by: Sam Phippen <samphippen@googlemail.com>
6773dfd
@samphippen
Member

OK, I've got the code raising a warning ala (borrowed from deprecation.rb):

send(:warn,message_must_be_string) unless message == nil or String === message

and a spec like this:

RSpec.should_receive(:warn)

and that fails. I've also tried RSpec.warn, but apparently that's a private method. What's the correct solution. I note that the warning is happening, because I see it in the console output of the specs being run.

@myronmarston
Member
send(:warn,message_must_be_string) unless message == nil or String === message

Any reason you used send(:warn, msg) rather than warn(msg)?

RSpec.should_receive(:warn)

This mock expectation is saying "The RSpec module object should receive the warn method". In your code, when you have send(:warn), it's not sending the message to the RSpec module...it's sending it to the default receiver--self at the point of call. I'm guessing self at that point is RSpec::Expectations::PositiveExpectationHandler, or RSpec::Expectations::NegativeExpectationHandler, right? So the warn message is being sent to a different object then the one your mock expectation specified.

In cases like these, I'll often do:

::Kernel.should_receive(:warn).with(/some message fragment/)

And then for the lib code, I'll do:

::Kernel.warn("some warning message")
@dchelimsky
Member

Alternatively you can still use just warn, and expect that on the object itself:

# spec:
object.should_receive(:warn).with('msg')

# in the object
warn('msg')
@myronmarston
Member

Yep, I would normally do that...but in this case, you've got two objects (RSpec::Expectations::PositiveExpectationHandler and RSpec::Expectations::NegativeExpectationHandler) that will be involved, and I thought it might be easier to write the spec if the warn message is sent to the same object (e.g. Kernel). Doesn't really matter though--the important thing is to set your mock expectation on the same object(s) the warn message is sent to.

@dchelimsky
Member

@myronmarston makes sense. One question: do you think this could reasonable be considered a deprecation warning? If so, let's just go through RSpec.warn_deprecation.

@myronmarston
Member

I don't think deprecation makes sense. To me, a deprecation implies:

  • that it used to be supported
  • that the deprecation warning will be removed at some future point.

Neither is true here.

Sent from my iPhone

On Oct 12, 2012, at 8:08 AM, David Chelimsky notifications@github.com wrote:

@myronmarston makes sense. One question: do you think this could reasonable be considered a deprecation warning? If so, let's just go through RSpec.warn_deprecation.


Reply to this email directly or view it on GitHub.

@samphippen
Member

I also think this shouldn't be a deprecation for the reasons @myronmarston mentioned. Getting back to the spec, the relevant lines of code now look like this:

describe "#should" do
  it "raises an error when the message object isn't a String" do
    3.should eq(3), :not_a_string
    ::Kernel.should_receive(:warn)
  end
end

and the code in lib/:

::Kernel.warn message_must_be_string unless message == nil or String === message

but for some reason the spec fails. Any thoughts?

(I've gisted the files here and here)

@dchelimsky
Member

should_receive needs to happen before the action that sends the expected message. This should work:

    ::Kernel.should_receive(:warn)
    3.should eq(3), :not_a_string
@samphippen
Member

oh of course. Brainfail.

@samphippen samphippen Make the message string check a warning instead of a raise
Signed-off-by: Sam Phippen <samphippen@googlemail.com>
385966a
@samphippen
Member

Alright. Sorted. Cheers @dchelimsky

@myronmarston
Member

Merged in 518a0f7

@samphippen
Member

Awesome. Cheers @myronmarston

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