Merge pull request #4 from openfoodfoundation/error-messages

Improve error message for n.times syntax

Author: mkllnk

.github/workflows/test.yml

@@ -21,3 +21,4 @@ jobs:
           ruby-version: ${{ matrix.ruby-version }}
           bundler-cache: true
       - run: bundle exec rspec
+      - run: bundle exec rubocop

.rubocop.yml

@@ -0,0 +1,31 @@
+AllCops:
+  NewCops: enable
+
+Layout/LineLength:
+  Max: 80 # default is 120
+
+Metrics/BlockLength:
+  AllowedMethods:
+    # RSpec context block containing many smaller blocks:
+    - Matchers.define
+    # RSpec context block containing many smaller examples:
+    - RSpec.describe
+
+Style/BlockDelimiters:
+  BracesRequiredMethods:
+    # The RSpec expect syntax is prettier with braced blocks.
+    - expect
+
+Style/StringLiterals:
+  # It's my preference to add or remove variable from strings without replacing
+  # the outer quotes.
+  EnforcedStyle: double_quotes
+
+Style/TrailingCommaInArrayLiteral:
+  # Consistent commas allow for adding and removing lines without affecting
+  # other lines of arrays. Less editing, prettier diffs.
+  EnforcedStyleForMultiline: consistent_comma
+
+Style/TrailingCommaInHashLiteral:
+  # Same reason as above.
+  EnforcedStyleForMultiline: consistent_comma

Gemfile

@@ -1,5 +1,8 @@
+# frozen_string_literal: true
+
 source "https://rubygems.org"
 gemspec
 
 gem "activerecord"
+gem "rubocop", require: false
 gem "sqlite3"

Gemfile.lock

@@ -24,6 +24,7 @@ GEM
       minitest (>= 5.1)
       mutex_m
       tzinfo (~> 2.0)
+    ast (2.4.2)
     base64 (0.2.0)
     bigdecimal (3.1.6)
     concurrent-ruby (1.2.3)
@@ -33,8 +34,18 @@ GEM
       ruby2_keywords
     i18n (1.14.1)
       concurrent-ruby (~> 1.0)
+    json (2.7.2)
+    language_server-protocol (3.17.0.3)
     minitest (5.22.2)
     mutex_m (0.2.0)
+    parallel (1.24.0)
+    parser (3.3.0.5)
+      ast (~> 2.4.1)
+      racc
+    racc (1.7.3)
+    rainbow (3.1.1)
+    regexp_parser (2.9.0)
+    rexml (3.2.6)
     rspec (3.13.0)
       rspec-core (~> 3.13.0)
       rspec-expectations (~> 3.13.0)
@@ -48,18 +59,34 @@ GEM
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.13.0)
     rspec-support (3.13.0)
+    rubocop (1.63.1)
+      json (~> 2.3)
+      language_server-protocol (>= 3.17.0)
+      parallel (~> 1.10)
+      parser (>= 3.3.0.2)
+      rainbow (>= 2.2.2, < 4.0)
+      regexp_parser (>= 1.8, < 3.0)
+      rexml (>= 3.2.5, < 4.0)
+      rubocop-ast (>= 1.31.1, < 2.0)
+      ruby-progressbar (~> 1.7)
+      unicode-display_width (>= 2.4.0, < 3.0)
+    rubocop-ast (1.31.2)
+      parser (>= 3.3.0.4)
+    ruby-progressbar (1.13.0)
     ruby2_keywords (0.0.5)
     sqlite3 (1.7.2-x86_64-linux)
     timeout (0.4.1)
     tzinfo (2.0.6)
       concurrent-ruby (~> 1.0)
+    unicode-display_width (2.5.0)
 
 PLATFORMS
   x86_64-linux
 
 DEPENDENCIES
   activerecord
   rspec-sql!
+  rubocop
   sqlite3
 
 BUNDLED WITH

lib/rspec/sql.rb

@@ -5,10 +5,13 @@
 
 require_relative "sql/query_summary"
 
+# We are building within the RSpec namespace for consistency and convenience.
+# We are not part of the RSpec team though.
 module RSpec
+  # RSpec::Sql contains our code.
   module Sql; end
 
-  RSpec::Matchers.define :query_database do |expected = nil|
+  Matchers.define :query_database do |expected = nil|
     match do |block|
       @queries = scribe_queries(&block)
 
@@ -28,6 +31,10 @@ module Sql; end
     end
 
     failure_message do |_block|
+      if expected.is_a?(Enumerator) && expected.inspect.match?(/:times>$/)
+        expected = expected.size
+      end
+
       <<~MESSAGE
         Expected database queries: #{expected}
         Actual database queries:   #{query_names}
@@ -41,7 +48,11 @@ module Sql; end
     end
 
     failure_message_when_negated do |_block|
-      "Expected no database queries but observed:\n\n#{query_descriptions.join("\n")}"
+      <<~TXT
+        Expected no database queries but observed:
+
+        #{query_descriptions.join("\n")}
+      TXT
     end
 
     def supports_block_expectations?
@@ -63,9 +74,9 @@ def query_summary
     def scribe_queries(&)
       queries = []
 
-      logger = ->(_name, _started, _finished, _unique_id, payload) {
+      logger = lambda do |_name, _started, _finished, _unique_id, payload|
         queries << payload unless %w[CACHE SCHEMA].include?(payload[:name])
-      }
+      end
 
       ActiveSupport::Notifications.subscribed(logger, "sql.active_record", &)
 

lib/rspec/sql/query_summary.rb

@@ -2,8 +2,9 @@
 
 module RSpec
   module Sql
+    # Converts a list of queries into a summary hash.
     class QuerySummary
-      QUERY_TYPES = [:delete, :insert, :select, :update].freeze
+      QUERY_TYPES = %i[delete insert select update].freeze
 
       attr_reader :summary
 

rspec-sql.gemspec

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 Gem::Specification.new do |s|
   s.name        = "rspec-sql"
   s.version     = "0.0.1"
@@ -8,8 +10,9 @@ Gem::Specification.new do |s|
   s.files       = Dir["lib/**/*.rb"]
   s.homepage    = "https://github.com/openfoodfoundation/rspec-sql"
   s.license     = "AGPL-3.0-or-later"
-  s.required_ruby_version = '>= 3.1', '< 4'
+  s.required_ruby_version = ">= 3.1", "< 4"
 
   s.add_runtime_dependency "activesupport"
   s.add_runtime_dependency "rspec"
+  s.metadata["rubygems_mfa_required"] = "true"
 end

spec/lib/rspec/sql_spec.rb

@@ -66,4 +66,92 @@
       }
     )
   end
+
+  it "prints user-friendly message expecting a number" do
+    message = error_message { expect { User.last }.to query_database 2 }
+    expect(message).to eq <<~TXT
+      Expected database queries: 2
+      Actual database queries:   ["User Load"]
+
+      Diff:
+      @@ -1 +1 @@
+      -2
+      +["User Load"]
+
+
+      Full query log:
+
+      User Load  SELECT "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT ?
+    TXT
+  end
+
+  it "prints user-friendly message expecting x.times" do
+    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"]
+
+
+      Full query log:
+
+      User Load  SELECT "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT ?
+    TXT
+  end
+
+  it "prints user-friendly message expecting list" do
+    message = error_message do
+      expect { User.last }.to query_database ["User Update"]
+    end
+
+    expect(message).to eq <<~TXT
+      Expected database queries: ["User Update"]
+      Actual database queries:   ["User Load"]
+
+      Diff:
+      @@ -1 +1 @@
+      -["User Update"]
+      +["User Load"]
+
+
+      Full query log:
+
+      User Load  SELECT "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT ?
+    TXT
+  end
+
+  it "prints user-friendly message expecting summary" do
+    message = error_message do
+      expect { User.last }.to query_database(
+        update: { user: 1 }
+      )
+    end
+
+    # 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"]
+
+      Diff:
+      @@ -1 +1 @@
+      -:update => {:user=>1},
+      +["User Load"]
+
+
+      Full query log:
+
+      User Load  SELECT "users".* FROM "users" ORDER BY "users"."id" DESC LIMIT ?
+    TXT
+  end
+
+  def error_message
+    yield
+  rescue RSpec::Expectations::ExpectationNotMetError => e
+    # Remove colours and trailing whitespace from message:
+    e.message.gsub(/\e\[(\d+)m/, "").gsub(/ $/, "")
+  end
 end