Merge pull request #768 from rspec/warn_about_bare_raise_error

Warn about bare `raise_error`

Changelog.md

@@ -19,6 +19,10 @@ Enhancements:
 * Use custom Time/DateTime/BigDecimal formatting for all matchers
   so they are consistently represented in failure messages.
   (Gavin Miller, #740)
+* Add configuration to turn off warnings about matcher combinations that
+  may cause false positives. (Jon Rowe, #768)
+* Warn when using a bare `raise_error` matcher that you may be subject to
+  false positives. (Jon Rowe, #768)
 
 Bug Fixes:
 

lib/rspec/expectations/configuration.rb

@@ -18,6 +18,10 @@ module Expectations
     #
     #   RSpec::Expectations.configuration
     class Configuration
+      def initialize
+        @warn_about_false_positives = true
+      end
+
       # Configures the supported syntax.
       # @param [Array<Symbol>, Symbol] values the syntaxes to enable
       # @example
@@ -133,6 +137,19 @@ def self.format_backtrace(backtrace)
           backtrace
         end
       end
+
+      # Configures whether RSpec will warn about matcher use which will
+      # potentially cause false positives in tests.
+      #
+      # @param value [Boolean]
+      attr_writer :warn_about_false_positives
+
+      # Indicates whether RSpec will warn about matcher use which will
+      # potentially cause false positives in tests, generally you want to
+      # avoid such scenarios so this defaults to `true`.
+      def warn_about_false_positives?
+        @warn_about_false_positives
+      end
     end
 
     # The configuration object.

lib/rspec/matchers.rb

@@ -732,7 +732,7 @@ def output(expected=nil)
     #   expect { do_something_risky }.to raise_error(PoorRiskDecisionError, /oo ri/)
     #
     #   expect { do_something_risky }.not_to raise_error
-    def raise_error(error=Exception, message=nil, &block)
+    def raise_error(error=nil, message=nil, &block)
       BuiltIn::RaiseError.new(error, message, &block)
     end
     alias_method :raise_exception,  :raise_error

lib/rspec/matchers/built_in/raise_error.rb

@@ -8,10 +8,16 @@ module BuiltIn
       class RaiseError
         include Composable
 
-        def initialize(expected_error_or_message=Exception, expected_message=nil, &block)
+        def initialize(expected_error_or_message=nil, expected_message=nil, &block)
           @block = block
           @actual_error = nil
+          @warn_about_bare_error =
+            RSpec::Expectations.configuration.warn_about_false_positives? &&
+            expected_error_or_message.nil?
+
           case expected_error_or_message
+          when nil
+            @expected_error, @expected_message = Exception, expected_message
           when String, Regexp
             @expected_error, @expected_message = Exception, expected_error_or_message
           else
@@ -23,6 +29,7 @@ def initialize(expected_error_or_message=Exception, expected_message=nil, &block
         # Specifies the expected error message.
         def with_message(expected_message)
           raise_message_already_set if @expected_message
+          @warn_about_bare_error = false
           @expected_message = expected_message
           self
         end
@@ -37,6 +44,7 @@ def matches?(given_proc, negative_expectation=false, &block)
           @eval_block = false
           @eval_block_passed = false
 
+          warn_about_bare_error if warning_about_bare_error && !negative_expectation
           return false unless Proc === given_proc
 
           begin
@@ -48,9 +56,7 @@ def matches?(given_proc, negative_expectation=false, &block)
             end
           end
 
-          unless negative_expectation
-            eval_block if @raised_expected_error && @with_expected_message && @block
-          end
+          eval_block if !negative_expectation && ready_to_eval_block?
 
           expectation_matched?
         end
@@ -103,6 +109,10 @@ def block_matches?
           @eval_block ? @eval_block_passed : true
         end
 
+        def ready_to_eval_block?
+          @raised_expected_error && @with_expected_message && @block
+        end
+
         def eval_block
           @eval_block = true
           begin
@@ -133,6 +143,21 @@ def prevent_invalid_expectations
           raise specific_class_error
         end
 
+        def warning_about_bare_error
+          @warn_about_bare_error && @block.nil?
+        end
+
+        def warn_about_bare_error
+          RSpec.warning("Using the `raise_error` matcher without providing a specific " \
+                        "error or message risks false positives, since `raise_error` " \
+                        "will match when Ruby raises a `NoMethodError`, `NameError` or " \
+                        "`ArgumentError`, potentially allowing the expectation to pass " \
+                        "without even executing the method you are intending to call. " \
+                        "Instead consider providing a specific error class or message. " \
+                        "This message can be supressed by setting: " \
+                        "`RSpec::Expectations.configuration.warn_about_false_positives = false`")
+        end
+
         def expected_error
           case @expected_message
           when nil

spec/rspec/expectations/configuration_spec.rb

@@ -87,6 +87,23 @@ def delegated?; true; end
         end
       end
 
+      describe "#warn_about_false_positives?" do
+        it "is true by default" do
+          expect(config.warn_about_false_positives?).to be true
+        end
+
+        it "can be set to false" do
+          config.warn_about_false_positives = false
+          expect(config.warn_about_false_positives?).to be false
+        end
+
+        it "can be set back to true" do
+          config.warn_about_false_positives = false
+          config.warn_about_false_positives = true
+          expect(config.warn_about_false_positives?).to be true
+        end
+      end
+
       shared_examples "configuring the expectation syntax" do
         before do
           @orig_syntax = RSpec::Matchers.configuration.syntax

spec/rspec/matchers/built_in/base_matcher_spec.rb

@@ -26,7 +26,7 @@ module RSpec::Matchers::BuiltIn
       it "re-raises any error other than one of those specified" do
         expect do
           matcher.match_unless_raises(ArgumentError){ raise "foo" }
-        end.to raise_error
+        end.to raise_error "foo"
       end
 
       it "stores the rescued exception for use in messages" do

spec/rspec/matchers/built_in/raise_error_spec.rb

@@ -1,11 +1,37 @@
 RSpec.describe "expect { ... }.to raise_error" do
   it_behaves_like("an RSpec matcher", :valid_value => lambda { raise "boom" },
                                       :invalid_value => lambda { }) do
-    let(:matcher) { raise_error }
+    let(:matcher) { raise_error Exception }
   end
 
   it "passes if anything is raised" do
-    expect {raise}.to raise_error
+    expect { raise "error" }.to raise_error "error"
+  end
+
+  it "issues a warning when used without an error class or message" do
+    expect_warning_with_call_site __FILE__, __LINE__+1, /without providing a specific error/
+    expect { raise }.to raise_error
+  end
+
+  it "can supresses the warning when configured to do so", :warn_about_false_positives do
+    RSpec::Expectations.configuration.warn_about_false_positives = false
+    expect_no_warnings
+    expect { raise }.to raise_error
+  end
+
+  it 'does not issue a warning when an exception class is specified (even if it is just `Exception`)' do
+    expect_no_warnings
+    expect { raise "error" }.to raise_error Exception
+  end
+
+  it 'does not issue a warning when a message is specified' do
+    expect_no_warnings
+    expect { raise "error" }.to raise_error "error"
+  end
+
+  it 'does not issue a warning when a block is passed' do
+    expect_no_warnings
+    expect { raise "error" }.to raise_error { |_| }
   end
 
   it "passes if an error instance is expected" do
@@ -48,14 +74,14 @@ def to_s
 
   it "fails if nothing is raised" do
     expect {
-      expect {}.to raise_error
+      expect { }.to raise_error Exception
     }.to fail_with("expected Exception but nothing was raised")
   end
 end
 
 RSpec.describe "raise_exception aliased to raise_error" do
   it "passes if anything is raised" do
-    expect {raise}.to raise_exception
+    expect { raise "exception" }.to raise_exception "exception"
   end
 end
 

spec/rspec/matchers/description_generation_spec.rb

@@ -136,7 +136,7 @@ def object.has_taste_for?(*args); true; end
   end
 
   example "expect(...).to raise_error" do
-    expect { raise }.to raise_error
+    expect { raise 'foo' }.to raise_error Exception
     expect(RSpec::Matchers.generated_description).to eq "should raise Exception"
   end
 

spec/spec_helper.rb

@@ -98,6 +98,12 @@ def hash_inspect(hash)
   end
 end
 
+RSpec.shared_context "with warn_about_false_positives set to false", :warn_about_false_positives do
+  original_value = RSpec::Expectations.configuration.warn_about_false_positives?
+
+  after(:context)  { RSpec::Expectations.configuration.warn_about_false_positives = original_value }
+end
+
 module MinitestIntegration
   include ::RSpec::Support::InSubProcess