#823 Added obfuscate option to dump shards #837

piotradamczyk5 · 2020-06-05T07:35:11Z

Fixes #823

Test Plan

How do we know the code works?

run
flank [android|ios] run --dump-shards --obfuscate
and compare output file with running
flank [android|ios] run --dump-shards
Check ObfuscationTest for checking obfuscation results

Checklist

Documented
Unit tested
release_notes.md updated

codecov-commenter · 2020-06-05T07:38:02Z

Codecov Report

Merging #837 into master will increase coverage by 0.32%.
The diff coverage is 96.55%.

@@             Coverage Diff              @@
##             master     #837      +/-   ##
============================================
+ Coverage     81.23%   81.56%   +0.32%     
  Complexity      637      637              
============================================
  Files           167      169       +2     
  Lines          3240     3292      +52     
  Branches        463      467       +4     
============================================
+ Hits           2632     2685      +53     
+ Misses          343      341       -2     
- Partials        265      266       +1

piotradamczyk5 · 2020-06-05T07:39:22Z

I pushed all documentation changes which were generated

adamfilipow92 · 2020-06-05T11:10:34Z

test_runner/src/test/kotlin/ftl/util/ObfuscationGsonTest.kt

+class ObfuscationGsonTest {
+
+    @Test
+    fun `Should return gson with correct type adapters registered`() {


Everywhere we starting test name from lower letter. Maybe we should keep always same naming convention?

adamfilipow92 · 2020-06-05T11:15:03Z

If you add new command you should update

README.md
flank.yml
flank.ios.yml

piotradamczyk5 · 2020-06-05T11:16:20Z

If you add new command you should update

README.md

flank.yml

flank.ios.yml

this is CLI option not flank settings. --dump-shards option is also missed there

adamfilipow92 · 2020-06-05T11:18:35Z

If you add new command you should update

README.md

flank.yml

flank.ios.yml

this is CLI option not flank settings. --dump-shards option is also missed there

Right, sorry!

adamfilipow92

👍

jan-goral · 2020-06-05T15:54:03Z

test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidRunCommand.kt

@@ -47,7 +47,7 @@ class AndroidRunCommand : CommonRunCommand(), Runnable {

        runBlocking {
            if (dumpShards)
-                dumpShards(config) else
+                dumpShards(args = config, obfuscatedOutput = obfuscate) else


Why obfuscate is passed via function argument? It shouldn't be in CommonArgs?

this is CLI option which are not in Common args, so I would like to make them same as others

jan-goral · 2020-06-05T16:21:15Z

test_runner/src/main/kotlin/ftl/util/ObfuscationGson.kt

+    }
+}
+
+private object ObfusctedIosJsonSerializer : JsonSerializer<List<List<String>>> {


typo, should be ObfuscatedIosJsonSerializer

jan-goral · 2020-06-05T16:22:17Z

test_runner/src/main/kotlin/ftl/util/ObfuscationGson.kt

+@VisibleForTesting
+internal object ListOfStringListTypeToken : TypeToken<ShardChunks>()
+
+private object ObfusctedAndroidJsonSerializer : JsonSerializer<List<String>> {


typo, should be ObfuscatedAndroidJsonSerializer

jan-goral · 2020-06-05T16:28:14Z

test_runner/src/main/kotlin/ftl/util/Obfuscation.kt

+private const val UPPER_CASE_CHARS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+private const val START_CONTEXT_KEY = ""
+
+interface Obfuscation {


Redundant abstraction if you remove this interface everything will work as well, so that means it is not needed. Keep it simple

jan-goral · 2020-06-05T16:29:33Z

test_runner/src/main/kotlin/ftl/util/Obfuscation.kt

+
+class AndroidObfuscation : Obfuscation {
+    private val contextMapAndroid = mutableMapOf<String, MutableMap<String, String>>()
+    private val androidTestMethodSeparator = '#'


it's not configurable so it could be const

jan-goral · 2020-06-05T16:33:58Z

test_runner/src/main/kotlin/ftl/util/ObfuscationGson.kt

+private object ObfusctedAndroidJsonSerializer : JsonSerializer<List<String>> {
+    private val obfuscation by lazy { AndroidObfuscation() }
+
+    override fun serialize(src: List<String>?, typeOfSrc: Type?, context: JsonSerializationContext?): JsonElement {


Are you sure that any any of this args could be null? IMO not.

jan-goral · 2020-06-05T16:37:19Z

test_runner/src/main/kotlin/ftl/util/Obfuscation.kt

+    }
+
+    private fun obfuscateAndroidPackageAndClass(packageNameWithClass: String): String {
+        return packageNameWithClass


could be expression

jan-goral · 2020-06-05T16:37:35Z

test_runner/src/main/kotlin/ftl/util/Obfuscation.kt

+}
+
+private fun obfuscateMethodName(methodName: String, context: MutableMap<String, String>): String {
+    return context.getOrPut(methodName) { nextSymbol(methodName, context) }


could be expression

jan-goral · 2020-06-05T16:57:00Z

test_runner/src/main/kotlin/ftl/util/Obfuscation.kt

+    fun obfuscate(input: String): String
+}
+
+class AndroidObfuscation : Obfuscation {


You already know my opinion about object oriented programming. Do we really need class for that I guess it's unnecessary complication to bound functions with state. I could be simple function like that:

typealias ObfuscationContext = Map<String, MutableMap<String, String>> fun ObfuscationContext.obfuscateAndroidTestMethod(input: String): String { ... }

The same about ios.

the idea was to keep it state, however it could be done much better by keeping context and make extension function. Thank you, for this idea

jan-goral · 2020-06-05T17:12:09Z

test_runner/src/main/kotlin/ftl/util/Obfuscation.kt

+    private fun obfuscateAndroidPackageAndClass(packageNameWithClass: String): String {
+        return packageNameWithClass
+            .split(androidPackageSeparator)
+            .fold(START_CONTEXT_KEY) { previous, next ->


IMO, START_CONTEXT_KEY is redundant. fold("") is better than fold(START_CONTEXT_KEY) because START_CONTEXT_KEY forces to remember next name, and scroll to check what value it holds.

…-obfuscation-option # Conflicts: # release_notes.md

…option' into #823-add-dump-shard-obfuscation-option

piotradamczyk5 · 2020-06-15T18:08:27Z

this task must be updated after merging #853

# Conflicts: # release_notes.md # test_runner/docs/ascii/flank.jar_-android-run.adoc # test_runner/docs/ascii/flank.jar_-firebase-test-android-run.adoc # test_runner/docs/ascii/flank.jar_-firebase-test-ios-run.adoc # test_runner/docs/ascii/flank.jar_-ios-run.adoc

…-obfuscation-option # Conflicts: # release_notes.md

adamfilipow92

👍

#823 Added obfuscate option to dump shards

988bcf1

piotradamczyk5 requested review from bootstraponline, pawelpasterz, jan-goral and adamfilipow92 June 5, 2020 07:35

piotradamczyk5 self-assigned this Jun 5, 2020

#837 Update release notes

ce29ea7

adamfilipow92 reviewed Jun 5, 2020

View reviewed changes

piotradamczyk5 added 2 commits June 5, 2020 13:17

Update ObfuscationGsonTest.kt

eee9bdb

Update ObfuscationTest.kt

929339c

adamfilipow92 previously approved these changes Jun 5, 2020

View reviewed changes

jan-goral suggested changes Jun 5, 2020

View reviewed changes

Piotr Adamczyk added 4 commits June 8, 2020 06:39

Merge remote-tracking branch 'origin/master' into #823-add-dump-shard…

20456a8

…-obfuscation-option # Conflicts: # release_notes.md

#823 Added obfuscate option to dump shards

41d926a

#823 Added obfuscate option to dump shards

6150c6f

Merge remote-tracking branch 'origin/#823-add-dump-shard-obfuscation-…

993d61f

…option' into #823-add-dump-shard-obfuscation-option

piotradamczyk5 dismissed adamfilipow92’s stale review via 993d61f June 8, 2020 04:46

piotradamczyk5 requested review from jan-goral and adamfilipow92 June 8, 2020 04:47

#823 Handle case when method name is not present

f4f53c6

Piotr Adamczyk added 4 commits June 17, 2020 15:05

#823 update docs

76395e1

Merge remote-tracking branch 'origin/master' into #823-add-dump-shard…

8c7d2b2

…-obfuscation-option # Conflicts: # release_notes.md

#823 update tests

193d9e2

#823 update tests

4e4e747

jan-goral approved these changes Jun 19, 2020

View reviewed changes

adamfilipow92 approved these changes Jun 19, 2020

View reviewed changes

piotradamczyk5 merged commit 23d66ed into master Jun 19, 2020

piotradamczyk5 deleted the #823-add-dump-shard-obfuscation-option branch June 19, 2020 14:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#823 Added obfuscate option to dump shards #837

#823 Added obfuscate option to dump shards #837

piotradamczyk5 commented Jun 5, 2020 •

edited

Loading

codecov-commenter commented Jun 5, 2020 •

edited

Loading

piotradamczyk5 commented Jun 5, 2020

adamfilipow92 Jun 5, 2020

piotradamczyk5 Jun 5, 2020

adamfilipow92 commented Jun 5, 2020

piotradamczyk5 commented Jun 5, 2020 •

edited

Loading

adamfilipow92 commented Jun 5, 2020

adamfilipow92 left a comment

jan-goral Jun 5, 2020

piotradamczyk5 Jun 8, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

jan-goral Jun 5, 2020

piotradamczyk5 Jun 8, 2020

jan-goral Jun 5, 2020

piotradamczyk5 commented Jun 15, 2020

adamfilipow92 left a comment

#823 Added obfuscate option to dump shards #837

#823 Added obfuscate option to dump shards #837

Conversation

piotradamczyk5 commented Jun 5, 2020 • edited Loading

Test Plan

Checklist

codecov-commenter commented Jun 5, 2020 • edited Loading

Codecov Report

piotradamczyk5 commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamfilipow92 commented Jun 5, 2020

piotradamczyk5 commented Jun 5, 2020 • edited Loading

adamfilipow92 commented Jun 5, 2020

adamfilipow92 left a 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

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

piotradamczyk5 commented Jun 15, 2020

adamfilipow92 left a comment

Choose a reason for hiding this comment

piotradamczyk5 commented Jun 5, 2020 •

edited

Loading

codecov-commenter commented Jun 5, 2020 •

edited

Loading

piotradamczyk5 commented Jun 5, 2020 •

edited

Loading