Test sharding for jenkins #8150
Conversation
| @@ -195,6 +195,7 @@ | |||
| "makelogs": "3.0.2", | |||
| "marked-text-renderer": "0.1.0", | |||
| "mocha": "2.5.3", | |||
| "murmurhash3js": "3.0.1", | |||
There was a problem hiding this comment.
What are we using for the hashes in the upcoming state PR? Can we use the same dependency?
There was a problem hiding this comment.
We're using sha256 for the state pr, because sha256 provides better unique-ness. Here we're using murmur3 because the hashes it produces are numbers, making it trivial to bucket based on the hash (see get_shard_num.js).
So, we could use the same hashing function in both places, but I think the problems we're trying to solve in both places are different and these are the best algorithms for their respective job.
| * | ||
| * @return {string} url | ||
| */ | ||
| export function findBundleUrl() { |
There was a problem hiding this comment.
If we want to hardcode the /tests.bundle.js string here, we should rename this to findTestBundleUrl. If we want to support passing in the string here, we can keep the current name but should pass in /tests.bundle.js and update the comments.
|
Done reviewing! Left some comments and had a spirited discussion with Spencer. |
spalger
force-pushed
the
test/shardTestsToEaseMemoryPressure
branch
from
8385343 to
0c0fe6b
Compare
| }); | ||
|
|
||
| // Filter root level describe statements as they come | ||
| setupRootDescribeCallFilter(describeName => { |
There was a problem hiding this comment.
@cjcenizal After we talked, I moved to this style. Putting the comments in this file made it less readable, so I pushed the walking/intercepting down to the lower module and just pulled out the filter bit. Thoughts?
|
Hmm... test failure? |
|
Dang, I pushed the wrong master snapshot to esvm... fixing. (Thought that we had fixed sonatype...) |
| // murmur3 produces 32bit integers, so we devide it by the number of chunks | ||
| // to determine which chunk the suite should fall in. +1 because the current | ||
| // chunk is 1-based | ||
| const shardNum = Math.floor(hashInt / hashIntsPerShard) + 1; |
There was a problem hiding this comment.
This would be suiteShard.
|
Looks great! Left some suggestions, but they're up to you to decide if you want to implement them or not. LG(great)TM. |
|
@spalger If you update based on any feedback you want to address and make sure the tests are passing, I'll +1 this. |
spalger
force-pushed
the
test/shardTestsToEaseMemoryPressure
branch
from
0c0fe6b to
3999f9f
Compare
spalger
force-pushed
the
test/shardTestsToEaseMemoryPressure
branch
3 times, most recently
from
21ead03 to
739d7ec
Compare
|
|
||
| // ensure that window.describe isn't messed with by other code, | ||
| // if it becomes necessary in the future we can make this fancier | ||
| Object.defineProperty(window, 'describe', { |
There was a problem hiding this comment.
Any chance that Mocha could choose to do this in the future and break this solution?
There was a problem hiding this comment.
There is a chance that something will try to override window.describe, but since the describe property is now configurable: false and writable: false (the default) it will cause an error to be thrown if something tries to change it.
spalger
force-pushed
the
test/shardTestsToEaseMemoryPressure
branch
from
739d7ec to
9f9ecf5
Compare
The tests in master are currently failing regularly because our current browser tests are serious memory hogs. Investigation reveals that nearly every test is retaining all of the memory it causes to be allocated. We have made some progress to being able to diagnose the problems, but we expect that problem to take some serious work to fix. We need a short-term solution though, and this is it. Rather than modify the bundling process, we will shard the top-level test suites by name. For now, we've created 4 shards, but adding new shards is trivial if we need to. Sharding is accomplished by creating a murmur3 hash of the top level suite names, then bucketing based on the hash output. If a test suite resolves to shard2, but we are running shard1, we simply never pass the function to `mocha.describe()`. Rather than redefine every describe statement, we have shimmed the global `window.describe()` function to accomplish this.
spalger
force-pushed
the
test/shardTestsToEaseMemoryPressure
branch
2 times, most recently
from
ba0fa27 to
88427e9
Compare
|
LGTM |
…oryPressure add test sharding Former-commit-id: d94ab0b
The tests in master are currently failing regularly because our current browser tests are serious memory hogs. Investigation reveals that nearly every test is retaining all of the memory it causes to be allocated. We have made some progress to being able to diagnose the problems, but we expect that problem to take some serious work to fix. We need a short-term solution though, and this is it.
Rather than modify the bundling process, we will shard the top-level test suites by name. For now, we've created 4 shards, but adding new shards is trivial if we need to.
Sharding is accomplished by murmur3 hashing the top level suite names then bucketing based on the hash output. If a test suite resolves to shard2, but we are running shard1, we simply never pass the function to mocha. Rather than redefine every describe statement, we have shimmed the global
window.describe()function to accomplish this.