These changes add handling of an additional parameter "--parallel-test" #1527
Conversation
…t" that takes a number as input. This number will then be used to limit execution of examples to the specified number of parallel threads. To accomplish this the ExampleGroups each are started in a sub-thread which can then start up to the specified number of example threads running in parallel. A global maximum is used to prevent going over the specified number and execution of additional threads will wait until an available thread is free for cases where the maximum number of parallel threads are in use. Additionally, these changes ensure that Before / After Suite and Before / After All calls are executed only once per expected grouping, but that Before / After Each calls are executed before and after each example as part of the parallel thread. The advantage of this change is that it fully supports all platforms supported by rspec (unlike parallel_tests gem which does not work correctly on all Windows systems) and the output remains serially reported so you avoid the need to stitch together test reports at the completion of testing (as can be required with solutions like parallel_tests and prspec gems where parallel execution is actually split between multiple instances of rspec).
| @@ -1,4 +1,6 @@ | |||
| $_rspec_core_load_started_at = Time.now | |||
| require 'thread' | |||
There was a problem hiding this comment.
If possible, this needs delaying until someone actively uses the feature, reason being is we try very hard not to pollute a users code base with std library stuff, which this would.
There was a problem hiding this comment.
I added this here because the threaded execution, in this proposed implementation, becomes the default, but with a maximum threadcount of 1. I can certainly move it to elsewhere, but it might then need to go in the runner.rb file instead which is still pretty core.
There was a problem hiding this comment.
I mean I'd like it contextually loaded when someone uses the feature, if they don't use the feature I really don't want this required.
There was a problem hiding this comment.
I'll have to create a completely separate execution path to support contextual loading which will lead to greater maintenance cost since there will be what is effectively a run and run_parallel method instead of handling everything in the run method. I'll commit the changes in as surgical a fix as possible in case you decide that the approach is not as good.
|
Good start, I've left some feedback, you'll notice this completely breaks the travis build, that needs fixing. |
|
Thanks for all the great feedback. As to the travis build, is there any way to get better information on why it failed? I tried looking around, but didn't see any error logging that could explain why it failed (looks like a timeout) |
The time out is why it failed. The test suite reached a point where it didn't emit any output for 10 minutes and when that happens travis kills the process and fails the build as it usually indicates dead lock (or something similar). |
|
I definitely want to review this at some point but probably won't get around to it for a bit. |
| filtered_examples.map do |example| | ||
| example.succeeded? | ||
| end.all? | ||
| end |
There was a problem hiding this comment.
all? can take a block. Since succeeded? is simply an attr and has no side-effects, why not leverage that:
filtered_examples.all?{ |example| example.succeeded? }Removing file-based mutex and replacing with Mutex class
Fixing multiline blocks to use 'do' instead of '{'
Removing extra comments
Switched environment to POSIX git commits
Fixed spacing on default method values '=' became ' = '
Removed changes to Example class in favour of using metadata for example success
Switched to .all? {...} from .map {...}.all?
Corrected method parameter reference from [int] to [Integer]
…on Pull Request #1527 feedback Removes need to pass 'num_threads' directly from options into execution methods and then uses config values to specify the thread max limit
… high up the thread chain resulting in less optimal thread utilization
| # Method will check global utilization of threads and if that number is | ||
| # at or over the allocated maximum it will wait until a thread is available | ||
| def wait_for_available_thread | ||
| # wait for available thread if we've reached our global limit |
…through the calls fixing line endings to use POSIX style (hopefully) Removing inline comments from code Reverted runner.rb self.run method back to original state Modified how the result of testing is collected in runner.rb run_specs_parallel so now it will only return an error code if there is an exception in executing the example or its before or after blocks
|
Thanks again for the feedback. https://github.com/bicarbon8/rspec-core/commit/708e869b3d019901be8cc0fbcb9b680b60add27c attempts to address the latest round of comments with the exception of changing the Thread.start { ... } multiline usage to a 'do ... end' block. I cannot change this due to the design of the Thread object which returns unless the curly braces are used. Hopefully this is ok for the two instances where this is used. |
|
Oh, you'll need to separate the lines so the blocks go to the right method. |
|
Hi @JonRowe , I'm not sure I get what you mean there, so if you have a second to explain a bit more I'd appreciate it. Also, I just realized that I haven't removed one of the globals so I still need to take care of that. EDIT |
…ow passes the variable by reference to the areas where needed.
…d of build machine
…ing in method call and undefined variable in cucumber test
…entation coverage
|
Looks like I finally managed to get some passing builds. For those that didn't pass I'm seeing the following error: It looks like there might be a problem with the version of Ruby's understanding of how I'm extracting the number of seconds the tests took. I would appreciate some help in finding a more cross-version compatible way of doing the same thing: def get_seconds(output_str)
return output_str[/Finished in (?<match>.*) second/, "match"]
endThanks in advance for your help! |
|
Ruby 1.8.7 does not support named matches. You'll need to refer to the older docs for proper usage: http://ruby-doc.org/core-1.8.7/String.html#method-i-5B-5D Looks like you have to pull the match position out of the matcher data. output_str[/Finished in (.*) second/, 1] |
|
thanks for the help @cupakromer I thought that would be the last bit to get a passing build, but it appears there might still be a problem with jruby-18mode as there was a timeout in the build 😦 |
|
I'll restart it for you, see if it was transitory, but we do need this work on JRuby as JRuby and RBX will benefit the most. |
| before_context_ivars.clear | ||
| reporter.example_group_finished(self) | ||
| end | ||
| end |
There was a problem hiding this comment.
This looks like it has a lot in common with run. We should look for a way to extract the common logic -- otherwise it'll add a maintenance burden to keep them in sync. Same with run_examples_parallel.
Actually, I wonder if we can come up with an injectable run strategy abstraction that can have parallel and serial implementations?
changed "examplegroup" variable to be "example_group" modified wait_for_completion methods in example_thread_runner.rb and example_group_thread_runner.rb to join to the sub-threads instead of polling modified wait_for_available_thead method in example_thread_runner.rb to only sleep for 0.1 seconds instead of 1 second to reduce wait overhead while polling for an available thread to run within changed doublequotes to singlequotes in "spec/command_line/parallel_spec.rb" to avoid the need for escaping
|
Sorry I'm so late on this, looks like there has already been a lot of feedback. I feel like this is "tacking on" the feature, rather than integrating it properly with RSpec, and that we need to take a radically different approach to do this in a way that we can support. Namely, we need to make evaluation a first-class concept (this type of thing: https://github.com/xaviershay/xspec/blob/master/lib/xspec/evaluators.rb ), hopefully expose that publicly, then start with parallel running in a gem to test it out before bringing it in to core. There are also a large number of things in RSpec known to be Thread-unsafe (particularly in mock setup/teardown) that need to be audited and thoroughly tested along with any parallel runner. We also need to think about thread setup: this won't work with acceptance tests, for example, because each thread needs its own database (and possibly other dependencies as well). Even if we don't tackle this right away, we need to not paint ourselves into a corner so that it can potentially be solved later. I'm interested in exploring this further post 3.0 release. |
|
I agree with @xaviershay -- I want this feature but only if we do it properly, which is going to be a lot more work than what's here, and will need to involve a full audit to deal with any thread safety issues. Closing. |
|
While obviously not the result I would have hoped for, I was able to take inspiration from @xaviershay and create a rspec-parallel gem that bolts itself to rspec in the same way that these changes did. I've managed to get the 2.14 version of the gem running successfully and have the basics in place for the 3.0.0.Beta2 version, but haven't yet tested it. Have a look if you're interested: Thanks again for all the good comments and feedback! |
These changes add handling of an additional parameter "--parallel-test" that takes a number as input. This number will then be used to limit execution of examples to the specified number of parallel threads. To accomplish this the ExampleGroups each are started in a sub-thread which can then start up to the specified number of example threads running in parallel. A global maximum is used to prevent going over the specified number and execution of additional threads will wait until an available thread is free for cases where the maximum number of parallel threads are in use.
Additionally, these changes ensure that Before / After Suite and Before / After All calls are executed only once per expected grouping, but that Before / After Each calls are executed before and after each example as part of the parallel thread.
The advantage of this change is that it fully supports all platforms supported by rspec (unlike parallel_tests gem which does not work correctly on all Windows systems) and the output remains serially reported so you avoid the need to stitch together test reports at the completion of testing (as can be required with solutions like parallel_tests and prspec gems where parallel execution is actually split between multiple instances of rspec).
This is also an attempt to partially cover the request at: #1254