Refactor config entities and arguments #831

jan-goral · 2020-06-02T01:50:17Z

This is a part of #718

Goals:

Unification of yaml entities and cli options as both do the same there was no reason to keep them separately.
Reduction of redundant code between AndroidArgs and IosArgs.
Convertion of AndroidArgs and IosArgs to data classes.
Extraction of factory methods and validators.

Test Plan

How do we know the code works?

This refactor of cli options, yaml config, AndroidArgs and IosArgs. It doesn't bring any features or changes so the existing unit tests should cover possible regression.

Checklist

Documented
Unit tested
release_notes.md updated

codecov-commenter · 2020-06-02T01:53:54Z

Codecov Report

Merging #831 into master will increase coverage by 1.01%.
The diff coverage is 93.78%.

@@             Coverage Diff              @@
##             master     #831      +/-   ##
============================================
+ Coverage     80.14%   81.15%   +1.01%     
+ Complexity      674      637      -37     
============================================
  Files           151      166      +15     
  Lines          3087     3226     +139     
  Branches        443      463      +20     
============================================
+ Hits           2474     2618     +144     
- Misses          336      343       +7     
+ Partials        277      265      -12

piotradamczyk5 · 2020-06-02T14:58:10Z

test_runner/src/main/kotlin/ftl/args/ValidateCommonArgs.kt

+        if (smartFlankGcsPath.count { it == '/' } <= 2 || !smartFlankGcsPath.endsWith(".xml")) {
+            throw FlankFatalError("smart-flank-gcs-path must be in the format gs://bucket/foo.xml")
+        }
+    }


Personally I prefer when instead of nested ifs

TBH me to. I will change this.

private fun CommonArgs.assertSmartFlankGcsPath(): Unit = with(smartFlankGcsPath) { when { isEmpty() -> Unit startsWith(FtlConstants.GCS_PREFIX).not() -> throw FlankFatalError( "smart-flank-gcs-path must start with gs://" ) count { it == '/' } <= 2 || endsWith(".xml").not() -> throw FlankFatalError( "smart-flank-gcs-path must be in the format gs://bucket/foo.xml" ) } }

piotradamczyk5 · 2020-06-02T14:59:18Z

test_runner/src/main/kotlin/ftl/cli/firebase/test/CommonRunCommand.kt

-                "which don't support ansi codes, to avoid corrupted output use `single` or `verbose`."]
-    )
-    var outputStyle: String? = null
+    // Flank specific



empty line not needed :)

piotradamczyk5 · 2020-06-02T15:02:27Z

test_runner/src/main/kotlin/ftl/config/common/CommonGcloudConfig.kt

+
+        override val map = mapOf(
+            "gcloud" to keys
+        )


could be single line

Rights, I was trying to keep one convention for maps and keys in all configs. Anyway this probably shouldn't be map, rather simple pair, same about another configs.

adamfilipow92 · 2020-06-02T15:03:44Z

test_runner/src/main/kotlin/ftl/config/android/AndroidGcloudConfig.kt

+    fun deviceMap(map: Map<String, String>?) {
+        if (map.isNullOrEmpty()) return
+        val androidDevice = Device(
+            model = map.getOrDefault("model", FtlConstants.defaultAndroidModel),


We use "model","version" etc in IosArgs and Android args. Maybe we should move keys to const/enums?

yeah, why not

Heh, if both android and ios gcloud config has common device option probably it should be in CommonGcloudConfig.

piotradamczyk5 · 2020-06-02T15:11:54Z

test_runner/src/main/kotlin/ftl/args/CreateIosArgs.kt

+    config: IosConfig? = null,
+    gcloud: IosGcloudConfig = config!!.platform.gcloud,
+    flank: IosFlankConfig = config!!.platform.flank,
+    commonArgs: CommonArgs = config!!.common.createCommonArgs(config.data)


this is a little bit unclear for me, config is nullable, but later on we declare that it is not null. So probably IosConfig is not nullable type.

Right, it doesn't looks well and should be explained, config is nullable cause when gcloud, flank, commonArgs are set explicit the config is redundant from logical perspective. Possible solution to get rid of null is:

fun createIosArgs( config: IosConfig ) = createIosArgs( gcloud = config.platform.gcloud, flank = config.platform.flank, commonArgs = config.common.createCommonArgs(config.data) ) private fun createIosArgs( gcloud: IosGcloudConfig, flank: IosFlankConfig, commonArgs: CommonArgs ) = IosArgs( commonArgs = commonArgs, xctestrunZip = gcloud.test!!.processFilePath("from test"), xctestrunFile = gcloud.xctestrunFile!!.processFilePath("from xctestrun-file"), xcodeVersion = gcloud.xcodeVersion, devices = gcloud.device!!, testTargets = flank.testTargets!!.filterNotNull() )

But it gives 2 functions instead of 1 and more lines. I'm am not sure which is better.

for me 2 functions looks much better

piotradamczyk5 · 2020-06-02T15:23:21Z

test_runner/src/main/kotlin/ftl/args/IosArgs.kt

+            FlankTestMethod(it, ignored = false)
+        },
+        this
+    )


I would format it

private fun IosArgs.calculateShardChunks() = if (disableSharding) listOf(emptyList()) else ArgsHelper.calculateShards( filteredTests = filterTests(Xctestrun.findTestNames(xctestrunFile), testTargets) .distinct() .map { FlankTestMethod(it, ignored = false) }, args = this )

adamfilipow92 · 2020-06-03T12:08:50Z

test_runner/src/main/kotlin/ftl/args/ValidateCommonArgs.kt

+        maxTestShards !in IArgs.AVAILABLE_SHARD_COUNT_RANGE &&
+        maxTestShards != -1
+    ) throw FlankFatalError(
+        "max-test-shards must be >= 1 and <= 50, or -1. But current is $maxTestShards"


I think we should use IArgs.AVAILABLE_SHARD_COUNT_RANGE.first / IArgs.AVAILABLE_SHARD_COUNT_RANGE.last in exception message.

adamfilipow92 · 2020-06-03T15:17:11Z

test_runner/src/main/kotlin/ftl/config/Device.kt

@@ -19,3 +23,19 @@ data class Device(
          orientation: $orientation""".trimStartLine()
    }


I know you don't change this line but:

override fun toString(): String { return """ - model: $model version: $version locale: $locale orientation: $orientation""".trimStartLine() }

vs

override fun toString() = """ - model: $model version: $version locale: $locale orientation: $orientation""".trimStartLine()

This only for discussion to keep one style. Personally I prefer second.

jan-goral self-assigned this Jun 2, 2020

jan-goral changed the title ~~Refactor Args~~ Refactor config entities and arguments Jun 2, 2020

jan-goral marked this pull request as ready for review June 2, 2020 01:57

jan-goral requested review from bootstraponline, pawelpasterz, adamfilipow92 and piotradamczyk5 June 2, 2020 01:57

jan-goral added the Tech Debt label Jun 2, 2020

piotradamczyk5 reviewed Jun 2, 2020

View reviewed changes

adamfilipow92 reviewed Jun 2, 2020

View reviewed changes

piotradamczyk5 reviewed Jun 2, 2020

View reviewed changes

jan-goral added 13 commits June 3, 2020 13:27

Omit global app apk if additional-app-test-apks specified

a66a2a5

CR fixes

63f5cab

Move useLegacyJUnitResult to correct class

eba7e74

Share models for cli & yaml config

9045f57

Fix tests

8404792

Sort

2d924bb

Fix after rebase

2b6ab48

Convert IosArgs to data class

7c12e78

Remove legacy yaml entities

05d0fa5

Update release_notes.md

4f79af9

CR fixes

ad2c0c7

CR fixes

3166fd0

Remove unused import

d10c9aa

jan-goral force-pushed the refactor-args branch from 7155940 to f17715f Compare June 3, 2020 11:40

Fix after rebase

01f482d

jan-goral force-pushed the refactor-args branch from f17715f to 01f482d Compare June 3, 2020 11:42

adamfilipow92 reviewed Jun 3, 2020

View reviewed changes

Use AVAILABLE_SHARD_COUNT_RANGE instead of hardcoded value

dbefff2

jan-goral requested review from adamfilipow92 and piotradamczyk5 June 3, 2020 14:52

adamfilipow92 reviewed Jun 3, 2020

View reviewed changes

adamfilipow92 approved these changes Jun 3, 2020

View reviewed changes

piotradamczyk5 approved these changes Jun 3, 2020

View reviewed changes

jan-goral merged commit acd142d into master Jun 3, 2020

jan-goral deleted the refactor-args branch June 3, 2020 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor config entities and arguments #831

Refactor config entities and arguments #831

jan-goral commented Jun 2, 2020 •

edited by bootstraponline

Loading

codecov-commenter commented Jun 2, 2020 •

edited

Loading

piotradamczyk5 Jun 2, 2020 •

edited

Loading

jan-goral Jun 2, 2020

jan-goral Jun 2, 2020

piotradamczyk5 Jun 2, 2020

piotradamczyk5 Jun 2, 2020

jan-goral Jun 2, 2020

adamfilipow92 Jun 2, 2020

jan-goral Jun 2, 2020

jan-goral Jun 2, 2020 •

edited

Loading

piotradamczyk5 Jun 2, 2020

jan-goral Jun 2, 2020

piotradamczyk5 Jun 2, 2020

piotradamczyk5 Jun 2, 2020

adamfilipow92 Jun 3, 2020

adamfilipow92 Jun 3, 2020

		@@ -19,3 +23,19 @@ data class Device(
		orientation: $orientation""".trimStartLine()
		}

Refactor config entities and arguments #831

Refactor config entities and arguments #831

Conversation

jan-goral commented Jun 2, 2020 • edited by bootstraponline Loading

Test Plan

Checklist

codecov-commenter commented Jun 2, 2020 • edited Loading

Codecov Report

piotradamczyk5 Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-goral Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-goral commented Jun 2, 2020 •

edited by bootstraponline

Loading

codecov-commenter commented Jun 2, 2020 •

edited

Loading

piotradamczyk5 Jun 2, 2020 •

edited

Loading

jan-goral Jun 2, 2020 •

edited

Loading