Fix Logcat Parsing by christopherperry · Pull Request #133 · gojuno/composer

christopherperry · 2018-04-05T09:47:04Z

Fix for #129

Might need more testing.

artem-zinnatullin · 2018-04-05T18:35:52Z

composer/src/main/kotlin/com/gojuno/composer/TestRun.kt

                        // Was not able to find more reliable solution to capture logcat per test.
-                        val startedTest: Pair<String, String>? = newline.parseTestClassAndName("TestRunner: started: ")
-                        val finishedTest: Pair<String, String>? = newline.parseTestClassAndName("TestRunner: finished: ")
+                        val startedTest: Pair<String, String>? = newline.parseTestClassAndName("""(.*TestRunner.*: started: )(.*)\((.*)\)""".toRegex())


Any chance to solve it without Regex?

My main concern is performance and readability :)

I wondered about perf, but personally find it very readable. It's easy to see the capture groups and destructuring makes it clear exactly what were pulling out of the log line.

I can give a non-regex implementation a shot too.

If it's not hard, please try, would love to compare!

Yeah, I'd compare it with non-regex solution, too

How about:

fun String.parseTestClassAndName(): Pair<String, String>? { val tokens = split(':') if (tokens.size != 3) return null if (tokens[0].trimStart('I', '/').startsWith("TestRunner")) { if (tokens[1] == " started" || tokens[1] == " finished") { return tokens[2].substringAfter("(").removeSuffix(")") to tokens[2].substringBefore("(").trim() } } return null }

I'm not sure about the performance of split(). Since it's a single character I'm assuming it's O(n) where n is the length of the string.

Maybe the first check should be

if (!trimStart('I', '/').startsWith("TestRunner"))

so we can avoid O(n) on every log.

fun String.parseTestClassAndName(): Pair<String, String>? { if (!trimStart('I', '/').startsWith("TestRunner")) return null val tokens = split(':') if (tokens.size != 3) return null if (tokens[1] == " started" || tokens[1] == " finished") { return tokens[2].substringAfter("(").removeSuffix(")") to tokens[2].substringBefore("(").trim() } return null }

artem-zinnatullin · 2018-04-05T18:36:56Z

composer/src/main/kotlin/com/gojuno/composer/TestRun.kt

-                                else -> it.substringAfter("(").removeSuffix(")") to it.substringBefore("(")
-                            }
+                        fun String.parseTestClassAndName(regex: Regex): Pair<String, String>? {
+                            return regex.find(this)?.destructured?.let { (_, testMethod, testClass) -> Pair(testMethod, testClass) }


Hm, typically to improve performance you create Pattern by Pattern.compile("…") once and use that

artem-zinnatullin · 2018-04-05T19:05:44Z

Btw, looks like you're committing with wrong git config, your commit doesn't have your avatar

You can check if commit email matches expected that is attached to github account:

git log

And if it's not, fix it like that:

git config user.email "correct@email.com"
git commit -a --amend --reset-author

artem-zinnatullin · 2018-04-05T20:12:01Z

Ah, also please add test case(s) that show how old parsing failed and new one fixes that problem! Thanks :)

christopherperry · 2018-04-05T23:16:04Z

@artem-zinnatullin Please have another look. I optimized and added some tests.

artem-zinnatullin · 2018-04-06T00:04:19Z

composer/src/main/kotlin/com/gojuno/composer/TestRun.kt

        }

-private fun saveLogcat(adbDevice: AdbDevice, logsDir: File): Observable<Pair<String, String>> = Observable
+class LogLineParser {


You don't need class :)

As you can see, Composer is class-less other than data-classes, just drop a private top-level function :)

I can't test it if it's private.

artem-zinnatullin · 2018-04-06T00:12:34Z

composer/src/test/kotlin/com/gojuno/composer/LogLineParserSpec.kt

+
+    context("parse test class and name") {
+
+        it("parses old TestRunner start logs") {


Maybe give better name? "old" doesn't really explain what is old and what is new

Also, you can extract subject under test which is a log line that you parse into a separate variable for better test structure

artem-zinnatullin · 2018-04-06T00:13:03Z

composer/src/main/kotlin/com/gojuno/composer/TestRun.kt

+        val tokens = logLine.split(':')
+        if (tokens.size != 3) return null
+
+        if (tokens[1] == " started" || tokens[1] == " finished") {


this looks vulnerable to spaces, maybe trim?

artem-zinnatullin · 2018-04-06T00:13:28Z

composer/src/main/kotlin/com/gojuno/composer/TestRun.kt


-private fun saveLogcat(adbDevice: AdbDevice, logsDir: File): Observable<Pair<String, String>> = Observable
+class LogLineParser {
+    fun parseTestClassAndName(logLine: String): Pair<String, String>? {


I honestly like this non-regexp version better, much more understandable!

I have a bug, I forgot the "newer" logs have a ton of stuff before "TestRunner". I'll submit an update later.

Sure, so far it looks really good, keep it up 👍

christopherperry · 2018-04-07T18:36:43Z

bump

artem-zinnatullin

Awesome work!

artem-zinnatullin · 2018-04-08T23:32:39Z

composer/src/test/kotlin/com/gojuno/composer/LogLineParserSpec.kt

+            }
+
+            it("extracts test class and method") {
+                assertThat(args).isEqualTo(Pair("com.example.SampleClass", "someTestMethod"))


nit: you can use infix function to create pair, it'll be little bit shorter

"com.example.SampleClass" to "someTestMethod"

artem-zinnatullin · 2018-04-08T23:33:24Z

composer/src/test/kotlin/com/gojuno/composer/LogLineParserSpec.kt

+import org.jetbrains.spek.api.dsl.context
+import org.jetbrains.spek.api.dsl.it
+
+class LogLineParserSpec : Spek({


This is great!

artem-zinnatullin · 2018-04-08T23:35:35Z

Just in case, I believe when GitHub will "Squash and Merge" it'll use first commit as base and might not associate it with your GitHub account, but I'm not sure

Possible solution: rebase with --reset-author and force push, but it's up2u :)

christopherperry · 2018-04-11T18:16:24Z

Fixed. Thanks @artem-zinnatullin

artem-zinnatullin · 2018-04-16T17:51:28Z

@yunikkk can you PTAL and merge? :)

evgengal · 2018-04-17T06:56:37Z

Please, merge!)

christopherperry · 2018-04-19T04:45:43Z

@evgengal I just built a jar and deployed that so I wouldn't have to wait. Put a fix for screenshots not working in there too, but it's kind of a special case.

yunikkk · 2018-04-19T07:46:50Z

@christopherperry merged and released 0.3.3 with it, thanks!

CristianGM · 2018-06-14T14:04:17Z

Well...now it doesn't work on emulators...
because of:
https://github.com/gojuno/composer/blob/master/composer/src/main/kotlin/com/gojuno/composer/TestRun.kt#L166

/storage/emulated/0 doesn't exist in emulators with API 22...

Sloy · 2018-06-14T14:30:34Z

I just created a new issue for what @CristianGM is commenting, in case you'd like to treat it as a different issue: #151

yunikkk · 2018-06-14T21:18:49Z

@CristianGM @Sloy thanks for reporting, adding fix.

artem-zinnatullin reviewed Apr 5, 2018

View reviewed changes

artem-zinnatullin requested review from artem-zinnatullin and yunikkk April 5, 2018 19:04

artem-zinnatullin added the enhancement label Apr 5, 2018

artem-zinnatullin reviewed Apr 6, 2018

View reviewed changes

artem-zinnatullin approved these changes Apr 8, 2018

View reviewed changes

artem-zinnatullin assigned yunikkk Apr 8, 2018

christopherperry added 4 commits April 11, 2018 11:10

Fix Logcat Parsing

844cfc1

optimize log parsing

29891b1

update parsing to fix issue with long prefix parsing

ab86ba6

use shorthand for pair

389e31b

christopherperry force-pushed the cp/fix-logcat-parsing branch from ce4d929 to 389e31b Compare April 11, 2018 18:12

fix screenshot root, oops

91f7ac0

artem-zinnatullin approved these changes Apr 16, 2018

View reviewed changes

yunikkk approved these changes Apr 19, 2018

View reviewed changes

yunikkk merged commit 5319795 into gojuno:master Apr 19, 2018


		context("parse test class and name") {

		it("parses old TestRunner start logs") {

Conversation

christopherperry commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherperry Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-zinnatullin commented Apr 5, 2018

Uh oh!

artem-zinnatullin commented Apr 5, 2018

Uh oh!

christopherperry commented Apr 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherperry commented Apr 7, 2018

Uh oh!

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-zinnatullin commented Apr 8, 2018

Uh oh!

christopherperry commented Apr 11, 2018

Uh oh!

artem-zinnatullin commented Apr 16, 2018

Uh oh!

evgengal commented Apr 17, 2018

Uh oh!

christopherperry commented Apr 19, 2018

Uh oh!

yunikkk commented Apr 19, 2018

Uh oh!

CristianGM commented Jun 14, 2018

Uh oh!

Sloy commented Jun 14, 2018

Uh oh!

yunikkk commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

christopherperry Apr 5, 2018 •

edited

Loading