Merge pull request #5 from openfoodfoundation/error-messages

Improve failure messages when not expecting a list

lib/rspec/sql.rb

@@ -3,7 +3,7 @@
 require "active_support"
 require "rspec"
 
-require_relative "sql/query_summary"
+require_relative "sql/query_matcher"
 
 # We are building within the RSpec namespace for consistency and convenience.
 # We are not part of the RSpec team though.
@@ -14,32 +14,22 @@ module Sql; end
   Matchers.define :query_database do |expected = nil|
     match do |block|
       @queries = scribe_queries(&block)
+      @matcher = Sql::QueryMatcher.new(@queries, expected)
+      expected = matcher.expected
 
-      if expected.nil?
-        !@queries.empty?
-      elsif expected.is_a?(Integer)
-        @queries.size == expected
-      elsif expected.is_a?(Enumerator) && expected.inspect.match?(/:times>$/)
-        @queries.size == expected.size
-      elsif expected.is_a?(Array)
-        query_names == expected
-      elsif expected.is_a?(Hash)
-        query_summary == expected
-      else
-        raise "What are you expecting?"
-      end
+      matcher.matches?
     end
 
     failure_message do |_block|
-      if expected.is_a?(Enumerator) && expected.inspect.match?(/:times>$/)
-        expected = expected.size
+      if expected.nil?
+        return "Expected at least one database query but observed none."
       end
 
       <<~MESSAGE
         Expected database queries: #{expected}
-        Actual database queries:   #{query_names}
+        Actual database queries:   #{matcher.actual}
 
-        Diff: #{Expectations.differ.diff_as_object(query_names, expected)}
+        Diff: #{diff(matcher.actual, expected)}
 
         Full query log:
 
@@ -59,16 +49,21 @@ def supports_block_expectations?
       true
     end
 
-    def query_names
-      @queries.map { |q| q[:name] || q[:sql].split.take(2).join(" ") }
-    end
-
     def query_descriptions
       @queries.map { |q| "#{q[:name]}  #{q[:sql]}" }
     end
 
-    def query_summary
-      Sql::QuerySummary.new(@queries).summary
+    def matcher
+      @matcher
+    end
+
+    def diff(actual, expected)
+      if expected.is_a?(Numeric)
+        change = actual - expected
+        format("%+d", change)
+      else
+        Expectations.differ.diff_as_object(actual, expected)
+      end
     end
 
     def scribe_queries(&)

lib/rspec/sql/query_matcher.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+require_relative "query_summary"
+
+module RSpec
+  module Sql
+    # Compares expectations with actual queries.
+    class QueryMatcher
+      attr_reader :expected
+
+      def initialize(queries, expected)
+        @queries = queries
+        @expected = sanitize_number(expected)
+      end
+
+      def matches?
+        # Simple presence validation.
+        return !@queries.empty? if expected.nil?
+
+        actual == expected
+      end
+
+      def actual
+        @actual ||= actual_compared_to_expected
+      end
+
+      private
+
+      # Support writing: `is_expected.to query_database 5.times`
+      def sanitize_number(expected)
+        if expected.is_a?(Enumerator) && expected.inspect.match?(/:times>$/)
+          expected.size
+        else
+          expected
+        end
+      end
+
+      def actual_compared_to_expected
+        case expected
+        when Integer
+          @queries.size
+        when Array
+          query_names
+        when Hash
+          query_summary
+        else
+          raise "What are you expecting?"
+        end
+      end
+
+      def query_names
+        @queries.map { |q| q[:name] || q[:sql].split.take(2).join(" ") }
+      end
+
+      def query_summary
+        QuerySummary.new(@queries).summary
+      end
+    end
+  end
+end

spec/lib/rspec/sql_spec.rb

@@ -67,17 +67,29 @@
     )
   end
 
-  it "prints user-friendly message expecting a number" do
-    message = error_message { expect { User.last }.to query_database 2 }
+  it "prints user-friendly message expecting nothing" do
+    message = error_message { expect { User.last }.not_to query_database }
     expect(message).to eq <<~TXT
-      Expected database queries: 2
-      Actual database queries:   ["User Load"]
+      Expected no database queries but observed:
 
-      Diff:
-      @@ -1 +1 @@
-      -2
-      +["User Load"]
+      User Load  SELECT "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT ?
+    TXT
+  end
+
+  it "prints user-friendly message expecting something" do
+    message = error_message { expect { nil }.to query_database }
+    expect(message).to eq(
+      "Expected at least one database query but observed none."
+    )
+  end
+
+  it "prints user-friendly message expecting a number" do
+    message = error_message { expect { User.last }.to query_database 0 }
+    expect(message).to eq <<~TXT
+      Expected database queries: 0
+      Actual database queries:   1
 
+      Diff: +1
 
       Full query log:
 
@@ -89,13 +101,9 @@
     message = error_message { expect { User.last }.to query_database 2.times }
     expect(message).to eq <<~TXT
       Expected database queries: 2
-      Actual database queries:   ["User Load"]
-
-      Diff:
-      @@ -1 +1 @@
-      -2
-      +["User Load"]
+      Actual database queries:   1
 
+      Diff: -1
 
       Full query log:
 
@@ -134,12 +142,12 @@
     # This message could be better but nobody has asked for it yet.
     expect(message).to eq <<~TXT
       Expected database queries: {:update=>{:user=>1}}
-      Actual database queries:   ["User Load"]
+      Actual database queries:   {:select=>{:users=>1}}
 
       Diff:
       @@ -1 +1 @@
       -:update => {:user=>1},
-      +["User Load"]
+      +:select => {:users=>1},
 
 
       Full query log:

spec/spec_helper.rb

@@ -80,7 +80,7 @@
   # Print the 10 slowest examples and example groups at the
   # end of the spec run, to help surface which specs are running
   # particularly slow.
-  config.profile_examples = 10
+  # config.profile_examples = 10
 
   # Run specs in random order to surface order dependencies. If you find an
   # order dependency and want to debug it, you can fix the order by providing