Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve failure messages when not expecting a list #5

Merged
merged 5 commits into from
Apr 23, 2024
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
43 changes: 19 additions & 24 deletions lib/rspec/sql.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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:

Expand All @@ -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(&)
Expand Down
60 changes: 60 additions & 0 deletions lib/rspec/sql/query_matcher.rb
Original file line number Diff line number Diff line change
@@ -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
Copy link

Choose a reason for hiding this comment

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

To minimise the dependency on QuerySummary, I would inject it in the constructor like this :

def initialize(queries, expected, query_summary=QuerySummary)
  ...
end

Now you are depending on an object that implement summary and not QuerySummary directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's okay to depend on QuerySummary here. It means that the calling module doesn't need to know the QuerySummary. It reduces the API and the matcher definition doesn't need to know which class is responsible for which format.

end
end
end
end
40 changes: 24 additions & 16 deletions spec/lib/rspec/sql_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand All @@ -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:

Expand Down Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading