Added printing outcome details #862
Conversation
piotradamczyk5
force-pushed
the
#829-added-outcome-details
branch
from
a70adf1 to
5f9c388
Compare
piotradamczyk5
requested review from
bootstraponline,
adamfilipow92,
jan-goral and
pawelpasterz
|
Let's make sure to test with gcloud CLI to ensure the table/outcomes match. |
The implementation is the same as for gcloud (from the code) |
|
Makes sense. |
|
When you cancel test on pending status OutcomeDetailFormatter Crash here is stack trace, i try cancel on running status and stack trace is the same Fixed |
piotradamczyk5
force-pushed
the
#829-added-outcome-details
branch
from
ea423a5 to
252771d
Compare
|
We have to check this PR very carefully. There is a lot of conditions and we don't want to get wrong output on some of them. |
| matrix.testExecutions.createTestExecutionDataListAsync() | ||
| .map { | ||
| FinishedTestMatrixData( | ||
| stepOutcome = GcToolResults.getExecutionResult(it.testExecution).outcome, |
There was a problem hiding this comment.
outcome might be null (it's java accessor)
There was a problem hiding this comment.
Right. Anyway this is not correct way to obtain flay test outcome, we have to fix it
| private fun updateFinishedMatrixData(matrix: TestMatrix) { | ||
| matrix.testExecutions.createTestExecutionDataListAsync() | ||
| .map { | ||
| FinishedTestMatrixData( |
There was a problem hiding this comment.
I am not fully convinced with this approach. First we map list to create list of FinishedTestMatrixData objects and with next step we destruct all of them and use its values.
I might be missing something here.
There was a problem hiding this comment.
Right it would be better to have function instead of data class:
private fun updateFinishedMatrixData(matrix: TestMatrix) =
matrix.testExecutions.createTestExecutionDataListAsync().forEach {
updateAllOutcomes(
stepOutcome = GcToolResults.getExecutionResult(it.testExecution).outcome,
isVirtualDevice = AndroidCatalog.isVirtualDevice(
it.testExecution.environment.androidDevice,
matrix.projectId.orEmpty()
),
testSuiteOverviewData = it.createTestSuitOverviewData(),
billableMinutes = it.step.testExecutionStep?.testTiming?.testProcessDuration?.seconds
?.let { testTimeSeconds -> Billing.billableMinutes(testTimeSeconds) }
)
}
private fun updateAllOutcomes(
stepOutcome: Outcome,
isVirtualDevice: Boolean,
testSuiteOverviewData: TestSuiteOverviewData?,
billableMinutes: Long?
) {
updateOutcome(stepOutcome)
updateOutcomeDetails(stepOutcome, testSuiteOverviewData)
billableMinutes?.let { updateBillableMinutes(it, isVirtualDevice) }
}
There was a problem hiding this comment.
I change the proposed name from updateAllOutcomes to updatedFinishedInfo, cause we update single outcome, details and minutes not all outcomes
jan-goral
dismissed
adamfilipow92’s stale review
We need to check this PR more carefully
| } | ||
|
|
||
| private fun getSuccessOutcomeDetails( | ||
| testSuiteOverviewData: TestSuiteOverviewData, |
There was a problem hiding this comment.
If test TestSuiteOverviewData will be receiver this method, will looks:
private fun TestSuiteOverviewData.getSuccessOutcomeDetails(
otherNativeCrash: Boolean
) = StringBuilder("$successCount test cases passed").apply {
if (skipped > 0) append(skippedMessage(skipped))
if (flakes > 0) append(flakesMessage(flakes))
if (otherNativeCrash) append(NATIVE_CRASH_MESSAGE)
}.toString()
and getDetails also will be shorter:
fun Outcome.getDetails(
testSuiteOverviewData: TestSuiteOverviewData?
) = when (summary) {
success, flaky -> testSuiteOverviewData?.getSuccessOutcomeDetails(successDetail?.otherNativeCrash ?: false)
failure -> failureDetail.getFailureOutcomeDetails(testSuiteOverviewData)
inconclusive -> inconclusiveDetail.formatOutcomeDetails()
skipped -> skippedDetail.formatOutcomeDetails()
unset -> "unset"
else -> "unknown"
}
There was a problem hiding this comment.
The code looks great. But I noticed that flank in difference to gcloud is treating flaky outcome as success. That was introduced long time ago https://github.com/Flank/flank/pull/554/files#diff-0863ffce1d9b2a9c25d2c95e3d4c6022R104. @bootstraponline What was the reason? IMO flaky tests are worst than failed.
piotradamczyk5
force-pushed
the
#829-added-outcome-details
branch
from
742d2df to
b5fb892
Compare
What happens if we treat flaky tests as flaky? For Flank, I think we need to consider the run to be successful for flaky tests. A flaky test is one that passed at least once. Flank should never error when all tests passed, even if they're flaky and passed on retry. There's a larger roadmap item around metrics and a system to manage flakiness called guardian. I'll share more on that soon. |
|
We have bug with displaying proper And the summary outcome is |
If the exit code of Flank is still 0 when the outcome is corrected, and the Jenkins JUnit XML plugin still parses the results as successful then I think we can remove this flaky = success line. |
piotradamczyk5
force-pushed
the
#829-added-outcome-details
branch
from
d54c30a to
bae8463
Compare
| @@ -3,6 +3,7 @@ package ftl.util | |||
| enum class SystemOutColor(val ansiCode: String) { | |||
| DEFAULT("\u001B[0m"), | |||
| RED("\u001B[31m"), | |||
| BLUE("\u001B[34m"), | |||
There was a problem hiding this comment.
I known you add only blue color but probably ansiCode can be private.
There was a problem hiding this comment.
cannot be cause it is used later in printing informations
| TableColumn(OUTCOME_DETAILS_COLUMN_HEADER, map { it.outcomeDetails }, OUTCOME_DETAILS_COLUMN_SIZE) | ||
| TableColumn(OUTCOME_COLUMN_HEADER, map { it.outcome }, dataColor = map { getOutcomeColor(it.outcome) }), | ||
| TableColumn(MATRIX_ID_COLUMN_HEADER, map { it.matrixId }), | ||
| TableColumn(OUTCOME_DETAILS_COLUMN_HEADER, mapNotNull { it.outcomeDetails }) | ||
| ) | ||
|
|
||
| private fun getOutcomeColor(outcome: String): SystemOutColor { |
There was a problem hiding this comment.
Its only suggestion but, maybe something like:
private fun getOutcomeColor(outcome: String): SystemOutColor =
// inconclusive is treated as a failure, flaky as a success
when (outcome) {
failure -> SystemOutColor.RED
success -> SystemOutColor.GREEN
flaky -> SystemOutColor.BLUE
else -> SystemOutColor.DEFAULT
}
There was a problem hiding this comment.
changed 👍 and removed deprecated comment
| ## Possible outputs | ||
| Numbers are representing `OUTCOME` column, points are representing `TEST DETAILS` column. | ||
| 1. `success | flaky` | ||
| * `${1} test cases passed | ${2} skipped | ${3} flakes | (Native crash) | ---` |
There was a problem hiding this comment.
| * `${1} test cases passed | ${2} skipped | ${3} flakes | (Native crash) | ---` | |
| * `${1} test cases passed | ${2} skipped | ${3} flakes | (Native crash) | --- | Robo test` |
|
Let's embed the uml diagram in the markdown file. Here's an example: |
test_runner/src/test/kotlin/ftl/fixtures/test_app_cases/flank-multiple-flaky.yml
Outdated
Show resolved
Hide resolved
test_runner/src/main/kotlin/ftl/reports/api/data/TestSuiteOverviewData.kt
Show resolved
Hide resolved
piotradamczyk5
force-pushed
the
#829-added-outcome-details
branch
from
71d0bad to
d911dc5
Compare
Fixes #829
Test Plan
Result table displayed correct outcome details according to outcome status
Checklist