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

366 parallel tests gem #3169

Closed
wants to merge 24 commits into from
Closed

366 parallel tests gem #3169

wants to merge 24 commits into from

Conversation

kreek
Copy link
Contributor

@kreek kreek commented Jul 24, 2019

Description of change

Adds the parallel_tests gem to locally take advantage of multiple cores.

Setup

Setup requires updating the test database configuration to have a variable number of databases (if the env var is missing it does not interfere with regular spec runs):

test:
  database: yourproject_test<%= ENV['TEST_ENV_NUMBER'] %>

Then you can create the additional databases by running:

bundle exec rake parallel:create
bundle exec rake parallel:prepare

Run

To run the rspec tests in parallel:

bundle exec rake parallel:spec

Some of our specs share state and need to refactored so they're properly set up and torn down and the sprint ended before they could all be refactored so I'm leaving this PR in draft mode and adding a ticket to the backlog. The list of specs is below. They share in common either creating/deleting files or running background jobs. One fix was to wrap a test helper with a mutex.syncronize so that access to files is locked per thread. Other tests probably do too much in that they are more integration tests than unit tests and we should mock and stub behavior not directly related to the subject's behavior.

spec/controllers/v0/vic/profile_photo_attachments_controller_spec.rb
spec/jobs/transactional_email_analytics_job_spec.rb
spec/jobs/vic/submission_job_spec.rb
spec/jobs/vic/submission_job_spec.rb
spec/lib/vic/service_spec.rb
spec/request/preneeds/burial_forms_request_spec.rb
spec/request/swagger_spec.rb
spec/uploaders/evss_claim_document_uploader_spec.rb

Testing done

locals specs

Testing planned

jenkins run specs

Acceptance Criteria (Definition of Done)

Unique to this PR

  • Reduces the spec suite's runtime (by roughly half depending on the machine, for myself from 5m38s to 3m02s)

Applies to all PRs

  • Appropriate logging
  • Swagger docs have been updated, if applicable
  • Provide link to originating GitHub issue, or connected to it via ZenHub
  • Does not contain any sensitive information (i.e. PII/credentials/internal URLs/etc., in logging, hardcoded, or in specs)
  • Provide which alerts would indicate a problem with this functionality (if applicable)

@va-vfs-bot va-vfs-bot temporarily deployed to 366-gem-parallel-tests/master July 24, 2019 13:19 Inactive
@annaswims
Copy link
Contributor

bundle exec rake parallel:spec doesn't seem to be running the tests in modules

@@ -18,6 +18,7 @@
require 'support/okta_users_helpers'
require 'support/poa_stub'
require 'pundit/rspec'
require 'test_prof/recipes/rspec/let_it_be'
Copy link
Contributor

Choose a reason for hiding this comment

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

Todo: figure out why this line causes trouble when running plain ol' rpsec

bundle exec rake spec spec/controllers/v0/preneeds/preneed_attachments_controller_spec.rb
/Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/test-prof-0.10.0/lib/test_prof/ext/active_record_refind.rb:7:in `<module:ActiveRecordRefind>': uninitialized constant TestProf::Ext::ActiveRecordRefind::ActiveRecord (NameError)
Did you mean?  TestProf::Ext::ActiveRecordRefind
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/test-prof-0.10.0/lib/test_prof/ext/active_record_refind.rb:6:in `<module:Ext>'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/test-prof-0.10.0/lib/test_prof/ext/active_record_refind.rb:4:in `<module:TestProf>'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/test-prof-0.10.0/lib/test_prof/ext/active_record_refind.rb:3:in `<top (required)>'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/test-prof-0.10.0/lib/test_prof/recipes/rspec/let_it_be.rb:130:in `require'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/test-prof-0.10.0/lib/test_prof/recipes/rspec/let_it_be.rb:130:in `<top (required)>'
	from /Users/annacarey/code/vets-api/spec/spec_helper.rb:21:in `require'
	from /Users/annacarey/code/vets-api/spec/spec_helper.rb:21:in `<top (required)>'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `require'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `block in requires='
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `each'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration.rb:1394:in `requires='
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:112:in `block in process_options_into'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:111:in `each'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:111:in `process_options_into'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/configuration_options.rb:21:in `configure'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:99:in `setup'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:86:in `run'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:71:in `run'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/lib/rspec/core/runner.rb:45:in `invoke'
	from /Users/annacarey/.rbenv/versions/2.4.5/lib/ruby/gems/2.4.0/gems/rspec-core-3.5.4/exe/rspec:4:in `<main>'

Copy link
Contributor

@omgitsbillryan omgitsbillryan left a comment

Choose a reason for hiding this comment

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

I don't know all that much about this gem, but my only concern is the amount of complexity it adds. This complexity seems warranted to me for speeding up CI 👍 . For local development however, it's more typical to run individual tests. Thoughts?


# environmen variables get stripped when running parallel tests so we set the db here
test_database_url: <%= if ENV['CI'] || ENV['TEST_ENV_NUMBER']
"postgis://#{ENV['POSTGRES_USER'] || 'postgres'}:#{ENV['POSTGRES_PASSWORD'] || 'password'}@#{ENV['POSTGRES_HOST'] || 'postgres'}:#{ENV['POSTGRES_PORT'] || '5432'}/#{ENV['POSTGRES_DATABASE]'] || 'vets_api_test'}#{ENV['TEST_ENV_NUMBER']}?pool=4"
Copy link
Contributor

Choose a reason for hiding this comment

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

😱

I sat staring at this for 5 minutes. It's basically generating the db url dynamically based on which ENV vars you have set and replacing them with known default values?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know it's bad and I hate it too, but I was struggling big time to make anything work.
Prior to this PR, the value of test_database_url in config/settings.yml was overridden by an environment variable set in docker (which I just learned was a thing from the config gem). But that value seems to be strippped when I run the rake test to run parallel tests.

Totally open to suggestions on what is less awkward.

@johnpaulashenfelter
Copy link
Contributor

@annaswims @omgitsbillryan @kreek Should we close this? Its been open a while.

@johnpaulashenfelter johnpaulashenfelter added the CLEANUP Issues, PRs, and such that need cleanup label Oct 23, 2019
@omgitsbillryan
Copy link
Contributor

🤷‍♂ My understanding is that it's a bit of added complexity for quicker spec runs. So long as this is only for use in CI (which it looks to be), seems like it could be a nice win. Seems like a cost benefit of, "how much time this shaved off rspec runs" V.S. "complexity added to our CI".

@annaswims
Copy link
Contributor

I'm going to abandon this (at least for now). I fixed one bug in simplecov simplecov-ruby/simplecov#746 only to encounter another.

If I run bundle exec rake parallel:spec_with_codeclimate_coverage (the way to run all of the tests, including modules while it's a WIP) the coverage report from one of the tests runs is not included in the total for simplecov.

@annaswims annaswims closed this Oct 25, 2019
@johnpaulashenfelter johnpaulashenfelter deleted the 366-gem-parallel-tests branch March 6, 2020 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLEANUP Issues, PRs, and such that need cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants