Include the spec location in warnings if we can't generate a call site. #1253

fables-tales · 2014-01-19T12:30:45Z

Whilst investigating rspec/rspec-mocks#496 I came across the problem where it wasn't clear where the warning was being issued from, and that the caller filter would fail because we're outside of user code at that point.

This patch makes it so that RSpec core's warn_with will include the location of the spec that's running if the call site is explicitly nil. This means that we can add warnings outside a user's testing block and still hopefully point them in the correct direction.

myronmarston · 2014-01-19T16:14:30Z

lib/rspec/core/warnings.rb

+  def self.warn_with(message, options = {})
+    if options.fetch(:call_site, :not_present).nil?
+      message << " Warning generated from spec at `#{RSpec.current_example.source_location.join(":")}`."
+    end


There are cases where there's not a current example (and thus, it will return nil) such as when we issue warnings at load time. This needs to be tolerant of that.

~~Do we define source_location on current_example? Cause otherwise this won't work on 1.8.7 right?~~

Ah I see it...

JonRowe · 2014-01-27T21:02:10Z

Heya @samphippen how you going on this?

fables-tales · 2014-02-12T15:14:56Z

Depends on http://github.com/rspec/rspec-support/pull/40 and is currently pointed at that branch. Will move to master when 40 is merged.

This is going to be depended on by a change in RSpec mocks: rspec/rspec-mocks#527

myronmarston · 2014-02-12T16:06:08Z

lib/rspec/core.rb

@@ -45,6 +47,8 @@
 module RSpec
  autoload :SharedContext, 'rspec/core/shared_context'

+  extend RSpec::Core::Warnings


In your rspec-support PR it has already extended this module on to RSpec -- so why extend it onto RSpec a second time here?

This is RSpec::Core::Warnings, which is not extended from RSpec support. As such it's needed here. The tests fail without this line.

Sorry, I misread this as extend RSpec::Support::Warnings. Ignore me.

myronmarston · 2014-02-12T16:09:16Z

Left a few comments but looks good.

JonRowe · 2014-02-12T21:15:27Z

It's still not clear to me why you've moved the methods into a module.

fables-tales · 2014-02-12T21:17:23Z

@JonRowe the module RSpec::Support::Warnings is so that we can call super from RSpec::Core::Warnings to invoke RSpec Support's warn_with. The module RSpec::Core::Warnings is mostly for symmetry with RSpec::Support::Warnings.

JonRowe · 2014-02-12T21:19:12Z

TIL that extend works with super

JonRowe · 2014-02-12T21:20:45Z

lib/rspec/core/warnings.rb

-    RSpec.configuration.reporter.deprecation :message => message
-  end
+      def warn_with(message, options = {})
+        if options.fetch(:call_site, :not_present).nil?


I think unless options.fetch(:call_site, :not_present) would read slightly better.

It could be more obvious we're looking for :call_site to have been explicitly set to nil.

I'm not sure about removing the .nil? check but how about making the symbol more communicative?

Something like options.fetch(:call_site, :truthy_if_not_present) ?

Except we actually don't care wether it's specifically nil, I think false should be equally valid for suppressing the callsite.

@myronmarston got opinions on this one? I think I like :call_site => false also working. Do you think the default symbol on the fetch needs a rename?

False should work. Don't care much about what you call the symbol.

Sent from my iPhone

On Feb 13, 2014, at 5:23 AM, Sam Phippen [email protected] wrote:

In lib/rspec/core/warnings.rb:

@Private

Used internally to print deprecation warnings

def self.warn_deprecation(message)

RSpec.configuration.reporter.deprecation :message => message

end

def warn_with(message, options = {})

if options.fetch(:call_site, :not_present).nil?
@myronmarston got opinions on this one? I think I like :call_site => false also working. Do you think the default symbol on the fetch needs a rename?

—
Reply to this email directly or view it on GitHub.

Hah, yes it should. Told you it was confusing!

OK. So in the case for rspec/rspec-mocks#527 we need to be able to create a warning that points a user at their test, even though no individual line of user code can be traced to the cause of the warning. I suggest we add an option that allows for this warning case, which is separate to nil call site. Thoughts?

One thing to consider...are the only places where we try to silence the call site places that happen outside of an example (such as CLI options)? In such case there won't be a current example so maybe the behavior here is OK because it won't affect them. I'm not sure though.

I think it's safer to add another option that asks for this behaviour explicitly than relying on the behaviour being implicitly triggered off :call_site => nil. That way we can flag warnings that have a current example (e.g. in mock teardown) as explicitly pointing at a user's spec without having to worry about call site falsyness.

That sounds good to me.

myronmarston · 2014-02-13T05:49:18Z

TIL that extend works with super

It's not so much that extend specifically works with super...it's a consequence of what extend and super do:

super means: delegate the handling of this message up the chain to the next class or module in the ancestor chain.
extend inserts the module in the ancestor chain between an object's singleton class and it's actual class.
Therefore, when you use super from an object's singleton method, it will go to an extended module before it hits the class of the object.

JonRowe · 2014-02-13T08:20:34Z

extend inserts the module in the ancestor chain between an object's singleton class and it's actual class.

As extended modules don't tend to appear in the ancestor chain, I always assumed doing a second extend would overwrite the original method definition, such that super between two extended modules wouldn't work.

bestie · 2014-02-13T16:22:58Z

Extended modules do appear in the ancestor chain of the singleton class of the object.

I put this gist together for LOLs https://gist.github.com/bestie/8978251

myronmarston · 2014-02-14T06:50:08Z

lib/rspec/core/warnings.rb

-  end
+      def warn_with(message, options = {})
+        if options.fetch(:call_site, :not_present).nil?
+          if message.chars.to_a.last != "."


Can this be unless message.end_with?('.')?

myronmarston · 2014-02-14T23:02:33Z

Code Climate has analyzed this pull request.

fables-tales · 2014-02-14T23:03:16Z

@myronmarston I think I'm happy with this now. Any more thoughts? If we're good on this I'll merge the support pull request, then point this back at support master, then merge this.

myronmarston · 2014-02-15T05:23:47Z

lib/rspec/core/warnings.rb

-    RSpec.configuration.reporter.deprecation :message => message
-  end
+      def warn_with(message, options = {})
+        if options.fetch(:spec_location, false)


There's no need for fetch here -- if options[:spec_location] will work just fine: if the key is missing, it'll return nil, which is falsey.

I'm generally a fan of fetch but it doesn't accomplish anything over #[] here and I think it creates confusion because I would look at this and wonder why it's being used since it doesn't do anything.

myronmarston · 2014-02-15T05:46:14Z

Sorry, I found a few things that I hadn't noticed before. (I need to get better at finding this stuff on the first review so there's less back-and-forth).

Side note: I think this is outside the scope of this PR, but we might want to add support for this to RSpec.deprecate, etc at some future point.

fables-tales · 2014-02-15T11:34:04Z

Sorry, I found a few things that I hadn't noticed before. (I need to get better at finding this stuff on the first review so there's less back-and-forth).

Hey @myronmarston don't worry. You do such a stellar job reviewing all these contributions people make, missing things from time to time is natural. I'll roll in the changes you've suggested later today (my time) and we can go from there.

fables-tales · 2014-02-15T13:07:10Z

Hey @myronmarston I did the rebase you wanted. I think this now covers all your feedback. Let me know what you think.

myronmarston · 2014-02-16T05:24:53Z

lib/rspec/core/warnings.rb

+        end
+
+        super(message, options)
+      end


This implementation can still mutate the passed message arg under the following circustances:

:use_spec_location_as_call_site => true is passed.

message already ends with a period.

I think you can just change message << " Warning..." to message += " Warning ..." to deal with this.

myronmarston · 2014-02-16T05:25:49Z

BTW, please hold off merging this until we get beta2 out the door. I don't want the core/mocks/support set of PRs to hold up the release by being partially merged.

fables-tales · 2014-02-16T14:56:52Z

BTW, please hold off merging this until we get beta2 out the door. I don't want the core/mocks/support set of PRs to hold up the release by being partially merged.

No problem. Merging these all down was post-beta in my mind anyway!

fables-tales · 2014-02-18T14:20:00Z

@myronmarston if this is good now, I'll merge the -support pull request and then point this back at support master, then merge this.

myronmarston · 2014-02-18T15:30:23Z

Go for it. I did notice this is 12 commits which is kinda a lot for the size of the change here, so squashing before merging could be beneficial, but if you feel like the commits are self-contained and there's value in keeping the commits as is, it's fine.

fables-tales · 2014-02-20T11:08:36Z

I squashed this to a single commit, will wait for travis then do the necessary shakes to get support and core reconciled against each other.

fables-tales · 2014-02-20T14:06:57Z

Hold on to your hats. I'm going to start doing the merging now.

Include the spec location in warnings if we can't generate a call site.

fables-tales mentioned this pull request Jan 19, 2014

Frozen support rspec/rspec-mocks#527

Merged

myronmarston reviewed Jan 19, 2014
View reviewed changes

bestie mentioned this pull request Feb 12, 2014

Extract warning methods into module rspec/rspec-support#40

Merged

myronmarston reviewed Feb 12, 2014
View reviewed changes

JonRowe reviewed Feb 12, 2014
View reviewed changes

myronmarston reviewed Feb 14, 2014
View reviewed changes

myronmarston reviewed Feb 15, 2014
View reviewed changes

myronmarston reviewed Feb 16, 2014
View reviewed changes

Add an option to use spec location if there is no call site.

360dbae

fables-tales pushed a commit that referenced this pull request Feb 20, 2014

Merge pull request #1253 from rspec/spec-location-if-no-call-site

05759fd

Include the spec location in warnings if we can't generate a call site.

fables-tales merged commit 05759fd into master Feb 20, 2014

fables-tales deleted the spec-location-if-no-call-site branch February 20, 2014 14:30

Uh oh!

Include the spec location in warnings if we can't generate a call site. #1253

Include the spec location in warnings if we can't generate a call site. #1253

Uh oh!

Conversation

fables-tales commented Jan 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonRowe commented Jan 27, 2014

Uh oh!

fables-tales commented Feb 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Feb 12, 2014

Uh oh!

JonRowe commented Feb 12, 2014

Uh oh!

fables-tales commented Feb 12, 2014

Uh oh!

JonRowe commented Feb 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Used internally to print deprecation warnings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Feb 13, 2014

Uh oh!

JonRowe commented Feb 13, 2014

Uh oh!

bestie commented Feb 13, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Feb 14, 2014

Uh oh!

fables-tales commented Feb 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Feb 15, 2014

Uh oh!

fables-tales commented Feb 15, 2014

Uh oh!

fables-tales commented Feb 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myronmarston commented Feb 16, 2014

Uh oh!

fables-tales commented Feb 16, 2014

Uh oh!

fables-tales commented Feb 18, 2014

Uh oh!