-
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
refactor: Simplify beforeRunTests return singature #1339
Conversation
Timestamp: 2020-11-23 20:18:15 |
Codecov Report
@@ Coverage Diff @@
## master #1339 +/- ##
============================================
- Coverage 79.06% 79.04% -0.03%
+ Complexity 706 705 -1
============================================
Files 244 244
Lines 4687 4692 +5
Branches 824 825 +1
============================================
+ Hits 3706 3709 +3
Misses 551 551
- Partials 430 432 +2 |
* Treat Args as context
408c779
to
f6c6d13
Compare
suspend fun dumpShards( | ||
args: AndroidArgs, | ||
suspend fun AndroidArgs.dumpShards( | ||
@VisibleForTesting |
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.
@VisibleForTesting
is used for field, class, method visibility, not for a method parameter, which do not have any visibility. We should not use it
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.
For me, it's the only convention for marking why something is exposed but is not used in production code.
fun dumpShards( | ||
args: IosArgs, | ||
fun IosArgs.dumpShards( | ||
@VisibleForTesting |
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.
same here
saveShardChunks( | ||
shardFilePath = shardFilePath, | ||
shards = shards, | ||
size = shards.flatMap { it.value.shards.values }.count(), | ||
obfuscatedOutput = args.obfuscateDumpShards | ||
obfuscatedOutput = obfuscateDumpShards | ||
) | ||
} |
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.
How about
getAndroidMatrixShards().run {
saveShardChunks(
shardFilePath = shardFilePath,
shards = this,
size = flatMap { it.value.shards.values }.count(),
obfuscatedOutput = obfuscateDumpShards
)
}
?
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 like using val
but also I don't like unnecessary nested scopes and explicit this
keyword so it is the same for me but the current version gives fewer changes in diff.
println(args) | ||
val (matrixMap, testShardChunks, ignoredTests) = cancelTestsOnTimeout(args.project) { runTests(args) } | ||
val (matrixMap, testShardChunks, ignoredTests) = | ||
cancelTestsOnTimeout(project) { runTests() } |
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.
could be single line
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.
single line is not fully visible on 14 cal split screen ;P.
This PR is a cherrypicked refactor from #1321
After merging this one, the #1321 will slim down a little.
Checklist