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

Use API instead of XML for result parsing for android #666

Merged
merged 17 commits into from Mar 24, 2020

Conversation

@jan-gogo
Copy link
Collaborator

jan-gogo commented Mar 13, 2020

Fixes #618, Fixes #545

Checklist

  • Documented
  • Unit tested
  • release_notes.md updated
@jan-gogo jan-gogo self-assigned this Mar 13, 2020
// it is making api calls under the hood
testExecution.createTestExecutionData()
}
}.map { deferred ->

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 13, 2020

Collaborator

I think we can use .awaitAll() instead, just for simplification

args: IArgs
): JUnitTestResult = GcTestMatrix.refresh(
// Android uses server side sharding, it results only one matrix, so we can simply get first.
matrices.map.values.first().matrixId,

This comment has been minimized.

Copy link
@bootstraponline

bootstraponline Mar 13, 2020

Contributor

iOS doesn't use server side sharding right now. Is it possible to use the API for both Android & iOS?

Edit: I see that processXmlFromApi is only used for Android right now. That makes sense.

import java.io.File
import kotlin.system.exitProcess

object TmpV2 {

This comment has been minimized.

Copy link
@bootstraponline

bootstraponline Mar 13, 2020

Contributor

we should probably delete TmpV2 and turn this into a proper JUnit test. What do you think?

This comment has been minimized.

Copy link
@bootstraponline

bootstraponline Mar 13, 2020

Contributor

Also probably delete the original tmp

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 16, 2020

Author Collaborator

Of Course I will do that. This is only draft of implementation that I tested manually and looks correct. I now need to update unit tests, cleanup a little, and It will be ready for review.

package ftl.reports.api

import com.google.api.services.toolresults.model.Duration
import java.util.*

This comment has been minimized.

Copy link
@bootstraponline

bootstraponline Mar 14, 2020

Contributor

I think we have a lint rule about no wildcard imports.

println(" - OK")

print("writing api results to file")
File(JUNIT_REPORT_FILE).writeText(jUnitTestResult.xmlToString())

This comment has been minimized.

Copy link
@bootstraponline

bootstraponline Mar 14, 2020

Contributor

To test this, run an Android app with multiple tests using server side sharding that has flaky tests. Use num-flaky-test-attempts set to a non-zero value such as 2. Then compare the JUnit XML file that FTL is creating vs the one we create via the API.

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 16, 2020

Author Collaborator

I was testing exactly that case. I know there is a difference between old and new JUnitReport.xml.
Old:

<?xml version='1.0' encoding='UTF-8' ?>
<testsuites>
  <testsuite name="NexusLowRes-28-en-portrait#" tests="3" failures="0" errors="0" skipped="0" time="3.103" timestamp="2020-03-10T17:12:24" hostname="localhost">
    <testcase name="test0" classname="com.example.test_app.InstrumentedTest" time="1.161">
      <webLink></webLink>
    </testcase>
    <testcase name="test1" classname="com.example.test_app.InstrumentedTest" time="1.034">
      <webLink></webLink>
    </testcase>
    <testcase name="test2" classname="com.example.test_app.InstrumentedTest" time="0.908">
      <webLink></webLink>
    </testcase>
  </testsuite>
</testsuites>

New:

<?xml version='1.0' encoding='UTF-8' ?>
<testsuites>
  <testsuite name="NexusLowRes-28-en-portrait" tests="1" failures="0" errors="0" skipped="0" time="1.161" timestamp="2020-03-10T17:07:54.422Z">
    <testcase name="test0" classname="com.example.test_app.InstrumentedTest" time="1.161"/>
  </testsuite>
  <testsuite name="NexusLowRes-28-en-portrait" tests="2" failures="0" errors="0" skipped="0" time="1.942" timestamp="2020-03-10T17:07:54.422Z">
    <testcase name="test1" classname="com.example.test_app.InstrumentedTest" time="1.034"/>
    <testcase name="test2" classname="com.example.test_app.InstrumentedTest" time="0.908"/>
  </testsuite>
</testsuites>
  1. In new JUnitReport suites reflects shards. I can fix it easily, but that was first what I achieved. I guess that maybe is better to expose some info about shards in JUnitReport. If you don't like that I will fix it.

  2. The timestamp is different yes. Old timestamp is simply timestamp of first of downloaded reports. New one is not correct I will fix it.

  3. I need to also fix weblinks.

This comment has been minimized.

Copy link
@bootstraponline

bootstraponline Mar 17, 2020

Contributor

makes sense!

@bootstraponline

This comment has been minimized.

Copy link
Contributor

bootstraponline commented Mar 14, 2020

When using Android Test Orchestrator, we'll want to verify the API test times match the times returned in the FTL XML file.

#557

@jan-gogo jan-gogo force-pushed the 545-junit-result-from-api branch 3 times, most recently from 7d905dc to 15d9f39 Mar 19, 2020
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 23, 2020

Codecov Report

Merging #666 into master will decrease coverage by 0.99%.
The diff coverage is 65.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #666      +/-   ##
============================================
- Coverage     77.10%   76.10%   -1.00%     
- Complexity      598      615      +17     
============================================
  Files            99      109      +10     
  Lines          2367     2528     +161     
  Branches        337      359      +22     
============================================
+ Hits           1825     1924      +99     
- Misses          335      381      +46     
- Partials        207      223      +16     
@jan-gogo jan-gogo force-pushed the 545-junit-result-from-api branch from 80ecae9 to 25e5867 Mar 23, 2020
@jan-gogo jan-gogo marked this pull request as ready for review Mar 23, 2020
Copy link
Collaborator

pawelpasterz left a comment

Great job! That is one big PR with lot of work. I am really impressed.

The only thing I can complain is multiple usage of runBlocking. Personally I don't like when this is used as kind of bridge between synchronous and asynchronous code (when the first one is invoked inside a coroutine). But TBH I don't know if we have better solution right now.

Again, great job!

return listOf(map["Model"], map["Version"], map["Locale"], map["Orientation"]).joinToString("-")
}

private fun List<JUnitTestCase>.sumTime() = this

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

I think this is redundant here but I guess it was left intentionally for sake of readability?

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

That's right, intentionally for readability. I usually use this for extension functions with expression body. Thats allow clear chain.

.getTestExecutions()
.createJUnitTestResult()

internal fun refreshTestMatrices(

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

I think we can make it private

}.awaitAll()
}

internal fun List<TestMatrix>.getTestExecutions(): List<TestExecution> = this

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

I think we can make it private

skipped = if (testCase.status == "skipped") null else "absent"
).apply {
if (errors != null || failures != null) {
webLink = getWebLink(toolResultsStep, testCase.testCaseId)

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

👍


private fun TestExecution.createTestExecutionData(): TestExecutionData {
val response: ListTestCasesResponse = GcToolResults.listTestCases(toolResultsStep)
val step: Step = GcToolResults.getStepResult(toolResultsStep)

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

Both GcToolResults.listTestCases and GcToolResults.getStepResult are making api calls and seems that they don't depend on each other. Do you think we could make async call for each?

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

Right, I will fix that.


fun Long.formatUtcDate() = utcDateFormat.format(this)!!

var TestCase.flaky: Boolean by mutableMapProperty { false }

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

👍

@@ -0,0 +1,34 @@
package ftl.reports.api

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

We already have Utils.kt file, can we rename this one to sth like ApiUtils or somehow different? Just for sake of clarity.

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

Looks like you don't care about packages :). I guess adding part of package as prefix for file is redundant.

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 24, 2020

Collaborator

Yeah, true. Sometimes I am kind of bitchy :D ¯_(ツ)_/¯

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

Easy. You are doing your job. It is always better to express your opinion than to do nothing. There are no ultimate ways, everything we do is based on consensus.

@@ -15,7 +15,7 @@ private val xmlMapper = XmlMapper(xmlModule)
.registerModules(KotlinModule())
.configure(FAIL_ON_UNKNOWN_PROPERTIES, false)

private val xmlPrettyWriter = xmlMapper.writerWithDefaultPrettyPrinter()
internal val xmlPrettyWriter = xmlMapper.writerWithDefaultPrettyPrinter()

private fun xmlBytes(path: Path): ByteArray {
if (!path.toFile().exists()) RuntimeException("$path doesn't exist!")

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

I am totally aware it is not part of your PR (so feel free to skip this comment). I think there is throw missing

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

Good checkup. I can fix it in this PR, no problem.

@@ -29,11 +32,11 @@ data class JUnitTestSuite(
val timestamp: String?, // String. Android only

@JacksonXmlProperty(isAttribute = true)
val hostname: String, // String.
val hostname: String? = null, // String.

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

Just for my understanding, are we ok with possible null here? Previous implementation did not predicted such value. Thanks!

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

I have asked @bootstraponline about that, he said #545 (comment). So I decide to change it to nullable here, because I didn't find hostname in API

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 24, 2020

Collaborator

Got it, thanks for explanation

Paths.get(args.localResultDir, FtlConstants.configFileName(args))
} else {
val configFilePath = if (args.useLocalResultDir())
Paths.get(args.localResultDir, FtlConstants.configFileName(args)) else

This comment has been minimized.

Copy link
@pawelpasterz

pawelpasterz Mar 23, 2020

Collaborator

TBH I don't like it when I need to read whole line to find out there is else at the end. But it is matter of personal preferences :)

This comment has been minimized.

Copy link
@jan-gogo

jan-gogo Mar 24, 2020

Author Collaborator

TBH I don't like if else and prefer using when but our detect config forces to use if else in cases like this. Anyway I like that kind of formatting cause of clear indentation same for two similar parts.

@jan-gogo jan-gogo dismissed stale reviews from pawelpasterz and bootstraponline via 3d5e8bf Mar 24, 2020
@pawelpasterz pawelpasterz self-requested a review Mar 24, 2020
@jan-gogo jan-gogo force-pushed the 545-junit-result-from-api branch from e7bd0b0 to 169c7b5 Mar 24, 2020
@jan-gogo jan-gogo merged commit f114c83 into master Mar 24, 2020
3 checks passed
3 checks passed
Validation
Details
Validation
Details
Bitrise Summary
Details
@jan-gogo jan-gogo deleted the 545-junit-result-from-api branch Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.