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

Speed up test suite #11470

Merged
merged 5 commits into from
Jul 30, 2019
Merged

Speed up test suite #11470

merged 5 commits into from
Jul 30, 2019

Conversation

monfresh
Copy link
Contributor

@monfresh monfresh commented Jul 19, 2019

How: Our biggest bottleneck in specs is the VACOLS-related DB
cleaning in database_cleaner.rb. Since only specific tests need to
interact with VACOLS, it makes sense to only perform the DB cleaning
for those tests.

  • Create separate database_cleaner and vacols_database_cleaner
    files that only clean the Postgres or both Postgres and Oracle DB, respectively.
  • Don't automatically require these 2 files in rails_helper. Instead,
    require one or the other as needed in each spec.
  • When the entire suite is run, once it encounters a require statement
    to load database_cleaner or vacols_database_cleaner, it will run the
    DB cleaning in both files for all subsequent tests. To avoid that, we
    additionally specify that the Postgres cleaning should only happen in
    specs that are tagged with :postgres, and the Oracle cleaning in tests
    tagged with :vacols.
  • Move the loading of the Vftypes, Issref, and Actcode VACOLS
    tables out of a before(:all) block and into a separate rake task
    since this only needs to run once for the lifetime of the test DB. We
    don't want to put it in a before(:suite) block because that will slow
    down running individual tests locally.
  • Move other helper methods out of rails_helper into separate files
    for better organization

These changes result in significant time savings locally. I tested with all non-feature specs:

bundle exec rspec --exclude-pattern "**/feature/**/*_spec.rb"

Before these changes, those tests ran in about 18 minutes. After the changes, they run in about 6.5 minutes! The difference in Circle CI, where we have parallelization, is not as drastic, and I'm not sure why.

@va-bot
Copy link
Collaborator

va-bot commented Jul 19, 2019

1 Warning
⚠️ This is a Big PR. Try to break this down if possible.

Generated by 🚫 Danger

@codeclimate
Copy link

codeclimate bot commented Jul 19, 2019

Code Climate has analyzed commit f2d7017 and detected 0 issues on this pull request.

View more on Code Climate.

@monfresh monfresh changed the title Allow model specs to run 4 minutes faster Speed up test suite Jul 22, 2019
@monfresh monfresh force-pushed the mb-rails-helper branch 15 times, most recently from c034422 to 6dd448a Compare July 26, 2019 13:57
- run:
name: RSpec
command: |
mkdir -p ~/test-results/rspec
testfiles=$(circleci tests glob "spec/**/*spec.rb" | circleci tests split --split-by timings)
testfiles=$(circleci tests glob "spec/**/*spec.rb" | circleci tests split --split-by=timings)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this makes a difference, but the documentation includes the = sign.

@monfresh monfresh self-assigned this Jul 26, 2019
require "rails_helper"

RSpec.describe Api::V2::HearingsController, type: :controller do
RSpec.describe Api::V2::HearingsController, :vacols, type: :controller do
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is a lot to change but I would rename it to be postgres_and_vacols to make it more clear

Copy link
Contributor

@anyakhvost anyakhvost left a comment

Choose a reason for hiding this comment

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

One suggestion to rename the variable.

**How**: Our biggest bottleneck in specs is the VACOLS-related DB
cleaning in `database_cleaner.rb`. Since only specific tests need to
interact with VACOLS, it makes sense to only perform the DB cleaning
for those tests.

- Create separate `database_cleaner` and `vacols_database_cleaner`
files that only clean the Postgres and Oracle DB, respectively.
- Don't automatically require these 2 files in `rails_helper`. Instead,
require one or the other as needed in each spec.
- When the entire suite is run, once it encounters a `require` statement
to load `database_cleaner` or `vacols_database_cleaner`, it will run the
DB cleaning in both files for all subsequent tests. To avoid that, we
additionally specify that the Postgres cleaning should only happen in
specs that are tagged with `:postgres`, and the Oracle cleaning in tests
tagged with `:vacols`.
- Move the loading of the `Vftypes`, `Issref`, and `Actcode` VACOLS
tables out of a `before(:all)` block and into a separate rake task
since this only needs to run once for the lifetime of the test DB. We
don't want to put it in a `before(:suite)` block because that will slow
down running individual tests locally.
- Move other helper methods out of `rails_helper` into separate files
for better organization
@monfresh monfresh added the Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master label Jul 30, 2019
@monfresh monfresh merged commit 7e95cc5 into master Jul 30, 2019
monfresh added a commit that referenced this pull request Jul 30, 2019
**Why**: In #11470, we moved the one-time VACOLS setup from a
`before(:all)` block into a separate rake task that runs in Circle CI
before the test suite is run. This PR allows that setup to happen
locally for people setting up the repo for the first time, or those who
might have run `make clean`, which destroys the VACOLS DBs before
creating them again.
va-bot pushed a commit that referenced this pull request Jul 30, 2019
**Why**: In #11470, we moved the one-time VACOLS setup from a
`before(:all)` block into a separate rake task that runs in Circle CI
before the test suite is run. This PR allows that setup to happen
locally for people setting up the repo for the first time, or those who
might have run `make clean`, which destroys the VACOLS DBs before
creating them again.
@pkarman pkarman deleted the mb-rails-helper branch August 23, 2019 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-to-Merge This PR is ready to be merged and will be picked up by va-bot to automatically merge to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants