Remove failure nodes from tests that passed on retry #798
Conversation
release_notes.md
Outdated
| @@ -1,7 +1,7 @@ | |||
| ## next (unreleased) | |||
|
|
|||
| - [#779](https://github.com/Flank/flank/pull/779) Print retries & display additional info. ([jan-gogo](https://github.com/jan-gogo)) | |||
| - | |||
| - [#796](https://github.com/Flank/flank/issues/796) Ability to generate XML results with passing re-runs removed. ([adamfilipow92](https://github.com/adamfilipow92)) | |||
There was a problem hiding this comment.
Ability to generate XML results with passing re-runs removed.
In the issue, the problem described is a passing flaky test will have failure nodes. What we want to do is remove failure nodes from flaky tests so that the JUnit CI plugin passes.
I'd reword this to be something like: Remove failure nodes from tests that passed on retry so that Jenkins JUnit plugin marks them as successful.
There was a problem hiding this comment.
Thanks I updated description.
| <testsuite name="" tests="1" failures="1" errors="0" skipped="0" time="2.676" timestamp="2020-05-19T14:40:18" hostname="localhost"> | ||
| <properties/> | ||
| <testcase name="test" classname="com.example.test_app.InstrumentedTest" time="1.516"> | ||
| <failure> |
There was a problem hiding this comment.
Let's use an example from the real XML linked to on the Slack thread from the issue.
I like taking bug reports and then turning them into tests to ensure they don't regress in the future. 🙂
There was a problem hiding this comment.
I think starting work from falling test is good idea. For me it can help understand problem and it's faster than running full path to reproduce problem every time and as you say we have regress protection. 🙂
|
Thanks for opening a draft pull request, it's great to get feedback quickly. 🙂 |
adamfilipow92
force-pushed
the
#796-Ability-to-generate-XML-results-with-passing-re-runs-removed-#796
branch
from
8f9d900 to
046e166
Compare
|
I've been thinking about the use case more. The current JUnit XML contains all the information which is great. This is used by tools to track test data over time. The downside is Jenkins gets confused. By default, Flank should probably produce two JUnit XMLs. One for Jenkins to read, and another that has the full information that can be uploaded into a test analytics platform. |
|
Right, I will keep original file and produce additional on gcs and on local fs. Additional file will be cleaned from fail nodes when one of tries pass. |
|
I noticed we have a different (probably) merge results in gcs and local fs. Maybe we should have same results in gcs and local storage? The main difference: in local file we have only one failure node and different time in test case. and local JUnitReport.xml
|
I think our merge should ideally match the gcs merge. Maybe Flank is not merging all the information correctly? The times from the API are slightly off from what FTL saves in the XML so that is expected. |
| import ftl.cli.firebase.test.ios.IosRunCommand | ||
| import ftl.config.Device | ||
| import ftl.config.FtlConstants | ||
| import ftl.ios.IosCatalog | ||
| import ftl.ios.Xctestrun | ||
| import ftl.run.status.asOutputStyle | ||
| import ftl.util.FlankFatalError | ||
| import ftl.util.FlankTestMethod | ||
| import ftl.util.* |
There was a problem hiding this comment.
I think we have a lint rule against wildcard imports?
There was a problem hiding this comment.
Yes, but I not enabling check when pull request is draft. Tell me please if I should commit only checked code even if pull request is a draft. I want make all right :)
There was a problem hiding this comment.
I think if you adjust your IDE settings then the problem will go away.
It's fine to have code that's not ready yet for a draft PR. 🙂
|
|
||
|
|
||
| @CommandLine.Option( | ||
| names = ["--ci-junit-result"], |
There was a problem hiding this comment.
I think since the default use case is CI, we probably want to generate this file by default. I'm not sure we need to enable the filename to be customized, we're not providing that option for other generated files.
There was a problem hiding this comment.
Alright, I will change it.
| file = args.ciJUnitResultFile!!, | ||
| fileBytes = testResult.xmlToString().toByteArray(), | ||
| rootGcsBucket = args.resultsBucket, | ||
| runGcsPath = args.resultsDir!! |
There was a problem hiding this comment.
Is there a better way to handle this without force unwraps? I think the error message might be confusing if the value ends up being null.
|
To maximize compatibility, I think we should default to writing |
But what with merged file generated on gcs? It's generated outside flank by default. When we add this option we will be have locally one file without fail nodes but on gcs we will have two files (merged and cleared). It will be fine behaviour? |
There are no conflicts between those two files due to different names. I don't see any reason to touch the xml result generated outside flank |
I don't want touching gcs result. I think about keeping in default two results locally and flag for turn off full result. It can help avoid some user confusion when we keep same amount of results locally and on gcs. |
|
Here's my understanding of the current state and the desired state. Let me know your thoughts. 🙂 Current State$ flank --version
version: local_snapshot
revision: e95ac5c67e9c0ae576922845ea2760488a5267a0gcloud:
app: ../test_app/apks/app-debug.apk
test: ./src/test/kotlin/ftl/fixtures/tmp/apk/app-debug-androidTest_155.apk
flank:
max-test-shards: 2Google Cloud Storage
Flank
Desired StateGoogle Cloud Storage
Flank
Flank should use the API to merge all the There's a ticket to upload all the Flank generated reports to the GCS bucket, however that can be resolved in a dedicated pull request. #750 |
|
I thinking about JUnitReportFull as local equivalent of
Now I have done upload |
adamfilipow92
force-pushed
the
#796-Ability-to-generate-XML-results-with-passing-re-runs-removed-#796
branch
from
40cffae to
9f516e9
Compare
Codecov Report
@@ Coverage Diff @@
## master #798 +/- ##
============================================
- Coverage 78.82% 78.22% -0.60%
+ Complexity 669 667 -2
============================================
Files 148 149 +1
Lines 2966 3004 +38
Branches 431 437 +6
============================================
+ Hits 2338 2350 +12
- Misses 354 375 +21
- Partials 274 279 +5 |
|
So to ensure we are on the same page, #750 will be resolved within this PR as well? |
test_runner/src/main/kotlin/ftl/cli/firebase/test/CommonRunCommand.kt
Outdated
Show resolved
Hide resolved
test_runner/src/main/kotlin/ftl/cli/firebase/test/android/AndroidRunCommand.kt
Outdated
Show resolved
Hide resolved
test_runner/src/main/kotlin/ftl/reports/api/PrepareForJUnitResult.kt
Outdated
Show resolved
Hide resolved
test_runner/src/main/kotlin/ftl/reports/api/PrepareForJUnitResult.kt
Outdated
Show resolved
Hide resolved
| @@ -24,5 +21,23 @@ object JUnitReport : IReport { | |||
| } else { | |||
| write(matrices, output, args) | |||
| } | |||
|
|
|||
| result?.let { | |||
| uploadToGcStorage(it, args) | |||
There was a problem hiding this comment.
Why not call GcStorage.uploadCiJUnitXml directly and pass reportPath(matrices, args).let(::File).name as fileName argument?. Is simpler, there is no needs for override fun reportPath, private fun uploadToGcStorage and val fileName.
There was a problem hiding this comment.
@jan-gogo you thinking about something like:
GcStorage.uploadCiJUnitXml(result, args, reportPath(matrices, args).let { ::File }.name)
?
I think we don't need execute reportPath because it's inside make additional work
check
fun reportPath(matrices: MatrixMap, args: IArgs): String {
val path = resolveLocalRunPath(matrices, args)
return Paths.get(path, reportName() + extension).toString()
}
we dont need val path = resolveLocalRunPath(matrices, args)
so maybe make something like:
private val fileName = "${reportName()}$extension"
override fun run(matrices: MatrixMap, result: JUnitTestResult?, printToStdout: Boolean, args: IArgs) {
if(result == null){
return
}
val output = result.xmlToString()
if (printToStdout) {
print(output)
} else {
write(matrices, output, args)
}
GcStorage.uploadCiJUnitXml(result, args, fileName)
}
or
private fun createReportFileName() = "${reportName()}$extension"
override fun run(matrices: MatrixMap, result: JUnitTestResult?, printToStdout: Boolean, args: IArgs) {
if(result == null){
return
}
val output = result.xmlToString()
if (printToStdout) {
print(output)
} else {
write(matrices, output, args)
}
GcStorage.uploadCiJUnitXml(result, args, createReportFileName())
}
Tell me what do you think please.
There was a problem hiding this comment.
not GcStorage.uploadCiJUnitXml(result, args, reportPath(matrices, args).let { ::File }.name)
but GcStorage.uploadCiJUnitXml(result, args, reportPath(matrices, args).let(::File).name)
I know that it make additional operations, but it's small impact on performance, and gives less additional code to maintain.
| @@ -10,3 +10,11 @@ internal fun List<TestExecution>.createJUnitTestResult() = JUnitTestResult( | |||
| .createJUnitTestSuites() | |||
| .toMutableList() | |||
| ) | |||
|
|
|||
| internal fun List<TestExecution>.createJUnitTestResultForCi() = JUnitTestResult( | |||
There was a problem hiding this comment.
Instead of duplicating code here and in 2 other places you can do something like this:
internal fun List<TestExecution>.createJUnitTestResult(
withStackTraces: Boolean = true
) = JUnitTestResult(
testsuites = this
.createTestExecutionDataListAsync()
.prepareForJUnitResult()
.let { executionDataList ->
if (withStackTraces) executionDataList
else executionDataList.removeStackTraces()
}
.createJUnitTestSuites()
.toMutableList()
)
I think replacing 3 functions for argument + condition is good trade off. Less duplicated code to maintain.
There was a problem hiding this comment.
Right, I will change it.
| @@ -1,3 +1,4 @@ | |||
| gcloud: | |||
| test: ./src/test/kotlin/ftl/fixtures/tmp/earlgrey_example.zip | |||
| xctestrun-file: ./src/test/kotlin/ftl/fixtures/tmp/EarlGreyExampleSwiftTests_iphoneos13.4-arm64e.xctestrun | |||
| results-dir: test_dir | |||
…nodes like on merge in gsc. When save full result not call api second time
adamfilipow92
force-pushed
the
#796-Ability-to-generate-XML-results-with-passing-re-runs-removed-#796
branch
from
d1297b2 to
cab9686
Compare
This pr have started review so I prefer solve #750 in other pr. As you say smaller pr == better. But it's only my point of view. So tell me please what you think. |
bootstraponline
changed the title
| @@ -11,6 +11,10 @@ internal fun List<TestExecutionData>.prepareForJUnitResult(): List<TestExecution | |||
| .reduceToPrimarySteps() | |||
| .reduceTestCases() | |||
|
|
|||
| internal fun List<TestExecutionData>.prepareJUnitResultForCi() = this | |||
| @@ -199,4 +199,10 @@ abstract class CommonRunCommand { | |||
| "which don't support ansi codes, to avoid corrupted output use `single` or `verbose`."] | |||
| ) | |||
| var outputStyle: String? = null | |||
|
|
|||
| @CommandLine.Option( | |||
| names = ["--full-junit-result"], | |||
There was a problem hiding this comment.
When you are adding new option you should update also flank.yml, flank.ios.yml and README.md.
adamfilipow92
deleted the
#796-Ability-to-generate-XML-results-with-passing-re-runs-removed-#796
branch
Fixes #796
Checklist