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

Support specifying multiple partitions #63

Merged
merged 4 commits into from Nov 16, 2016

Conversation

@bendemboski
Copy link
Contributor

bendemboski commented Oct 28, 2016

The primary use case is splitting test modules across multiple CI containers, and having each of those CI containers run its list of tests in multiple parallel runs (you would specify split, parallel, and multiple partitions for each container). This can also be used for unbalanced partitioning, e.g. if you want one container to run 2/3 of the tests and the other run 1/3 (you would specify split and then different numbers of partitions for different containers).

Fixes #62

Ben Demboski
The primary use case is splitting test modules across multiple CI containers, and having each of those CI containers run its list of tests in multiple parallel runs (you would specify split, parallel, and multiple partitions for each container). This can also be used for unbalanced partitioning, e.g. if you want one container to run 2/3 of the tests and the other run 1/3 (you would specify split and then different numbers of partitions for different containers).
@trentmwillis
Copy link
Member

trentmwillis commented Oct 28, 2016

Thanks for the work on this! A quick glance looks pretty good. I'll review in depth some time this weekend, if you could look into the codeclimate issue before then it'd be appreciated.

Ben Demboski
@bendemboski
Copy link
Contributor Author

bendemboski commented Nov 3, 2016

I think this is all ready to go now.

Copy link
Member

trentmwillis left a comment

Sorry for letting this sit. Looking good so far, main thing is I'd like to keep _partition without the s. Thanks for including tests!

@@ -98,6 +102,8 @@ ok 2 PhantomJS 1.9 - Exam Partition #3 - some other other test
ok 3 PhantomJS 1.9 - Exam Partition #2 - some other test
```

You can also combine the `parallel` option with the `partition` option to split tests, and then recombine partitions into parallel runs. This would, for example, allow you to run tests in multiple CI containers and have each CI container parallelize its list of tests.

This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 12, 2016

Member

Can we add another code example here of how to use the partition option with parallel?

if (partitions) {
partitions = partitions.join(',');
}
commandOptions.query = addToQuery(commandOptions.query, '_partitions', partitions);

This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 12, 2016

Member

We should leave this as _partition for consistency with the CLI params. QUnit will automatically treat repeated URL params as an array. So if we do tests?_partition=1&_partition=2 then you'll get QUnit.urlParams._partition === ['1', '2'].

*
* @param {String} baseUrl
* @param {Array} partitions

This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 12, 2016

Member

Should be Array<Number> right?

}
}


This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 12, 2016

Member

Nit: delete extra line

Ben Demboski
Also fix a whitespace error and put a multi-partition/parallel example in the readme.
Copy link
Member

trentmwillis left a comment

A few more super tiny things and then this will be good to merge. Let me know when you update and I'll merge+release.

@@ -34,7 +34,7 @@ module.exports = TestCommand.extend({

availableOptions: [
{ name: 'split', type: Number, description: 'A number of files to split your tests across.' },
{ name: 'partition', type: Number, description: 'The number of the partition to run after splitting.' },
{ name: 'partition', type: [Array, String], description: 'The number of the partition(s) to run after splitting.' },

This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 15, 2016

Member

Shouldn't this be type: [Array, Number]?

}

var group = partition - 1;
tests = tests.concat(lintTestGroups[group]).concat(otherTestGroups[group]);

This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 15, 2016

Member

concat() can accept multiple arrays at once, so let's do that instead.

var group = partition - 1;
tests = tests.concat(lintTestGroups[group]).concat(otherTestGroups[group]);
}
return tests;

This comment has been minimized.

Copy link
@trentmwillis

trentmwillis Nov 15, 2016

Member

Nitpick: newline before this return.

Ben Demboski
@bendemboski
Copy link
Contributor Author

bendemboski commented Nov 16, 2016

@trentmwillis feedback addressed

@trentmwillis trentmwillis merged commit b037543 into ember-cli:master Nov 16, 2016
2 checks passed
2 checks passed
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@trentmwillis
Copy link
Member

trentmwillis commented Nov 16, 2016

🎉 thanks for all your work on this!

@bendemboski
Copy link
Contributor Author

bendemboski commented Nov 16, 2016

👍

@Dhaulagiri
Copy link

Dhaulagiri commented Jan 4, 2017

@bendemboski @trentmwillis do you have an example of this working on circleci somewhere one could look at? I'm trying this in our circle setup but parallelizing tests within a container doesn't seem to make the overall test runs much faster.

(happy to move this conversation to community slack instead)

@bendemboski
Copy link
Contributor Author

bendemboski commented Jan 4, 2017

@Dhaulagiri I don't have an example that I can make public. My guess is that the limiting factor in your test speed is CPU, rather than blocking on async events or something, so parallelizing within the same container won't speed anything up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.