-
Notifications
You must be signed in to change notification settings - Fork 118
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
#870 Added --directories-to-pull validation and avoid making request with empty toolStepResult #876
#870 Added --directories-to-pull validation and avoid making request with empty toolStepResult #876
Conversation
…with empty toolStepResult
I noticed some differnces on master and your branche. We have multiple-success yml
FIXED |
@@ -7,6 +7,7 @@ internal fun List<TestExecution>.createJUnitTestResult( | |||
withStackTraces: Boolean = false | |||
) = JUnitTestResult( | |||
testsuites = this | |||
.filterNullToolResultsStep() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I wonder why FTL is returning
null
fortoolResultsStep
? - Can we reproduce this error?
- Its safe to skip some tests execution and get correct results?
- Maybe flank should display proper warning about some unexpected behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I wonder why FTL is returning
null
fortoolResultsStep
?- Can we reproduce this error?
- Its safe to skip some tests execution and get correct results?
- Maybe flank should display proper warning about some unexpected behaviour?
AD 1. If matrix is invalid FTL returns null
as toolResultsStep
AD 2. This is more to avoid crashing flank in the future, so far, for now, I do not know how to reproduce this issue
AD 3. This is safe because tests results are based on a request made with toolResultsStep
which will not succeed without toolResultsStep
AD 4. Ideally, we should not trigger INVALID matrix request, but for now, we do not know which settings will cause matrix invalid (see point 2)
@@ -169,8 +169,7 @@ fun withGlobalExceptionHandling(block: () -> Int) { | |||
} | |||
|
|||
private fun SavedMatrix.logError() { | |||
println("More details are available at [${this.webLink}]") | |||
println(this.asPrintableTable()) | |||
println("Matrix is $state") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be no table output if flank throw FTLError
before MatrixResultsReport
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTLError
is thrown after printing table
listOf(
CostReport,
MatrixResultsReport
).map {
it.run(matrices, testSuite, printToStdout = true, args = args) >>>>>>>>>>>>> PRINTING TABLE
}
if (!matrices.allSuccessful()) {
listOf(
HtmlErrorReport
).map { it.run(matrices, testSuite, printToStdout = false, args = args) }
}
JUnitReport.run(matrices, testSuite?.apply {
if (ignoredTestCases.isNotEmpty()) {
testsuites?.add(ignoredTestCases.toJunitTestsResults())
}
}, printToStdout = false, args = args)
when {
args.fullJUnitResult -> processFullJunitResult(args, matrices, testShardChunks)
args.useLegacyJUnitResult -> processJunitXml(testSuite, args, testShardChunks)
}
matrices.validateMatrices(args.ignoreFailedTests) >>>>>>>>>>> FTLError
Fixes #870
Test Plan
NPE with missing
toolStepResult
was caused because we try to get results for an invalid matrix. Matrix was invalid because FTL rejects the request with invalid--directories-to-pull
value.To avoid this issue correct validation of this parameter was introduced, as well as filtering getting results for empty
toolStepResult
.Gcloud policies were applied to
--directories-to-pull
:/sdcard
or/data/local/tmp
a-zA-Z0-9_-./+
In order to test this issue, please use
--directories-to-pull
value which does not meet this 2 criteria.Checklist