-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add Parallel spec rake tests, based on category with better output. #2671
Conversation
The tests are parallelized by category, which makes for more sensible output. The output is also all printed to the screen only when those tests are finished. So there is no confusing overlapping output. All unit tests are run as one category (a unit test is defined as anything that isn't a feature test). The feature tests are categorized by top level subfolders under `/spec/feature. All specs not in a subfolder will be part of the `other` category. The test categories can also be run seperately, by seperate docker containers. They do not all have to be run at once. `rake spec:parallel:setup` - sets up the DBs needed for parallel testing `rake spec:parallel` - runs all the different spec categories in parallel `rake spec:parallel:CATEGORY` - runs specs from one category, but with parallel configurations set `rake spec:CATEGORY` - runs specs from one category` - Tested locally - Remove parallel-tests gem - add documentation - figure out why one reader test is consistently failing when parallelized - integrate into the Jenkins job
@@ -78,7 +78,7 @@ test: | |||
<% if ENV['POSTGRES_PASSWORD'] %> | |||
password: <%= ENV['POSTGRES_PASSWORD'] %> | |||
<% end %> | |||
database: caseflow_certification_test<%= ENV['TEST_ENV_NUMBER'] %> | |||
database: caseflow_certification_test<%= ENV['TEST_SUBCATEGORY'] %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we update the Redis config too? It looks like they'd all be hitting the same redis namespace which could cause clashes (an issue with the existing parallel tests too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am updating the cache config here: https://github.com/department-of-veterans-affairs/caseflow/pull/2671/files#diff-639e7393aa097999e816b53faa94bab4L11
our tests don't actually run against redis. We are using Rails file cache. Which is sort of a stubbed cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, I just noticed that for the first time. I wonder why we use file_store for tests? Couldn't we use memstore to both speed up the tests & more closely mirror production? of course, this would be outside the scope of this PR but just wondering your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah i agree with you on both points. It's worth considering upgrading our test caching mechanism as well as it being outside the scope of this PR.
Also I found this fun little guy:
https://github.com/department-of-veterans-affairs/caseflow-commons/pull/108/files
lib/tasks/spec.rake
Outdated
|
||
namespace :spec do | ||
feature_spec_folders = Dir.entries("./spec/feature").select do | ||
|file| !['.', '..'].include?(file) && File.directory?(File.join("./spec/feature", file)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the |file|
to the line above to be more idiomatic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how this happened. I'm very embarrassed.
lib/tasks/spec.rake
Outdated
task spec_name do | ||
envs = "export TEST_SUBCATEGORY=#{spec_name};" | ||
|
||
run_command_and_print_output("#{envs} rake spec:#{spec_name}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't the rake spec:reader
itself set it's own TEST_SUBCATEGORY
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally did that but figured it was more stable to set the env variable out of process. I could move it back in.
lib/tasks/spec.rake
Outdated
require 'rspec/core/rake_task' | ||
|
||
def run_command_and_print_output(command) | ||
output_stream = open("|#{command}", "r") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You previously created a ShellCommand
class for this sort of thing. Do we want to use that here to standardize? It looks like its run
method should be usable here, but if not, can we move this logic there so it can be reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gooooood catch. I will move this there.
lib/tasks/spec.rake
Outdated
output = "" | ||
output_stream.each { |line| output << line; print "."; $stdout.flush; } | ||
puts output | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add comments explaining how this method works? I don't quite grok is. Does the output from the open
command immediately return to output_stream
as an array?
lib/tasks/spec.rake
Outdated
|file| !['.', '..'].include?(file) && File.directory?(File.join("./spec/feature", file)) | ||
end | ||
|
||
spec_names = feature_spec_folders + ["other", "unit"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this and feature_spec_folders
be constants?
@@ -9,6 +9,6 @@ if ENV["RAILS_ENV"] == "test" | |||
add_filter "lib/tasks" | |||
end | |||
|
|||
SimpleCov.command_name "Test Run" + (ENV["TEST_ENV_NUMBER"] || "") | |||
SimpleCov.command_name ENV["TEST_SUBCATEGORY"] || "all" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you extend your test plan section. Did you run the new test setup and verify simplecov output? Did you verify the output shows up in travis? etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not intending to set this up in travis. Just jenkins.
I did veryify simplecov is working, but I should write that down too.
@@ -8,7 +8,7 @@ | |||
# and recreated between test runs. Don't rely on the data there! | |||
config.cache_classes = true | |||
|
|||
cache_dir = Rails.root.join("tmp", "cache", "paralleltests#{ENV['TEST_ENV_NUMBER']}") | |||
cache_dir = Rails.root.join("tmp", "cache", "test_#{ENV['TEST_SUBCATEGORY']}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a high level can you describe how this setup works? Are these all running against the same instance of the application? It looks like all the parallel tests would hit on the same port. Is this problematic? Does that code need to be updated too possibly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are different files where the Rails file_store cache is stored. I'm just configuring the tests to use different files for each test category here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great in general, thanks for doing this! +1 to cleaning up & removing the parallel tests gem 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple things worth fixing, but I don't want to block this PR since you're going to run into merge conflict hell the longer this stays open 👍 🙏
fire in the hole 🔥 |
The tests are parallelized by category, which makes for more sensible
output. The output is also all printed to the screen only when those
tests are finished. So there is no confusing overlapping output.
All unit tests are run as one category (a unit test is defined as
anything that isn't a feature test).
The feature tests are categorized by top level subfolders under
/spec/feature
. All specs not in a subfolder will be part of theother
category.The test categories can also be run seperately,
by seperate docker containers. They do not all have to be run at once.
Commands added
rake spec:parallel:setup
- sets up the DBs needed for parallel testingrake spec:parallel
- runs all the different spec categories in parallelrake spec:parallel:CATEGORY
- runs specs from one category, but with parallel configurations setrake spec:CATEGORY
- runs specs from one category`Test plan
Notes
connects #2458