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

Parallel support #1357

Closed
wants to merge 39 commits into from
Closed

Parallel support #1357

wants to merge 39 commits into from

Conversation

boaty82
Copy link
Contributor

@boaty82 boaty82 commented Apr 25, 2018

I’ve previously used another implementation to deliver parallel execution of cucumber tests, however it basically wrapped up the creation and execution of Runtime’s, followed by the need to merge the multiple JSON/HTML etc reports that followed.

I reviewed the current implementation and also took note of comments in Support concurrent executions of scenarios also I perused all tickets related to running in parallel cukes for other insights

Main changes

Added --threads argument to runtime options

Allows users to specify the (max) number of threads to be used to run the tests

The actual number of threads used will be the smaller of the given --threads value and the number of features to be executed

Tests are placed into a Queue and consumed by each thread when needed, resulting in a less deterministic order of Features being run

Ability to still run tests synchronously

There may be situations where certain Features/Scenarios need to be run in a synchronized/serial manner. To this end features/scenarios will be scanned for usages of a tag beginning with @synchronized (case insensitive) and will run these Feature files separately, prior to any Feature files not found to have this tag. Buckets of synchronized Feature files can also be created by using any suffix e.g. @synchronized-1 or @synchronized-bob

New formatter introduced TimelineFormatter

Which produces reports using vsjis.org timeline to highlight which feature was run on which Thread and when.
Note: the resources (js, html etc) are currently within cucumber-core if the PR is accepted then I’d imagine these would have to be moved out into its own project, similarly to cucumber-html.
timeline
sync_and_parallel_timeline.zip

Glue / RuntimeGlue

This was the main problem in that it serves 2 purposes.

  1. Glue Loader/Builder
  2. Glue Context

Before tests are executed the glue is loaded (Backend.loadGlue) and then during the test run the step defs etc held in cache are modified, and hence the glue instance cannot be shared.

Also it appears that JavaBackend held onto the glue reference to be used during buildWorld and disposeWorld calls - to support Java8 Lambdas

Ideally this class/interface would be broken down into more granular APIs to reduce this issue, but this is quite a big change.

To reduce the number of breaking changes I went with the premise that the Glue initially loaded is a template and it is cloned per thread during execution, which resulted in very few changes to be made.

But there were 2 changes - Backend.buildWorld and Backend.disposeWorld - whereby the glue is now passed in as an argument, so it is the glue “clone” applicable to the running thread. This means any other implementations of Backend outside of this repository need updating.

Unfortunately, I could not think of another way around this, but happy to discuss further.

Others Issues encountered / rectified

  • Stats
    Was not thread safe

  • UndefinedStepsTracker
    Was not thread safe

  • EventBus thread safety
    registerHandlerFor is not thread safe, originally I updated it to be but couldn’t find any sign of sharing instances - so I reverted the change

  • UnreportedStepExecutor implementation
    Extracted this out of Runner and into its own class
    Glue is injected, but as there was no real usage of this functionality I couldn’t tell if the Glue “template” was OK to be injected or not - I assume it is but have left a TODO comment for review/discussion

  • LocalizedXStreams not thread safe
    Internal Map to hold cache of LocalizedXStream was not threadsafe

  • Formatters output being incorrect as not thread safe
    I have updated all Formatter implementations located in cucumber-jvm, specifically
    AndroidInstrumentationReporter, HTMLFormatter, JSONFormatter, JUnitFormatter, PrettyFormatter, RerunFormatter, TestNGFormatter, UsageFormatter
    to be thread safe
    Also added a note to the javadocs.
    Note any impl's in other repositories still need reviewing/updating

  • TestSourcesModel updated this to be thread safe in the event any instances are shared (none in cucumber-jvm repo)

  • Update ObjectFactory implementations to be thread safe
    Specifically
    DefaultJavaObjectFactory, GuiceFactory, NeedleFactory, OpenEJBObjectFactory, OsgiObjectFactoryBase, PicoFactory, WeldFactory all were minor changes so I felt new tests were not needed
    SpringFactory appeared to be threadsafe already so I added some tests to SpringFactoryTest and it proved to be the case
    Note any impl's in other repositories still need reviewing/updating

boaty82 added 30 commits April 19, 2018 10:35
TODO: update Formatter impls etc to be thread safe
no new tests were worthwhile
a lot of methods unchanged, but moved out of deleted static class
not needed as test nodes are transferred into main dom
ideally initial glue would be immutable and would then be copied per thread.  this is what i've tried, also tried to reduce the number of big/breaking changes - but would've like to split RuntimeGlue up a bit

JavaBackend seems to be a bit of a culprit, for Java8 lambda support.  Again, I've tried to not introduce too much refactoring/breaking changes, but as a minimum I've had to pass the glue reference into build/dispose world rather than holding a reference to it within the backend.  This is obviously a breaking change to other implementations of Backend outside of this repo, so they should be updated accordingly.
@synchronized
@synchronized-1

becomes 2 groups of tests run first followed by the remaining

supported by --threads
@boaty82
Copy link
Contributor Author

boaty82 commented May 18, 2018

I hacked together a way to get JUnit to run in parallel. Just to get a feel for stuff it needs: https://github.com/cucumber/cucumber-jvm/tree/junit-parallel

OK, I will take a look

I used your idea about event buffering and flushing only after the run.

😃 good to know my thoughts were on the right lines

Not just for the formatters but also because JUnit's event listener is also listening to the bus. But it should only listen to the bus of one specific runner and it does need those events in real time

I was thinking of creating a decorator for the original EventBus per running thread which can store the event(s), until they should be dispatched - they could simply then dispatch/flush them onto the original EventBus - just need to be careful and ensure synchronization is done correctly to avoid the issue with the EventBus receiving events from multiple sources and thus the events being out of sequence.

It might be a good idea to start over from scratch.
I think it would better to make it possible to run pickles in parallel through JUnit or TestNG before we make this possible through the command line and start adding extra features like @synchronized or command line options like --parallel.

OK sounds like a reasonable plan of attack. I still believe these features should be added, as it's always good to test a system with many threads (as a replacements for users/customers) to ensure performance/thread/state safety

I also think it will be okay to leave out the performance improvements to Glue for now. Don't throw the code away though.

Don't worry I won't I'll just stash it for now

@mpkorstanje
Copy link
Contributor

I was thinking of creating a decorator for the original EventBus per running thread which can store the event(s), until they should be dispatched - they could simply then dispatch/flush them onto the original EventBus - just need to be careful and ensure synchronization is done correctly to avoid the issue with the EventBus receiving events from multiple sources and thus the events being out of sequence.

Sounds about right.

We can synchronize on send and add a synchronized sendAll method.

@mpkorstanje
Copy link
Contributor

OK sounds like a reasonable plan of attack. I still believe these features should be added, as it's always good to test a system with many threads (as a replacements for users/customers) to ensure performance/thread/state safety

Cucumber isn't a performance or load testing tool though. Scenarios can't be repeated and the rate at which tests are executed is unpredictable and uncontrollable. Using it as such would give you meaningless results.

The only good use case for parallel execution is to speed up tests that spend a significant amount of time waiting for things to happen (webdriver, io, ect). Calling glue is inherently slow so cucumber won't ever be able to output any significant volume and the message bus and attached listeners are going bottleneck any serious amount of parallel work.

@boaty82
Copy link
Contributor Author

boaty82 commented May 18, 2018

Sorry performance was a bad choice of a word, and I meant it from a test/CI execution point of view rather than a production system test

Completely agree, this isn’t what cucumber is for - nor should it be.

But certainly it helps being able to run multiple threads so the total time of the run is reduced. It also helps to ensure that tests are not interfering with each other by them sometimes running at the same time

And it has in the past helped to identify some very poorly written production code by a team (DateFormat being a static final, when it isn’t threadsafe)

And I’m not concerned about any potential bottleneck that the events/eventbus may introduce - its more about getting more throughput of the tests by running them in parallel - and also the potential for the test run order to be less deterministic

@mpkorstanje
Copy link
Contributor

mpkorstanje commented May 18, 2018

Ah in that case I think your needs should be met. Surefire has a large suite of configuration options for running in parallel. You can configure the number of forks per build, the number of threads per fork per cpu, ect. Or even set it to unlimited.

It'd still be nice to have --threads eventually though but most people don't use command line runner.

@boaty82
Copy link
Contributor Author

boaty82 commented May 19, 2018

Not quite. I still have a use case for --threads argument, in which I need tests from within a running application so we can get a handle on the spring context, and use Cucumbers Runner, as the application (not mine, but a commercial platform) uses ant & is quite monolithic and takes ~5-6 mins to startup - plus we need to execute multiple test sets so don't really want to wait for startup each time

Also most of my other micro services use gradle, not maven, and I generally call the Main class

@mpkorstanje
Copy link
Contributor

That is a reasonable use case. Let's discuss it in more depth when we get to it. We may learn more things along the way.

@mpkorstanje
Copy link
Contributor

Would you be able to work off #1367?

I've pushed some refactoring to this branch that will make it easier to implement parallel support aswell as some other refactoring that are happening. I don't want to merge it just yet because we've just released 3.0.0. Best to give people some time to find bugs before everything is changed again.

@boaty82
Copy link
Contributor Author

boaty82 commented May 22, 2018

If I can, I will. I’ll try to pull it down and start working on it in the next few days

@boaty82
Copy link
Contributor Author

boaty82 commented May 29, 2018

@mpkorstanje sorry for the delay, didn't get a chance to start on this last week and it was a bank holiday this weekend so have been busy throughout

I assume you want me to fork from the refactor-runtime-constructors branch and then raise a new PR?

Are there any tasks in Refactor Runtime 1367 you specifically want me to work on - or are you expecting me to just add support for --threads ?

@mpkorstanje
Copy link
Contributor

@boaty82 I've re-factored the runtime to the point that parallel execution should be fairly easy to implement. JUnit and TestNG may already work but I've yet to confirm this.

I believe you should be able to implement parallel support for the Runtime and make the plugins able to handle scenarios that are reported out of order. I am thinking it'd be best to leave --parallel out of @CucumberOptions. This should be left to JUnit and TestNG.

@mpkorstanje
Copy link
Contributor

And yes, you please work from refactor-runtime-constructors branch.

@mpkorstanje
Copy link
Contributor

Ability to still run tests synchronously
There may be situations where certain Features/Scenarios need to be run in a synchronized/serial manner. To this end features/scenarios will be scanned for usages of a tag beginning with @synchronized (case insensitive) and will run these Feature files separately, prior to any Feature files not found to have this tag. Buckets of synchronized Feature files can also be created by using any suffix e.g. @synchronized-1 or @synchronized-bob

I don't think we should support this. People are told to write their scenarios in such a way that they are independent from each other. So we shouldn't make an effort facilitate this sort of behavior. It also won't be possible to support this from JUnit or TestNG.

However it is very easy to implement this behavior your own test scripts.

cucumber --tags @synchronized
cucumber --tags "not @synchronized"  --threads 10

Or when running multiple sets

cucumber --tags "@synchronized and @synchronized-1"
cucumber --tags "@synchronized and @synchronized-2"
cucumber --tags "not @synchronized" --threads 10

I happen to know that for JUnit and TestNG maven provides similar functionality. You can create multiple executions with different configurations and select different tests in each configuration. I don't know about gradle.

@boaty82
Copy link
Contributor Author

boaty82 commented May 29, 2018

I don't think we should support this. People are told to write their scenarios in such a way that they are independent from each other

Hi, that's fine I won't add it. We already are doing what you describe (multiple executions) but just thought it would a nice supported feature.

BTW features being independent is not the problem, as in my case they are, but the production functionality being executed is not independent e.g. multiple features executing the same timed job(s)

I will still put the features into a queue so they can be pulled as each thread finishes executing it's feature. This will result in quicker overall test execution, rather than splitting 10 features into 2 lists and giving each thread a list. As the 5 in 1 list could be very quick and result in doing very little in comparison to the other thread

@mpkorstanje
Copy link
Contributor

I believe the scheduler has a queu for runnables to be executed.

@boaty82
Copy link
Contributor Author

boaty82 commented May 29, 2018

I believe you should be able to implement parallel support for the Runtime and make the plugins able to handle scenarios that are reported out of order

But I thought we agreed that issues like this shouldn't leak into many classes. I'm going to decorate EventBus to queue certain types of events from the thread until another certain event e.g. TestCaseFinished was received, upon which it could then flush all the events to ensure they are received in the expected order

I can see you've done something like this in BatchEventBus but I'm a little confused by the

        @Override
        public void send(Event event) {
            super.send(event);
            queue.add(event);
            if(event instanceof TestCaseFinished){
                parent.sendAll(queue);
                queue.clear();
            }
        }

I'm not 100% sure why it should be calling super.send(event) for all events as well as adding them to the queue to then be sent again later once TestCaseFinished is received

@mpkorstanje
Copy link
Contributor

I thought we agreed that issues like this shouldn't leak into many classes.

i don't believe it does. It only affects runtime, the runner supplier and the event bus. The main loop in Runtime could look like:

        ExecutorService executor = Executors.newFixedThreadPool(5);
        
        for (CucumberFeature cucumberFeature : features) {
            for (final PickleEvent pickleEvent : compiler.compileFeature(cucumberFeature)) {
                if (filters.matchesFilters(pickleEvent)) {
                    executor.submit(new Runnable() {
                        @Override
                        public void run() {
                            runnerSupplier.get().runPickle(pickleEvent);
                        }
                    });
                }
            }
        }

@mpkorstanje
Copy link
Contributor

mpkorstanje commented May 29, 2018

I'm not 100% sure why it should be calling super.send(event) for all events as well as adding them to the queue to then be sent again later once TestCaseFinished is received

We have two types of listeners to deal with.

  • JUnit which listens to individual pickles in isolation. These events shouldn't be queued up. Queuing them up will cause JUnit to think these tests completed instantly.
  • Formatters/Plugins. These need to get their events as an synchronized batch per pickle because the events lack the context to associate them to a pickle when multiple pickles are executed concurrently.
 Junit ---listens to---> [BatchEventBus] ---sends batches of events to--> [EventBus]  <---listens too--- HTML formatter 

@mpkorstanje
Copy link
Contributor

That said. BatchEventBus is a terrible name. If you know a better one please let me know!

@mpkorstanje mpkorstanje added this to the 4.0.0 milestone May 30, 2018
mpkorstanje added a commit that referenced this pull request Jun 5, 2018
[Core] Refactor Runtime

Extracting:
    Backend creation,
    Glue creation,
    Runner creation
    Feature compilation

from the runtime allows

    Tests on runners to skip the creation of Runtime
    Makes extracting Filter provider(#1352) and Feature provider(#1366) easier.
    Other runtimes such as JUnit and TestNg to skip the creation of the runtime
    --parallel (#1357), junit 5 (#1149) and pickle runner support.
    Clarifies the execution loop of Cucumber
@boaty82 boaty82 mentioned this pull request Jun 9, 2018
6 tasks
@boaty82
Copy link
Contributor Author

boaty82 commented Jun 9, 2018

PR replaced by Parallel cukes #1389

@boaty82 boaty82 closed this Jun 9, 2018
@lock
Copy link

lock bot commented Jun 9, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

1 similar comment
@lock
Copy link

lock bot commented Jun 24, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants