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

[Rails 7] Get rid of db-query-matchers gem #7232

Conversation

alejandroperea
Copy link
Contributor

@alejandroperea alejandroperea commented Dec 18, 2021

I've seen in this issue #7196 that db-query-matchers gem is incompatible with Rails 7 and looks no longer maintained.

I've introduced a custom Rspec matcher that subscribes to sql.active_record event and checks if the passed query matches (or not) with the executed ones.

@alejandroperea alejandroperea force-pushed the 7196-get-rid-of-db-query-matchers branch from ab242b6 to e6df0bc Compare December 18, 2021 10:30
@alejandroperea alejandroperea mentioned this pull request Dec 18, 2021
@varyonic
Copy link
Contributor

Interesting. This dependency was introduced by #5811 The latest publisher of db-query-matchers is still active using Ruby on GitHub so I think too soon to say abandoned.

@rafaelfranca
Copy link
Contributor

rafaelfranca commented Jan 19, 2022

Is this dependency worth though? It is only used in two tests, which can be easily rewritten to check the same thing. For example we could subscribe to the active_record.sql notification in those tests and check if the queries we don't want to be there are not there.

@javierjulio
Copy link
Member

If we can do the same thing through ActiveRecord notification subscription, I'd be ok with that and would approve. Ideally, we'd still want the database query check for these test cases.

@rafaelfranca
Copy link
Contributor

If we can do the same thing through ActiveRecord notification subscription

That is how db-query-matchers is implemented.

I agree we should keep the SQL check.

@alejandroperea do you want to improve the test to use the ActiveSupport::Notification.subscribe('sql.active_record')?

@alejandroperea
Copy link
Contributor Author

@rafaelfranca Sure! I'll take a look this weekend 👍

@alejandroperea alejandroperea force-pushed the 7196-get-rid-of-db-query-matchers branch 2 times, most recently from 2ae618b to f7cdccc Compare January 22, 2022 11:03
@alejandroperea
Copy link
Contributor Author

@rafaelfranca Done! It is the first time I use ActiveSupport::Notifications (super cool, btw), I hope I did it properly 🙏

I've added an Rspec matcher for doing this, just in case we want to use it in another place in the future.

@alejandroperea alejandroperea force-pushed the 7196-get-rid-of-db-query-matchers branch from f7cdccc to da69461 Compare January 22, 2022 16:46
Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Just a very small feedback that you're free to address but I'm good with the current version too. Thanks so much!

spec/spec_helper.rb Outdated Show resolved Hide resolved
@alejandroperea alejandroperea force-pushed the 7196-get-rid-of-db-query-matchers branch from da69461 to 0c08626 Compare January 23, 2022 09:17
@alejandroperea
Copy link
Contributor Author

I agree @deivid-rodriguez ! Change applied 👍

Thanks for the review! :)

@tagliala tagliala mentioned this pull request Jan 23, 2022
11 tasks
@deivid-rodriguez deivid-rodriguez merged commit 056edf6 into activeadmin:master Jan 23, 2022
@deivid-rodriguez
Copy link
Member

Thanks so much!!

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

Successfully merging this pull request may close these issues.

None yet

5 participants