Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

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

Merged
merged 1 commit into from
Feb 20, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/rspec/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

require 'rspec/support/caller_filter'
require 'rspec/core/warnings'
require 'rspec/support/warnings'

require_rspec['core/flat_map']
require_rspec['core/filter_manager']
Expand Down Expand Up @@ -45,6 +44,8 @@
module RSpec
autoload :SharedContext, 'rspec/core/shared_context'

extend RSpec::Core::Warnings
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# @private
def self.wants_to_quit
# Used internally to determine what to do when a SIGINT is received
Expand Down
50 changes: 33 additions & 17 deletions lib/rspec/core/warnings.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,38 @@
require "rspec/support/warnings"

module RSpec
module Core
module Warnings
# @private
#
# Used internally to print deprecation warnings
def deprecate(deprecated, data = {})
RSpec.configuration.reporter.deprecation(
{
:deprecated => deprecated,
:call_site => CallerFilter.first_non_rspec_line
}.merge(data)
)
end

# @private
#
# Used internally to print deprecation warnings
def self.deprecate(deprecated, data = {})
RSpec.configuration.reporter.deprecation(
{
:deprecated => deprecated,
:call_site => CallerFilter.first_non_rspec_line
}.merge(data)
)
end
# @private
#
# Used internally to print deprecation warnings
def warn_deprecation(message)
RSpec.configuration.reporter.deprecation :message => message
end

# @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[:use_spec_location_as_call_site]
message += "." unless message.end_with?(".")

if RSpec.current_example
message += " Warning generated from spec at `#{RSpec.current_example.location}`."
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These message additions depend on the message ending with a period to read well. Are we sure that's the case everywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth making this append a period to the message if it lacks one before adding the extra bit so it's not a concern.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dealt with, but GitHub hasn't squashed this comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's generally a bad idea to mutate a passed arg but your implementation here will mutate message if there is already a period at the end of the message.

Also, I'm not sure the "RSpec could not determine which call generated this warning" bit has any value -- to a user, being told "I can't tell you any more about this" isn't useful or helpful. There's nothing actionable for the user based on whether or not rspec can add a call site. It would be a bit like if grep added a "can't find any more matches" message after everytime you ran it.

If there's no example, I don't think it should append anything.

end

super(message, options)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ah I see it...

end
end
38 changes: 38 additions & 0 deletions spec/rspec/core/warnings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,42 @@
end
end

describe "#warn_with" do
context "when :use_spec_location_as_call_site => true is passed" do
let(:options) {
{
:use_spec_location_as_call_site => true,
:call_site => nil,
}
}

it "adds the source location of spec" do
line = __LINE__ - 1
file_path = RSpec::Core::Metadata.relative_path(__FILE__)
expect(Kernel).to receive(:warn).with(/The warning. Warning generated from spec at `#{file_path}:#{line}`./)

RSpec.warn_with("The warning.", options)
end

it "appends a period to the supplied message if one is not present" do
line = __LINE__ - 1
file_path = RSpec::Core::Metadata.relative_path(__FILE__)
expect(Kernel).to receive(:warn).with(/The warning. Warning generated from spec at `#{file_path}:#{line}`./)

RSpec.warn_with("The warning", options)
end

context "when there is no current example" do
before do
allow(RSpec).to receive(:current_example).and_return(nil)
end

it "adds no message about the spec location" do
expect(Kernel).to receive(:warn).with(/The warning\.$/)

RSpec.warn_with("The warning.", options)
end
end
end
end
end