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

Add support for num-uniform-shards option #714

Merged
merged 6 commits into from
Apr 13, 2020

Conversation

jan-goral
Copy link
Contributor

@jan-goral jan-goral commented Apr 10, 2020

Fixes #713

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated

@jan-goral jan-goral self-assigned this Apr 10, 2020
@jan-goral jan-goral added this to the May 2020 milestone Apr 10, 2020
@jan-goral jan-goral force-pushed the 713-num-uniform-shards-option branch from d492b27 to 50cb090 Compare April 10, 2020 06:26
@codecov-io
Copy link

codecov-io commented Apr 10, 2020

Codecov Report

Merging #714 into master will increase coverage by 0.71%.
The diff coverage is 86.11%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #714      +/-   ##
============================================
+ Coverage     78.77%   79.48%   +0.71%     
- Complexity      665      666       +1     
============================================
  Files           113      113              
  Lines          2676     2696      +20     
  Branches        380      385       +5     
============================================
+ Hits           2108     2143      +35     
+ Misses          322      305      -17     
- Partials        246      248       +2     

@@ -99,6 +100,10 @@ class AndroidArgs(

devices.forEach { device -> assertDeviceSupported(device) }

require((numUniformShards != null && maxTestShards > 1).not()) {
Copy link
Contributor

@pawelpasterz pawelpasterz Apr 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require throws IllegalArgumentException. Flank has dedicated FlankFatalError for such cases. We could throw this instead. Cause message will still be printed but without stack trace. Let me know what do you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

shardingOption = ShardingOption().apply {
if (args.numUniformShards != null) {
testTargets = testShards.flatten()
uniformSharding = UniformSharding().setNumShards(args.numUniformShards)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When number of numUniformShards is bigger then number of tests flank we get error while processing results

java.lang.IllegalStateException: response.testCases must not be null
	at ftl.reports.api.CreateTestExecutionDataKt.createTestExecutionData(CreateTestExecutionData.kt:34)
	at ftl.reports.api.CreateTestExecutionDataKt$createTestExecutionData$1.invokeSuspend(CreateTestExecutionData.kt)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.ResumeModeKt.resumeUninterceptedMode(ResumeMode.kt:45)
	at kotlinx.coroutines.internal.ScopeCoroutine.afterCompletionInternal(Scopes.kt:32)
	at kotlinx.coroutines.JobSupport.completeStateFinalization(JobSupport.kt:310)
	at kotlinx.coroutines.JobSupport.tryFinalizeFinishingState(JobSupport.kt:236)
	at kotlinx.coroutines.JobSupport.tryMakeCompletingSlowPath(JobSupport.kt:849)
	at kotlinx.coroutines.JobSupport.tryMakeCompleting(JobSupport.kt:811)
	at kotlinx.coroutines.JobSupport.makeCompletingOnce$kotlinx_coroutines_core(JobSupport.kt:787)
	at kotlinx.coroutines.AbstractCoroutine.resumeWith(AbstractCoroutine.kt:111)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:46)
	at kotlinx.coroutines.DispatchedTask.run(Dispatched.kt:241)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:594)
	at kotlinx.coroutines.scheduling.CoroutineScheduler.access$runSafely(CoroutineScheduler.kt:60)
	at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:740)

In this particular case test apk had 7 tests and numUniformShards was set to 10. As result 10 shards was launched.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@jan-goral jan-goral force-pushed the 713-num-uniform-shards-option branch from 5038a91 to 4dd963f Compare April 13, 2020 03:50
@jan-goral jan-goral requested a review from pawelpasterz April 13, 2020 03:56
@jan-goral jan-goral force-pushed the 713-num-uniform-shards-option branch from 4dd963f to 375a9f1 Compare April 13, 2020 06:28
@jan-goral jan-goral merged commit 0d1155d into master Apr 13, 2020
@jan-goral jan-goral deleted the 713-num-uniform-shards-option branch April 13, 2020 06:46
@jan-goral jan-goral added the New Option Used to track PR with new configuration option (useful when updating fladle) label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Option Used to track PR with new configuration option (useful when updating fladle)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for num-uniform-shards FTL beta option
3 participants