Expose `assert_queries_match` and `assert_no_queries_match` assertions #50373

fatkodima · 2023-12-16T21:45:34Z

Follow up to #50281.

The originally introduced (exposed) helpers are a bit limited. For example:

we must specify an exact number of queries we are expecting
only application related queries are considered, schema/transactions related queries are always skipped
only one matcher can be specified

These limitations work most of the time for regular users, but not when trying to test some lower behavior (for example, inside some gem's tests).

For example, it was not possible to test that an index and a foreign key are created when running some operation.
Now, this can be done via:

assert_queries_match(/create index/i, include_schema: true) do
  do_something
end

cc @byroot @p8

p8 · 2023-12-17T14:04:12Z

activestorage/test/models/variant_with_record_test.rb

-      assert_queries(6) do
-        # 6 queries:
-        # users x 1
+      assert_queries(5) do


Strange 🤔 Any idea why this changed?

The assert_queries method originally counted for cached queries, now it is not.
We can probably also have an option to configure if we count for cached queries or not, but not sure if it will be used much.

p8 · 2023-12-17T14:13:54Z

I like that this cleans up the ActiveRecord code base and makes sure we only have one assert_queries.
My original PR was mostly for Rails apps. I hadn't thought of libraries using it.
Not sure if the options are too complex for a Rails app, but as these assertions are used in edge cases it might make sense to give more control.
Should the other assertions like assert_sql also be made public?

fatkodima · 2023-12-17T15:09:21Z

Should the other assertions like assert_sql also be made public?

My original idea for this PR was exactly to make assert_sql public, but when I investigated the current assert_queries implementation and realised that it can be extended, the need for assert_sql disappeared. I actually changed its implementation in this PR to use assert_queries.

byroot · 2023-12-18T12:15:07Z

So in my opinion we should keep the assert relatively simple and straight forward, I'd rather avoid to cram too much in there.

We can however add a few more assertions, e.g. for the regexp we could have assert_performed_query /SELECT ..../.

fatkodima · 2023-12-18T14:08:40Z

Wdyt if I left assert_queries/assert_no_queries as is, and introduce new assert_sql/assert_no_sql with all the needed complexity?

seanpdoyle · 2023-12-18T14:41:18Z

activerecord/lib/active_record/testing/query_assertions.rb

+            if expected_count.nil?
+              assert_operator matched_queries.size, :>=, 1, "1 or more queries expected, but none were executed."
+            else
+              assert_equal expected_count, matched_queries.size, "#{matched_queries.size} instead of #{expected_count} queries were executed. Queries: #{matched_queries.join("\n\n")}"
+            end
+          end


Would it be valuable to support Range instances too?

Something like assert_queries(2..3) { ... }. It'd also support endless Ranges: assert_queries(..3) { ... }.

Suggested change

if expected_count.nil?

assert_operator matched_queries.size, :>=, 1, "1 or more queries expected, but none were executed."

else

assert_equal expected_count, matched_queries.size, "#{matched_queries.size} instead of #{expected_count} queries were executed. Queries: #{matched_queries.join("\n\n")}"

end

end

case expected_count

when NilClass

assert_operator matched_queries.size, :>=, 1, "1 or more queries expected, but none were executed."

when Range

assert_includes expected_count, matched_queries.size, "expected #{matched_queries.size} executed queries to be within #{expected_count}. Queries: #{matched_queries.join("\n\n")}"

else

assert_equal expected_count, matched_queries.size, "#{matched_queries.size} instead of #{expected_count} queries were executed. Queries: #{matched_queries.join("\n\n")}"

end

end

Can you provide a use case for this?

The main motivation is flexibility and maintenance.

I'm imagining introducing this assertion into an existing application test suite in the hopes that it'd drive efforts to minimize queries (either as an optimization or as a reaction to issues like N+1s). In this hypothetical application, I might introduce the assertion with a specific query, pass CI, then merge my change. A teammate might make a change that would make a query and the test might fail. They could update the assertion to use the more correct number, pass CI, then merge their change.

Depending on the goals of the team using this assertion that type of churn might start a virtuous cycle that forces the team to change the test's number for necessary queries. Alternatively, it might change often enough to develop a negative perception, feel onerous, and ultimately get removed.

In cases where the assertion is used as an optimization, teams might be interested in providing a Range (like "fewer than 10" or ..10) that they could use as a arbitrary goal.

Personally, my preference is for the specific, exact count version. In my experiences, constant churn can deter that style of usage.

Personally, my preference is for the specific, exact count version.

This is my preference too. Let's see what other people say.

byroot · 2023-12-18T22:16:13Z

Wdyt if I left assert_queries/assert_no_queries as is, and introduce new assert_sql/assert_no_sql with all the needed complexity?

Yeah, I'd prefer that. As long as assert_sql doesn't do too much. I think taking a single regexp would be plenty. If you need to match a second one, you can nest them. If you need more than that, you can write a custom assertion IMO.

byroot · 2023-12-18T22:17:21Z

Just to clarify my stance, I think we should aim for the 20/80 here. 20% of the features that are sufficient for 80% of users.

fatkodima · 2023-12-19T00:08:54Z

Made some changes in a separate commit - reverted an ability to provide multiple regexes. Currently (as before), it allows only one regexp.

Now, the only changes this PR introduces is the ability to not pass an exact number of queries and ability to account for internal queries (like TRANSACTION, schema related queries).

Considering, that these changes are now pretty simple imo and even cover everything from assert_sql (except a nicer message for multiple regexes), I wonder if we really need to introduce any other assertion methods? 🤔

If this is ok, I will squash my commits.

byroot · 2023-12-19T06:59:33Z

activerecord/lib/active_record/testing/query_assertions.rb

+      #
+      #   assert_queries(ignore_none: true) { Post.columns }
+      #
+      def assert_queries(expected_count = nil, matcher: nil, ignore_none: false, &block)


I really don't like ignore_none as a parameter name, as it's a double negation.

Also I wonder that the use case is?

byroot · 2023-12-19T07:00:36Z

I still think it would be much simpler to use if that capability was in another assert_ method.

fatkodima · 2023-12-19T09:58:41Z

if that capability was in another assert_ method.

Can you explain, which exact capability do you mean?

Yeah, I was thinking about assert_sql as another assertion, but that leads to another confusion, imo, as what is the difference between assert_queries and assert_sql then, considering that both are queries and that assert_queries already support matcher argument.

I would appreciate, if you will help me separate capabilities between assert_queries and some other assertion method (and give it a name) 🙏

byroot · 2023-12-19T10:19:49Z

Can you explain, which exact capability do you mean?

Ok. So my thinking is I see two use cases.

The main one is to assert how many queries are performed by a block of code, this generally is for performance critical sections of code, you want to make sure you don't accidentally introduce a new query. That is what assert_queries currently does. Maybe it could be renamed to assert_queries_count or something.

Then the other use case it to assert that a specific query was (or wasn't) performed. To be honest I don't quite see as uch value in this one, it's more niche I think, but I don't mind so much having it.

and that assert_queries already support matcher argument.

Should it though? We're making this public now, so we have the opportunity to rethink the interface. I'm deep in the find_or_create bug right now, but would be worth looking at how the matcher is used in Rails test suite, to see if it wouldn't be just as fine using a different assert?

fatkodima · 2023-12-19T17:33:07Z

Updated assert_queries to simply check for the number of queries and added assert_sql to be able to do more sophisticated checks, like for regexes, internal queries etc. Please, take a look.

seanpdoyle · 2023-12-19T19:01:12Z

activerecord/lib/active_record/testing/query_assertions.rb

+      #
+      #   assert_sql(include_all: true) { Post.columns }
+      #
+      def assert_sql(expected_count = nil, matcher: nil, include_all: false, &block)


How do you feel about renaming the :matcher keyword argument to :match? It'd mirror the new :match keyword argument that assert_raises supports, and would correspond to the Regexp#match method name.

p8 · 2023-12-19T19:13:17Z

If assert_sql requires a match argument we could rename the assertion and make the Regexp a regular required comment:

assert_sql_match(/create index/i, include_all: true) do
  do_something
end

Or make it more inline with assert_queries:

assert_query_match(/create index/i, include_all: true) do
  do_something
end

fatkodima · 2023-12-19T19:16:52Z

If assert_sql requires a match argument

Currently, it does not require a matcher argument.

byroot · 2023-12-20T09:06:10Z

activerecord/lib/active_record/testing/query_assertions.rb

+      #
+      # If the +:matcher+ option is provided, only queries that match the matcher are counted.
+      #
+      #   assert_no_sql(1, match: /LIMIT \?/) { Post.first }


You probably meant:

Suggested change

# assert_no_sql(1, match: /LIMIT \?/) { Post.first }

# assert_no_sql(match: /LIMIT \?/) { Post.first }

byroot · 2023-12-20T09:06:47Z

activerecord/lib/active_record/testing/query_assertions.rb

+      #
+      #   assert_sql(include_all: true) { Post.columns }
+      #
+      def assert_sql(expected_count = nil, match: nil, include_all: false, &block)


I think I'd inverse the parameters:

Suggested change

def assert_sql(expected_count = nil, match: nil, include_all: false, &block)

def assert_sql(match, count: nil, include_all: false, &block)

What do you think?

I originally thought for match to not be required to be able to write something like assert_sql(include_all: true) to test that any SQL query was made.

IMO assert_sql means I want to assert some query matches something.

If I only care about the count, it should be assert_queries.

Yeah, I agree with you. There is a need to update quite a few test cases to specify these concrete sqls, like in https://github.com/rails/rails/pull/50373/files#diff-9bc4b7719b110cde3044f7e2ce6f8cda8f8e3263790d600e6d6792e40c02fa70R831

Will try to update with the suggestion.

Also, does the count apply to the matching queries or all queries?
Edit: Nevermind, this is rather obvious.

Made matcher a required argument.

byroot · 2023-12-20T13:51:57Z

activerecord/test/cases/migration_test.rb

@@ -1437,7 +1437,7 @@ def test_changing_column_null_with_default
        raise "need an expected query count for #{classname}"
      }

-      assert_queries(expected_query_count, ignore_none: true) do
+      assert_sql(//, count: expected_query_count, include_all: true) do


This suggest assert_queries should have an include_all too.

byroot · 2023-12-20T13:54:38Z

Starting to look good to me. Only the assert_sql(// use strike me as not ideal, all the rest seem sensible.

p8 · 2023-12-20T17:04:24Z

If I only see assert_sql and assert_queries it's unclear to me which one accepts a matcher.
Also I would need to look up include_all to see what is normally excluded.
Not sure if two seperate options:include_transactions and :include_schema is too much, but most of the time you'd probably only need to set one to true.

Maybe something like:

assert_sql_match(/ADD FOREIGN KEY/i, include_schema: true) do
  @connection.add_foreign_key(:comments, :posts)
end

byroot · 2023-12-20T17:43:02Z

:include_transactions and :include_schema is too much

I'm not even sure ignoring transactions make that much sense. Looking at our private implementation, it ignore schema queries by default (because they tend to be flaky), but transactions are included.

And IMO it makes sense because begin; query_1; query_2; commit isn't the same thing as just query_1; query_2.

fatkodima · 2023-12-20T18:37:42Z

If I only see assert_sql and assert_queries it's unclear to me which one accepts a matcher.

What if then we rename assert_sql to assert_queries_match? Or maybe there is a better name?

If we are going to change that now transactions queries are accounted by default, should we rename :include_all to something else? :include_schema etc?

byroot · 2023-12-20T18:40:09Z

What if then we rename assert_sql to assert_queries_match? Or maybe there is a better name?

Or assert_queries to assert_queries_count. Then assert_sql can become assert_query_match?

Just a though, no strong opinion here.

If we are going to change that now transactions queries are accounted by default, should we rename :include_all to something else? :include_schema etc?

Yeah that's the idea.

p8 · 2023-12-20T20:24:06Z

I like assert_queries_count and assert_query_match.

fatkodima · 2023-12-20T21:56:48Z

assert_query_match

You probably meant assert_queries_match? On par with assert_queries_count

fatkodima · 2023-12-20T23:34:13Z

Changed names:
assert_queries -> assert_queries_count
assert_sql -> assert_queries_match
assert_no_sql -> assert_no_queries_match
include_all -> include_schema

and transaction related queries are now always counted.

byroot · 2023-12-21T09:52:25Z

LGTM. What do you think @p8 ?

p8 · 2023-12-21T11:58:31Z

Looks great!

fatkodima · 2023-12-21T12:21:07Z

Thank you all! ❤️ That was quite a lengthy discussion 💪 😅

p8 · 2023-12-21T13:36:47Z

Thanks for the work @fatkodima ! 😄

Follow up to rails#50382. `assert_queries` was renamed to `assert_queries_count` in in rails#50373.

Follow up to rails#50382. `assert_queries` was renamed to `assert_queries_count` in rails#50373.

@fatkodima

This was initially changed in rails#50373, with an explanation that the cached query was not included in the count. rails#50373 (comment) However, it seems that this is no longer the case. /cc @fatkodima

@fatkodima

This was initially changed in rails#50373, with an explanation that the cached query was not included in the count. rails#50373 (comment) However, it seems that this is no longer the case. /cc @fatkodima

See rails/rails#50373

rails-bot bot added the activerecord label Dec 16, 2023

fatkodima force-pushed the improve-queries-assertions-matchers branch from c1572f3 to 3a0ee30 Compare December 16, 2023 22:19

rails-bot bot added the activestorage label Dec 16, 2023

fatkodima force-pushed the improve-queries-assertions-matchers branch from 3a0ee30 to 9a1543b Compare December 16, 2023 22:28

p8 reviewed Dec 17, 2023

View reviewed changes

seanpdoyle reviewed Dec 18, 2023

View reviewed changes

fatkodima force-pushed the improve-queries-assertions-matchers branch from 9a1543b to a22acbd Compare December 19, 2023 00:02

byroot reviewed Dec 19, 2023

View reviewed changes

fatkodima force-pushed the improve-queries-assertions-matchers branch from a22acbd to 5d7fc51 Compare December 19, 2023 17:30

fatkodima changed the title ~~Improve assert_queries and assert_no_queries with more matching options~~ Expose assert_sql and assert_no_sql assertions Dec 19, 2023

seanpdoyle reviewed Dec 19, 2023

View reviewed changes

fatkodima force-pushed the improve-queries-assertions-matchers branch from 5d7fc51 to 17bdd93 Compare December 19, 2023 22:11

byroot reviewed Dec 20, 2023

View reviewed changes

fatkodima force-pushed the improve-queries-assertions-matchers branch from 17bdd93 to d765ecb Compare December 20, 2023 13:35

byroot reviewed Dec 20, 2023

View reviewed changes

fatkodima force-pushed the improve-queries-assertions-matchers branch from d765ecb to ec23031 Compare December 20, 2023 16:50

fatkodima force-pushed the improve-queries-assertions-matchers branch from 3d886f1 to 1176988 Compare December 20, 2023 23:10

rails-bot bot added the actionview label Dec 20, 2023

fatkodima changed the title ~~Expose assert_sql and assert_no_sql assertions~~ Expose assert_queries_match and assert_no_queries_match assertions Dec 20, 2023

fatkodima force-pushed the improve-queries-assertions-matchers branch from 1176988 to 118d67e Compare December 20, 2023 23:11

Expose assert_queries_match and assert_no_queries_match assertions

f48bbff

fatkodima force-pushed the improve-queries-assertions-matchers branch from 118d67e to f48bbff Compare December 20, 2023 23:30

rails-bot bot added the actiontext label Dec 20, 2023

byroot merged commit 60df089 into rails:main Dec 21, 2023
4 checks passed

fatkodima deleted the improve-queries-assertions-matchers branch December 21, 2023 12:20

byroot mentioned this pull request Jan 5, 2024

Expose assert_queries and assert_no_queries assertions #50281

Merged

4 tasks

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Jan 11, 2024

Fix assertion method call

7da9209

Follow up to rails#50382. `assert_queries` was renamed to `assert_queries_count` in in rails#50373.

jonathanhefner mentioned this pull request Jan 11, 2024

Fix assertion method call #50716

Closed

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Jan 11, 2024

Fix assertion method call

4abb8af

Follow up to rails#50382. `assert_queries` was renamed to `assert_queries_count` in rails#50373.

aidanharan added a commit to rails-sqlserver/activerecord-sqlserver-adapter that referenced this pull request Apr 18, 2024

Lease connections and renamed assert_queries to assert_queries_count

1557674

See rails/rails#50373

aidanharan added a commit to rails-sqlserver/activerecord-sqlserver-adapter that referenced this pull request Apr 18, 2024

Rename assert_queries test

1e2b1b4

See rails/rails#50373

aidanharan mentioned this pull request Apr 18, 2024

Rename assert_queries test rails-sqlserver/activerecord-sqlserver-adapter#1158

Merged

aidanharan added a commit to rails-sqlserver/activerecord-sqlserver-adapter that referenced this pull request Apr 18, 2024

Method assert_sql changed to assert_queries_match

c85e862

See rails/rails#50373

	# assert_no_sql(1, match: /LIMIT \?/) { Post.first }
	# assert_no_sql(match: /LIMIT \?/) { Post.first }

	def assert_sql(expected_count = nil, match: nil, include_all: false, &block)
	def assert_sql(match, count: nil, include_all: false, &block)

Expose assert_queries_match and assert_no_queries_match assertions #50373

Expose assert_queries_match and assert_no_queries_match assertions #50373

Conversation

fatkodima commented Dec 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p8 commented Dec 17, 2023

fatkodima commented Dec 17, 2023

byroot commented Dec 18, 2023

fatkodima commented Dec 18, 2023

seanpdoyle Dec 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Dec 18, 2023

byroot commented Dec 18, 2023

fatkodima commented Dec 19, 2023 • edited

Choose a reason for hiding this comment

byroot commented Dec 19, 2023

fatkodima commented Dec 19, 2023

byroot commented Dec 19, 2023

fatkodima commented Dec 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p8 commented Dec 19, 2023

fatkodima commented Dec 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p8 Dec 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byroot commented Dec 20, 2023

p8 commented Dec 20, 2023 • edited

byroot commented Dec 20, 2023

fatkodima commented Dec 20, 2023 • edited

byroot commented Dec 20, 2023

p8 commented Dec 20, 2023

fatkodima commented Dec 20, 2023

fatkodima commented Dec 20, 2023

byroot commented Dec 21, 2023

p8 commented Dec 21, 2023 • edited

fatkodima commented Dec 21, 2023

p8 commented Dec 21, 2023

Expose `assert_queries_match` and `assert_no_queries_match` assertions #50373

Expose `assert_queries_match` and `assert_no_queries_match` assertions #50373

fatkodima commented Dec 16, 2023 •

edited

seanpdoyle Dec 18, 2023 •

edited

fatkodima commented Dec 19, 2023 •

edited

fatkodima commented Dec 19, 2023 •

edited

p8 Dec 20, 2023 •

edited

p8 commented Dec 20, 2023 •

edited

fatkodima commented Dec 20, 2023 •

edited

p8 commented Dec 21, 2023 •

edited