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

Projects
None yet
3 participants
@bendemboski
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
Support specifying multiple partitions
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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Oct 28, 2016

Owner

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.

Owner

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

This comment has been minimized.

Show comment
Hide comment
@bendemboski

bendemboski Nov 3, 2016

Contributor

I think this is all ready to go now.

Contributor

bendemboski commented Nov 3, 2016

I think this is all ready to go now.

@trentmwillis

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.

@trentmwillis

trentmwillis Nov 12, 2016

Owner

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

@trentmwillis

trentmwillis Nov 12, 2016

Owner

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

Show outdated Hide outdated lib/commands/exam.js
if (partitions) {
partitions = partitions.join(',');
}
commandOptions.query = addToQuery(commandOptions.query, '_partitions', partitions);

This comment has been minimized.

@trentmwillis

trentmwillis Nov 12, 2016

Owner

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'].

@trentmwillis

trentmwillis Nov 12, 2016

Owner

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'].

Show outdated Hide outdated lib/commands/exam.js
*
* @param {String} baseUrl
* @param {Array} partitions

This comment has been minimized.

@trentmwillis

trentmwillis Nov 12, 2016

Owner

Should be Array<Number> right?

@trentmwillis

trentmwillis Nov 12, 2016

Owner

Should be Array<Number> right?

Show outdated Hide outdated lib/utils/tests-options-validator.js
}
}

This comment has been minimized.

@trentmwillis

trentmwillis Nov 12, 2016

Owner

Nit: delete extra line

@trentmwillis

trentmwillis Nov 12, 2016

Owner

Nit: delete extra line

Ben Demboski
Update query params
Also fix a whitespace error and put a multi-partition/parallel example in the readme.
@trentmwillis

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.

Show outdated Hide outdated lib/commands/exam.js
@@ -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.

@trentmwillis

trentmwillis Nov 15, 2016

Owner

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

@trentmwillis

trentmwillis Nov 15, 2016

Owner

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

Show outdated Hide outdated vendor/ember-exam/test-loader.js
}
var group = partition - 1;
tests = tests.concat(lintTestGroups[group]).concat(otherTestGroups[group]);

This comment has been minimized.

@trentmwillis

trentmwillis Nov 15, 2016

Owner

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

@trentmwillis

trentmwillis Nov 15, 2016

Owner

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

Show outdated Hide outdated vendor/ember-exam/test-loader.js
var group = partition - 1;
tests = tests.concat(lintTestGroups[group]).concat(otherTestGroups[group]);
}
return tests;

This comment has been minimized.

@trentmwillis

trentmwillis Nov 15, 2016

Owner

Nitpick: newline before this return.

@trentmwillis

trentmwillis Nov 15, 2016

Owner

Nitpick: newline before this return.

Ben Demboski
@bendemboski

This comment has been minimized.

Show comment
Hide comment
@bendemboski

bendemboski Nov 16, 2016

Contributor

@trentmwillis feedback addressed

Contributor

bendemboski commented Nov 16, 2016

@trentmwillis feedback addressed

@trentmwillis trentmwillis merged commit b037543 into trentmwillis:master Nov 16, 2016

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

This comment has been minimized.

Show comment
Hide comment
@trentmwillis

trentmwillis Nov 16, 2016

Owner

🎉 thanks for all your work on this!

Owner

trentmwillis commented Nov 16, 2016

🎉 thanks for all your work on this!

@bendemboski

This comment has been minimized.

Show comment
Hide comment
@bendemboski

bendemboski Nov 16, 2016

Contributor

👍

Contributor

bendemboski commented Nov 16, 2016

👍

@Dhaulagiri

This comment has been minimized.

Show comment
Hide comment
@Dhaulagiri

Dhaulagiri 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 @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

This comment has been minimized.

Show comment
Hide comment
@bendemboski

bendemboski Jan 4, 2017

Contributor

@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.

Contributor

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