8000 [Rails 7] Get rid of db-query-matchers gem by alejandroperea · Pull Request #7232 · activeadmin/activeadmin · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 23, 2022
Merged

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

merged 1 commit into from
Jan 23, 2022

Conversation

alejandroperea
Copy link
Contributor
@alejandroperea alejandroperea commented Dec 18, 2021 •
edited
Loading

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.
8000

@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
Closed
@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 •
edited
Loading

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
javierjulio
javierjulio approved these changes Jan 22, 2022
View reviewed changes
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!

deivid-rodriguez
deivid-rodriguez approved these changes Jan 23, 2022
View reviewed changes
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!

@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
Merged
11 tasks
@deivid-rodriguez deivid-rodriguez merged commit 056edf6 into activeadmin:master Jan 23, 2022
@deivid-rodriguez
Copy link
Member

Thanks so much!!

@alejandroperea alejandroperea deleted the 7196-get-rid-of-db-query-matchers branch January 25, 2022 14:33
@deivid-rodriguez deivid-rodriguez mentioned this pull request Jan 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierjulio javierjulio javierjulio approved these changes

@deivid-rodriguez deivid-rodriguez deivid-rodriguez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alejandroperea @varyonic @rafaelfranca @javierjulio @deivid-rodriguez
0