Test Runner Refactor + Simultaneous Test Run Support - supported by all "run types" (hosted, sauce, etc)

[awal112358]

Refactored the test Runner system to be modular/plugin oriented and added in functionality to launch simultaneous testRunner instances. Tested with saucelabs, chromeOnly, local selenium, and hosted selenium grid configurations successfully.

My goal is to allow for single-file selenium grid configurations as referenced in issue #111 and to create a modular basis for more test runner types and functionality in the future.

Prior to this change, the syntax for defining a capability was limited to:

  capabilities: {
    "browserName": "firefox",
    ...
  }

With this change, you can now do something like this:

  capabilities: [{
    "browserName": "firefox",
    "count": 5
  },
  {
    "browserName": "chrome",
    "count": 3
  }]

This would request 5 firefox and 3 chrome instances from your test runner. This allows for more ease of use with hosted selenium grid solutions, saucelabs, etc.

To prevent breaking existing implementations, the original object syntax (eg. capabilities: {}) will continue to work.

[0x-r4bbit]

@awal112358 could you rebase your stuff on current master, so we have clean PR that only covers your features? Also, it'd be better if your feature is then squashed into one commit, makes it easier to review and understand your code.

Nice-to-have™: a little bit more detailed PR message what your PR does and what changes on the top level for the end user.

[awal112358]

Thanks Pascal!

I cleaned up my fork and expanded my description of the change. Please let me know what else needs improvement. I'm more than happy to keep working on it.

Andrew

[0x-r4bbit]

@awal112358 Great job! Now it's much easier to get an idea of what's going on here :) Thanks for updating the PR!

[awal112358]

Thanks. I know it's kind of a big change and I'm sure it's a little scary so let me know if there's anything I can do to elaborate or make things more clear.

[juliemr]

Not sure this comment captures what it does - I'd say something like 'Resolve files to be located relative to current working directory'

[juliemr]

I think this is moving in the right direction, but there's still some fairly significant issues.

• This change doesn't match the style of the rest of the code - check out http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml
  • Subset of the above - the .class pattern doesn't match Protractor style. I'd prefer to see composition rather than an inheritance-type pattern.
• Reusing runner.js for both kicking off the actual run and dealing with multiple capabilities based on process.env.firstRun is clunky and difficult to follow. Separate this into different files. I like the idea of cli.js calling launcher.js - then launcher.js can start separate processes of runner.js.
• Let's rename the runners folder to driverproviders - 'runner' has become overloaded here.
• It would be cool if launcher.js could tell if the selenium server needs to be started up for multiple test runs, and just start it once (instead of having each process start its own). This might be a feature for another PR though.

Do these seem reasonable to you?

[juliemr]

One more point

• I don't think the behavior of the output of multiple runners is well defined. Do they all clamor for stdout together? Need some sort of solution here.

[awal112358]

Hi Julie,

Yeah, I think everything seems reasonable. I'll do some refactoring today to go with a more google-y style and I think the launcher/driver/providers ideas is good. I was thinking about doing some additional refactoring to try to separate some of the logic out further.

As for the output part, good catch. I was testing with a quick 1 spec test and everything came out perfect but I definitely see how this could get messy if a bunch of tests are all reporting to the same stdout. Let me think of this one a bit.

Hopefully I'll have another iteration by today or tomorrow and we can review that.

Thanks!

Andrew

[awal112358]

I've still got a decent amount of work to do, but I refactored to use a cli/launcher/runner system a bit today.

cli (basically only worries about optimist configuration) -> launcher (does as little as possible to spin up runners) -> runner.js (self configures from the passed in optimist args and executes).

I like how the configParser stuff came out. I feel like it's a bit easier to understand how it all works together now and it's a little more abstracted than before.

I'll still plan to coordinate the output of the worker processes tomorrow and hopefully get around to refactoring the primary runner class to be a little less classical OO and a little more compositionally oriented, as mentioned.

If you get a chance to check out the progress so far on my branch, let me know if you feel this is in the right direction.

https://github.com/awal112358/protractor/commit/dfdc21e9d112a335162eafb394ff5b59552d88d2

Thanks!

Andrew

[awal112358]

More changes:

https://github.com/awal112358/protractor/commit/54e2203aaf76886c9f1648489ee2e965f9007102
https://github.com/awal112358/protractor/commit/05bc4e231e9868fd22e481754445e246fb49b639

I was hoping to get it all closed out today but I've got probably another full day of work to get everything tested, fix a couple of issues, integrate the latest changes pulled into the protractor branch, cleanup doc, and do a little more abstraction to try to lessen the size of testRunner.js.

Nevertheless, it should now be a little closer to google style guidelines (I hope), is more compositional in nature, handles output of simultaneous runs gracefully, and there's now a launcher separate from the runner.js.

I'll hopefully finish this up on Monday and make another pull request. In the meantime, if you notice any issues let me know and I'll try to address them before my pull request.

[juliemr]

FYI something you've done has confused Github, so I can't view the changes easily. I'll take a look at your repo, but you might want to check and figure out why github is unhappy.

[awal112358]

LOL I'm not surprised, I'm a total noob to github and it's ways. The branch of interest is "newRunnerSystem". Once it's complete I'll rebase it onto my master and hopefully it'll show back up here in 1 nice flat commit.

This may be a better link. https://github.com/awal112358/protractor/compare/master...newRunnerSystem

It's still a WIP - from a quick self-review I see doc outdated, some style stuff that needs fixing, etc but hopefully it's closer.

[juliemr]

No worries, github still confuses me. I'm taking a quick look at your link at the moment.

[juliemr]

Awesome, looking good! I'm excited for this change.

Big picture:

• I think it might be cleaner to have the launcher use child_process.send to give the config and capabilities to the runner instances instead of adding environment variables. The runners could wait for a start message with their config before beginning. Then the configParser won't have to be used twice.
• With the previous comment, I don't think there's a a reason to have both runner and testRunner, so consolidate down to one.
• The output (for multiple tests) should have some sort of header line indicating which run it was.
• The configMap and loadConfig functions in the test runner feel awkward - I think these should be taken care of in the configParser and the launcher.

Style nitpicks:

• For private variables, _ should go after the name (privateVar_ instead of _privateVar)
• Add a space after if and for
• Methods on a class should be added as Class.prototype.foo = function instead of inside the constructor with this.foo = function

[awal112358]

Awesome - thanks Julie. Luckily I'm in the middle of a couple of these requests as we speak so yay for that. I'll take care of the rest and get you a new PR soon!

Thanks

[awal112358]

Oh - the only other thing I should check on is that I was thinking about abstracting the jasmine/cucumber/mocha stuff out of the main runner, because it's starting to feel like it's own object(s) and the main runner is getting big. The biggest thing I'm having trouble with is thinking of a name to represent the protractor runner versus the 3 lower level runners. Any preferences here?

[juliemr]

I have no idea why github decided this issue was merged! I'm sorry, I can't reopen - we can continue to discuss here, and then can you open a new PR when things are ready?

[awal112358]

sounds good :)

[juliemr]

As for the test framework stuff - I agree that it probably belongs in its own object, but I think that would be better done in a later PR - this one is doing tons already!

[awal112358]

gotcha. I'll just focus on cleanup and closeout then. Thanks again

[awal112358]

Oh! I forgot. I actually started out with the child_process.send, but ran into something I don't know how to get past. The onPrepare arg can be a direct js function, and I can't seem to pass those over send. That was what prompted me to abstract to configParser - because I knew I'd have to call on it twice - once to get the right capabilities parsed out and once to get the onPrepare stuff.

Any thoughts on that problem?

[juliemr]

Good point - and an interesting problem. After thinking about it more, I think it makes sense to have the config file required once per capability - and you need to parse it initially, so you will need to do it the way you initially had it.

I'm not a huge fan of re-using the config file for both the launcher and the runners, but I can't think o a better way to do it yet.

[awal112358]

I agree 100%. If I figure out a better way to do this I'll try to come back and improve it.

[awal112358]

Sorry for the delay - got pulled off onto some other things. I'm mostly done with the latest changes but I've got a bug or two to squash still. I'll hope to have you a PR by end of today.

Thanks!

[glepretre]

Hi,

First, I've tested the multiCapabilities this morning: it works great! Thanks for that!

But, one of my apps doesn't handle multiple connection to the data and it's causing tests failures. I was wondering if there's a way to define multiCapabilites (and keeping a single protractor config file) but disable parallel execution, to run each instance of webdriver after the other ends?

For now, I used a script running different protractor config files to do that.

EDIT: I asked this on StackOverflow too : Stack Overflow Question but I wanted your point of view as you followed this PR.

[gytisgreitai]

If i understand correctly, currently there is no way to pass multiCapabilities via cli ?

[juliemr]

@gytisgreitai you can pass anything in the configuration file via CLI, but the format will probably be ugly since it's an array of objects. See optimist for information on parsing those. https://www.npmjs.org/package/optimist

[FrigoEU]

I tried this with the following configuration:

multiCapabilities: [{
'browserName': 'chrome',
count: 3
}]

I was expecting my test files to be split in 3 and executed in parallel to finish in a third of the time (I have 3 test files). Instead all my tests are being executed in each of the 3 chrome browsers. Is there a possibility to have protractor run how I first expected it, meaning splitting my tests across the three instances to speed up my testing suite?

[elgalu]

How would the split work @FrigoEU ?
Should protractor try splitting per file name? or per describe?
Given i have designed my spec files to be run independently, I have a bash (shell) script that run each spec file in parallel, having this in protractor would be awesome!!!
👍

[FrigoEU]

Either would be fine for me. I expect splitting per file to be easier to implement, but I have no idea how Jasmine or Protractor actually work on the inside. it would be a really nice feature since E2E tests (at least in the projects I worked in) tend to run relatively long.

[juliemr]

I started issue #569 on test sharding - that way it'll be easier to find the discussion. Please continue there!