-
Notifications
You must be signed in to change notification settings - Fork 119
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
SmartFlank algo #397
SmartFlank algo #397
Conversation
Codecov Report
@@ Coverage Diff @@
## smart_flank #397 +/- ##
==============================================
Coverage ? 79.37%
Complexity ? 530
==============================================
Files ? 68
Lines ? 1702
Branches ? 264
==============================================
Hits ? 1351
Misses ? 191
Partials ? 160 |
// Create a map with information from previous junit run | ||
testResult.testsuites?.forEach { testsuite -> | ||
testsuite.testcases?.forEach { testcase -> | ||
junitMap[testcase.name] = testcase.time.toDouble() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testcase name isn't unique. The fully qualified name is suite.name + testCase.classname + testCase.name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! I wanted to test that by hooking with a real run.
|
||
val noTestsAdded = timeBudget == targetShardDuration | ||
val testOverBudget = test.time >= timeBudget | ||
// We want to iterate over testcase going from higher to lower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
higher to lower
I think slowest to fastest is more descriptive.
shard.time += tm.time | ||
|
||
// Sort the shards to keep the most empty shard first | ||
shards = shards.sortedBy { it.time } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
algorithm looks great, left come comments on the approach.
if (testShardChunks.isEmpty()) fatalError("Failed to populate test shard chunks") | ||
} | ||
// If maxShards is infinite or we have more shards than tests, let's match it | ||
val shardsCount = if (maxShards == -1 || maxShards > testCount) testCount else maxShards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is great, we should be able to drop the old chunk based sharding method.
We'll still want to add in test-methods-always-run, that could be in a different method to follow the single responsibility principle.
return TestShard(shardTime, shardTests) | ||
} | ||
val tm = TestMethod(it.name, it.time) | ||
val shard = shards[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val shard = shards.first()
4a09cf3
to
07e8d1c
Compare
c383718
to
7844101
Compare
cc1f92b
to
60ce276
Compare
testTargetsAlwaysRun, | ||
testShards | ||
) | ||
if (true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably we should remove this?
|
||
result | ||
} else { | ||
calculateShards( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testTargetsAlwaysRun support is missing. I think calculateShards should be removed
val result = convertShards(Shard.calculateShardsByTime(filteredTests, oldTestResult, testShards)) | ||
val shardCreationTime = TimeUnit.SECONDS.convert(System.nanoTime() - start, TimeUnit.NANOSECONDS) | ||
|
||
println("Shard calculation took: $shardCreationTime") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think users will benefit from shard calculation numbers. This makes more sense in a JUnit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. will remove.
val testcases = mutableListOf<TestMethod>() | ||
runningTests.forEach { | ||
// junitMap doesn't include `class `, we remove it to search in the map | ||
val key = it.replace("class ", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use replaceFirst
* Smart Flank * SmartFlank algo (#397) * SmartFlank * SmarkFlank code review * Fix iOS tests, add testsAlwaysRun * Print cache hit rate and shard times * Lint fix * Round times when printing to stdout * Fix tests * Fixed int division * iOS now working * Fix iOS sharding * Update catalog fixtures * Fix ShardTest * Add performance test * Rename junitGcsPath to smartFlankGcsPath * Fix lint issues * Update release_notes.md * Continue on failed visit file
No description provided.