#707 Refactor Shard object by splitting it into smaller functions

[piotradamczyk5]

Fixes #707 Refactor Shared object by splitting it into smaller functions

Checklist

• Documented
• Unit tested
• release_notes.md updated

[codecov-commenter]

Codecov Report

Merging #799 into master will increase coverage by 0.03%.
The diff coverage is 86.48%.

@@             Coverage Diff              @@
##             master     #799      +/-   ##
============================================
+ Coverage     78.77%   78.81%   +0.03%     
+ Complexity      696      666      -30     
============================================
  Files           143      148       +5     
  Lines          2950     2959       +9     
  Branches        423      426       +3     
============================================
+ Hits           2324     2332       +8     
  Misses          354      354              
- Partials        272      273       +1     

[jan-goral]

I cannot find any reason to keep those logic wrapped in Shard object, it breaks SRP. This file is rather big and a lot of functions here are not related to each other so can be keep separately. I see 4 main parts of this feature here that can be split into dedicated files.

1. Function shardCountByTime and all related private functions
2. Function createShardsByShardCount and all related private functions
3. Function createTestMethodDurationMap which is shared part
4. Data classes, shouldn't hold any logic, they can be keep together if they are simple.

[jan-goral]

Also shardCountByTime function needs more work, there are 4 returns and 1 hidden throw + some assignments (value assignments are good for writing but not for reading).

I was thinking about something like that but without magic numbers:

fun shardCountByTime(
    testsToRun: List<FlankTestMethod>,
    oldTestResult: JUnitTestResult,
    args: IArgs
): Int = when {
    args.shardTime == -1 -> -1
    args.shardTime < -1 || args.shardTime == 0 -> throw FlankFatalError("Invalid shard time ${args.shardTime}")
    else -> testsToRun.testTotalTime(oldTestResult, args).let { testsTotalTime ->
        if (testsTotalTime <= args.shardTime) -1
        else min(ceil(testsTotalTime / args.shardTime).toInt(), args.maxTestShards)
    }
}

private fun List<FlankTestMethod>.testTotalTime(
    oldTestResult: JUnitTestResult,
    args: IArgs
) = sumByDouble {
    if (it.ignored) Shard.IGNORE_TEST_TIME
    else createTestMethodDurationMap(oldTestResult, args)[it.testName] ?: Shard.DEFAULT_TEST_TIME_SEC
}

Additionally flank now is using ftl api to run shards, and this api has restriction on maximum test shards up to 50. So below code is not valid now:

        // If there is no limit, use the calculated amount
        if (args.maxTestShards == -1) {
            return shardsByTime
        }

[jan-goral]

createShardsByShardCount still breaks SRP. As I see, it does at least 3 things.

1. creates testsCases and cacheMiss.
2. creates shards from testCases
3. prints info.

[bootstraponline]

Additionally flank now is using ftl api to run shards, and this api has restriction on maximum test shards up to 50.

FYI the API restriction is changing soon to 250. I don't want to hardcode the API restriction in Flank as Firebase can change this anytime.

[jan-goral]

Additionally flank now is using ftl api to run shards, and this api has restriction on maximum test shards up to 50.

FYI the API restriction is changing soon to 250. I don't want to hardcode the API restriction in Flank as Firebase can change this anytime.

We have IArgs.AVAILABLE_SHARD_COUNT_RANGE value, it is used to validate possible maximum shards on config level. Without hardcoded max value we are losing ability to check it early so It will be done later on ftl side.

[bootstraponline]

We have IArgs.AVAILABLE_SHARD_COUNT_RANGE value, it is used to validate possible maximum shards on config level.

We'll likely have to revisit this design. 50 is currently the global limit for both physical and virtual devices. In the future 250 will be the limit for virtual devices. Physical devices will still be limited to 50.

Without hardcoded max value we are losing ability to check it early so It will be done later on ftl side.

That's a good point. Let's keep the hard coded values.

Ideally FTL would expose an API for us to fetch the shard limits so we don't have to push a new Flank release every time. Probably the numbers change infrequently enough this won't be a big deal. If they did keep changing it then possibly https://firebase.google.com/docs/remote-config would be helpful.

[piotradamczyk5]

Additionally flank now is using ftl api to run shards, and this api has restriction on maximum test shards up to 50. So below code is not valid now:

        // If there is no limit, use the calculated amount
        if (args.maxTestShards == -1) {
            return shardsByTime
        }

so what should be done here? remove this condition?

[bootstraponline]

so what should be done here? remove this condition?

I think we should update to respect the limit defined in AVAILABLE_SHARD_COUNT_RANGE, then in a future PR we'll have to solve for the 250 limit once FTL makes this change.

[piotradamczyk5]

AVAILABLE_SHARD_COUNT_RANGE

so what should be done here? remove this condition?

I think we should update to respect the limit defined in AVAILABLE_SHARD_COUNT_RANGE, then in a future PR we'll have to solve for the 250 limit once FTL makes this change.

so new condition will look
// If there is no limit, use the calculated amount
if (args.maxTestShards == NO_LIMIT) {
return AVAILABLE_SHARD_COUNT_RANGE.last
}

[jan-goral]

There is no good reason to wrap functions in object scope.
Kotlin thankfully has no java limitations so we are not forced to keep java rules.
Classes and objects are good for composing data, not functions.

[jan-goral]

I think there is still a lot of space for improvements. As refactors need more care than common tasks solving them in comment -> fix -> repeat workflow doesn't make sense so I will leave it at current state. I wonder if other team members have any suggestions.

[pawelpasterz]

#707 has main goals

1. Split code into smaller chunks/pieces
2. Simplify logic (for some time flank had new features 'attached' only which led to a bit tangled code)

I think with this PR (great job!) we managed to handle 1). IMO we should continue our effort with second part, maybe not within this issue but followup one.

Let me know know what think, and thanks!

cc
@bootstraponline

[bootstraponline]

1). IMO we should continue our effort with second part, maybe not within this issue but followup one

Agreed. We can consider converting the Flank refactor (#718) ticket into an Epic composed of multiple tickets. I'd like to get the remaining features/bug fixes out and then focus on the refactor. What do you think?