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

DB-query integration tests #18403

Merged
merged 3 commits into from Oct 17, 2017
Merged

DB-query integration tests #18403

merged 3 commits into from Oct 17, 2017

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Oct 16, 2017

This PR adds a few integration tests on the number of ActiveRecord DB queries generated by specific route actions. The implementation in the CaptureQueries module uses the ActiveSupport::Notifications subscription API to capture all ActiveRecord queries generated within a block, and assert an expected count.

This will allow us to more easily track (and automatically prevent regressions on) the number of database queries performed by high-traffic routes.

I've added a few basic tests to get started:

  • milestone POST (passing, signed-in user) expects 11 queries
  • milestone POST (failing, signed-in user) expects 11 queries
  • user_progress GET (signed-in user, with existing user_level) expects 7 queries
  • script_levels #show (signed-in user with existing user_level) expects 22 (!) queries

These query counts are not ideal, and the numbers will be reduced in an optimization PR that will come after these integration tests.

I've refactored the existing CachingTest tests to use this implementation instead of calling no_database (which stubs ActiveRecord::Base#connection), since I think it's a cleaner approach.

This PR also renames an API route-path fragment from :script_name to :script- it turns out that :script_name is a reserved word in Rails path helper, and its use was preventing the new integration tests from working properly.

:script_name is a reserved word in Rails-route path helpers.
Prevent DB-query regressions on high-scale routes.
Copy link

@jopolsky jopolsky left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@wjordan wjordan merged commit 14d60ea into staging Oct 17, 2017
@wjordan wjordan deleted the db-query-test branch October 17, 2017 00:30
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

2 participants