Add doc for `assert_queries` and `assert_no_queries` #50334

yykamei · 2023-12-12T04:26:40Z

Follow-up to #50281

Motivation / Background

I think assert_queries and assert_no_queries are so useful that I want to read the documentation for the assertions. This pull request adds the doc for them and updated the test case named test_assert_queries to verify :matcher works as expected.

Additional information

I also changed assert_queries to make sure the subscriber of "sql.active_record" is unsubscribed without affecting other test cases. Without this change, some test cases fail because of execution of String#match, which may cause ArgumentError: invalid byte sequence in UTF-8.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
- This does not include behavior changes

skipkayhil

Looks good to me, will need another review from a committer 👍

yahonda · 2023-12-12T05:05:16Z

QueryLogsTest#test_invalid_encoding_query test gets ArgumentError: invalid byte sequence in UTF-8 for most of Ruby versions and database adapters. I understand this commit does not change the QueryLogsTest#test_invalid_encoding_query test itself, but not sure if they are not related.

https://buildkite.com/rails/rails/builds/102711#018c5c47-a1c3-46da-aea1-457b9f7d70bc/1096-1105

skipkayhil · 2023-12-12T05:13:46Z

I understand this commit does not change the QueryLogsTest#test_invalid_encoding_query test itself, but not sure if they are not related.

Yeah this is really weird... the failures also seem distributed between different adapters on different version of Ruby. I'll retry a few to see if the QueryLogsTest is just flaky

p8 · 2023-12-12T05:23:44Z

activerecord/test/cases/assertions/query_assertions_test.rb

+            Post.where(id: 1).first
+          end
+        }
+        assert_match(/0 instead of 1 queries/, error.message)


Thanks for adding this 🙏
Could you move this to a separate test case method?

Thank you! It makes sense to me. I moved the code with 7a268ec.

By the way, I couldn't run the test on my local machine. It's weird...

p8 · 2023-12-12T05:30:28Z

It seems QueryLogsTest is using these new assertions, which shouldn’t happen.

skipkayhil · 2023-12-12T05:35:18Z

It seems QueryLogsTest is using these new assertions, which shouldn’t happen.

Ah, I think the subscriber stays active for all other tests once the QueryAssertions tests are run once, hence the flakiness.

Edit: Aha! And the tests weren't flaky before because the new assert_queries was never called with a matcher, so it is in fact this PR that is introducing the flakiness

yahonda · 2023-12-12T06:32:32Z

Would you squash your commits?

yykamei · 2023-12-12T06:40:12Z

Thank you. I squashed it, but I'm concerned about it. Is it OK to merge?

#50334 (comment)

yahonda · 2023-12-12T07:02:28Z

(Updated)

Looks like this end closes QueryAssertionsTest class so test_assert_queries_with_matcher and test_assert_no_queries are not executed because they are out of QueryAssertionsTest class.

rails/activerecord/test/cases/assertions/query_assertions_test.rb

Line 25 in 9c8f964

end

yahonda · 2023-12-12T07:34:31Z

Opened a feature request at rubocop-minitest rubocop/rubocop-minitest#279 to find tests that are not executed.

yykamei · 2023-12-12T08:01:13Z

Thank you! I understood the reason that the two cases were not run. I changed assert_queries a bit to make sure ActiveSupport::Notifications.unsubscribe("sql.active_record") is called in order to prevent other test cases from failing because of unexpected match operation.

Edit: This didn't work 😱

p8 · 2023-12-12T09:09:49Z

@yykamei It seems you just had bad luck with that CI run, the failure seems unrelated.
I'd just run the tests again.

yykamei · 2023-12-12T09:15:15Z

Thank you 🙏

Follow-up to rails#50281 I think `assert_queries` and `assert_no_queries` are so useful that I want to read the documentation for the assertions. This patch adds the doc for them and updated the test case named `test_assert_queries` to verify `:matcher` works as expected. I also changed `assert_queries` to make sure the subscriber of `"sql.active_record"` is unsubscribed without affecting other test cases. Without this change, some test cases fail because of execution of `String#match`, which may cause `ArgumentError: invalid byte sequence in UTF-8`.

This pull request enables `Minitest/NonExecutableTestMethod` cop to find non-executed test that is out of `ActiveSupport::TestCase` and its subclasses. This cop is based on the request since there was a test that is not executed found at rails#50334 (comment) and implemented to RuboCop Minitest 0.34.0 via rubocop/rubocop-minitest#279 This cop works as follows. As of right now, there is no offenses by reverting the merge commit via rails#50334 ``` $ git revert -m 1 9517841 $ bundle exec rubocop Inspecting 3254 files ... snip ... Offenses: activerecord/test/cases/assertions/query_assertions_test.rb:27:5: W: Minitest/NonExecutableTestMethod: Test method should be defined inside a test class to ensure execution. def test_assert_no_queries ... ^^^^^^^^^^^^^^^^^^^^^^^^^^ 3254 files inspected, 1 offense detected $ ```

This pull request enables `Minitest/NonExecutableTestMethod` cop to find non-executed test that is out of `ActiveSupport::TestCase` and its subclasses. This cop is based on the request since there was a test that is not executed found at rails#50334 (comment) and implemented to RuboCop Minitest 0.34.0 via rubocop/rubocop-minitest#279 This cop works as follows. As of right now, there is no offenses by reverting the merge commit via rails#50334 ``` $ git revert -m 1 9517841 $ bundle exec rubocop Inspecting 3254 files ... snip ... Offenses: activerecord/test/cases/assertions/query_assertions_test.rb:27:5: W: Minitest/NonExecutableTestMethod: Test method should be defined inside a test class to ensure execution. def test_assert_no_queries ... ^^^^^^^^^^^^^^^^^^^^^^^^^^ 3254 files inspected, 1 offense detected $ ``` * `Gemfile.lock` has been updated as follows ``` bundle update rubocop rubocop-minitest --conservative ```

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

skipkayhil approved these changes Dec 12, 2023

View reviewed changes

skipkayhil added the ready PRs ready to merge label Dec 12, 2023

p8 reviewed Dec 12, 2023

View reviewed changes

yykamei force-pushed the add_doc_for_assert_queries branch from 7a268ec to 9c8f964 Compare December 12, 2023 06:37

yahonda mentioned this pull request Dec 12, 2023

test_* method out of ActiveSupport::TestCase subclasses rubocop/rubocop-minitest#279

Closed

yykamei force-pushed the add_doc_for_assert_queries branch from 5096ddb to 6982acf Compare December 12, 2023 08:16

yykamei force-pushed the add_doc_for_assert_queries branch from 6982acf to ccc512f Compare December 14, 2023 00:09

byroot merged commit 9517841 into rails:main Dec 14, 2023
4 checks passed

yykamei deleted the add_doc_for_assert_queries branch December 14, 2023 23:58

yahonda mentioned this pull request Dec 17, 2023

Enable Minitest/NonExecutableTestMethod cop #50379

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add doc for `assert_queries` and `assert_no_queries` #50334

Add doc for `assert_queries` and `assert_no_queries` #50334

yykamei commented Dec 12, 2023 •

edited

skipkayhil left a comment

yahonda commented Dec 12, 2023

skipkayhil commented Dec 12, 2023

p8 Dec 12, 2023

yykamei Dec 12, 2023

p8 commented Dec 12, 2023

skipkayhil commented Dec 12, 2023 •

edited

yahonda commented Dec 12, 2023

yykamei commented Dec 12, 2023

yahonda commented Dec 12, 2023 •

edited

yahonda commented Dec 12, 2023

yykamei commented Dec 12, 2023 •

edited

p8 commented Dec 12, 2023

yykamei commented Dec 12, 2023

Add doc for assert_queries and assert_no_queries #50334

Add doc for assert_queries and assert_no_queries #50334

Conversation

yykamei commented Dec 12, 2023 • edited

Motivation / Background

Additional information

Checklist

skipkayhil left a comment

Choose a reason for hiding this comment

yahonda commented Dec 12, 2023

skipkayhil commented Dec 12, 2023

p8 Dec 12, 2023

Choose a reason for hiding this comment

yykamei Dec 12, 2023

Choose a reason for hiding this comment

p8 commented Dec 12, 2023

skipkayhil commented Dec 12, 2023 • edited

yahonda commented Dec 12, 2023

yykamei commented Dec 12, 2023

yahonda commented Dec 12, 2023 • edited

yahonda commented Dec 12, 2023

yykamei commented Dec 12, 2023 • edited

p8 commented Dec 12, 2023

yykamei commented Dec 12, 2023

Add doc for `assert_queries` and `assert_no_queries` #50334

Add doc for `assert_queries` and `assert_no_queries` #50334

yykamei commented Dec 12, 2023 •

edited

skipkayhil commented Dec 12, 2023 •

edited

yahonda commented Dec 12, 2023 •

edited

yykamei commented Dec 12, 2023 •

edited