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

Extract methods assert_queries and assert_no_queries #42465

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

ricardotk002
Copy link
Contributor

Summary

Inspired by https://github.com/rails/rails/pull/40842/files#r650377037

Both methods are defined in multiple parts of the framework. It would
be useful to put them in a proper place, so that repetition is
avoided.

I chose the implementation from ActiveRecord because it's a bit more
complete with the SQLCounter class, and also because other parts
depend on it.

@pixeltrix
Copy link
Contributor

Can we move this to ActiveRecord::Testing::QueryAssertions - we don't want to pollute the Active Support code with references to Active Record, thanks. Other than that looks good 👍🏻

Both methods are defined in multiple parts of the framework. It would
be useful to put them in a proper place, so that repetition is
avoided.

I chose the implementation from `ActiveRecord` because it's a bit more
complete with the `SQLCounter` class, and also because other parts
depend on it.
@ricardotk002
Copy link
Contributor Author

@pixeltrix Makes sense. I pushed new changes to address that.

@pixeltrix pixeltrix merged commit 26c00cb into rails:main Jun 14, 2021
@pixeltrix
Copy link
Contributor

@ricardotk002 thanks for the quick turnaround on the changes 👍🏻

@ricardotk002 ricardotk002 deleted the extract-assert-queries branch June 15, 2021 03:53
@rafaelfranca
Copy link
Member

rafaelfranca commented Jun 17, 2021

This is exposing those helpers to applications. We removed them years ago. Even though this is adding as private API, I think it would be better to not expose this on lib. Is that code duplication that bad?

@ricardotk002
Copy link
Contributor Author

I agree that it probably shouldn't be exposed to the public. Still, I think there's a necessity for verifying the absence of N+1 queries in some parts of the framework (ActiveStorage, ActionText, etc); plus the implementation for assert_queries doesn't seem trivial, in most cases you'll find yourself copying/pasting it from somewhere else. I'm happy to follow another approach to keep it as a private API, if needed.

@rafaelfranca
Copy link
Member

I think we should wait this duplication become a pain before trying to extract. I'm fine with copying parts of the implementation for all the places that need them. At this points this is already in all frameworks that need it already.

@pixeltrix
Copy link
Contributor

I think we should wait this duplication become a pain before trying to extract. I'm fine with copying parts of the implementation for all the places that need them. At this points this is already in all frameworks that need it already.

The reason I suggested extracting it was that it was copied as part of #40842 so it seemed like a good point to do it with it being in three places but it's not a hill I'm going to die on if people feel strongly the other way 🙃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants