[K/N] Fix approach of expected failures caused by outputCheck fail.

`Expected failure flag` was wrongly included in list of test checks. However, the nature of checks and this flag is different: - any test check can pronounce the test as failed. - `Expected failure flag` must invert the accumulated effect of all checks. Having flag as an additional check cannot catch usecase, when test is failed only by producing wrong stdout. This commit extracts this flag out of check list. ^KT-59288
2023-11-29 12:24:21 +01:00
parent bb8a7b6795
commit 85882413db
10 changed files with 123 additions and 130 deletions
@@ -256,7 +256,6 @@ class ObjCToKotlinSteppingInLLDBTest : AbstractNativeSimpleTest() {
            checks = TestRunChecks(
                executionTimeoutCheck = ExecutionTimeout.ShouldNotExceed(testRunSettings.get<Timeouts>().executionTimeout),
                exitCodeCheck = TestRunCheck.ExitCode.Expected(0),
-                expectedFailureCheck = null,
                outputDataFile = null,
                outputMatcher = spec.let { TestRunCheck.OutputMatcher { output -> spec.checkLLDBOutput(output, testRunSettings.get()) } },
                fileCheckMatcher = null,
@@ -190,6 +190,7 @@ internal class TestCase(
    val checks: TestRunChecks,
    val extras: Extras,
    val fileCheckStage: String? = null, // KT-62157: TODO move it to extras
+    val expectedFailure: Boolean = false,
 ) {
    sealed interface Extras
    class NoTestRunnerExtras(val entryPoint: String, val inputDataFile: File? = null, val arguments: List<String> = emptyList()) : Extras
@@ -124,7 +124,7 @@ private class ExtTestDataFile(
        structureFactory.ExtTestDataFileStructure(testDataFile, allSourceTransformers)
    }

-    private val isIgnoredTarget: Boolean = settings.isIgnoredTarget(structure.directives)
+    private val isExpectedFailure: Boolean = settings.isIgnoredTarget(structure.directives)

    private val testDataFileSettings by lazy {
        val optIns = structure.directives.multiValues(OPT_IN_DIRECTIVE)
@@ -197,7 +197,7 @@ private class ExtTestDataFile(
    private fun determineIfStandaloneTest(): Boolean = with(structure) {
        if (directives.contains(NATIVE_STANDALONE_DIRECTIVE)) return true
        if (directives.contains(FILECHECK_STAGE.name)) return true
-        if (isIgnoredTarget) return true
+        if (isExpectedFailure) return true
        // To make the debug of possible failed testruns easier, it makes sense to run dodgy tests alone
        if (directives.contains(IGNORE_NATIVE.name) ||
            directives.contains(IGNORE_NATIVE_K1.name) ||
@@ -524,10 +524,11 @@ private class ExtTestDataFile(
            modules = modules,
            freeCompilerArgs = assembleFreeCompilerArgs(),
            nominalPackageName = testDataFileSettings.nominalPackageName,
+            expectedFailure = isExpectedFailure,
            checks = TestRunChecks.Default(timeouts.executionTimeout).copy(
                fileCheckMatcher = fileCheckStage?.let { TestRunCheck.FileCheckMatcher(settings, testDataFile) },
-                exitCodeCheck = TestRunCheck.ExitCode.Expected(0).takeUnless { isIgnoredTarget },
-                expectedFailureCheck = TestRunCheck.ExpectedFailure.takeIf { isIgnoredTarget },
+                // for expected failures, it does not matter, which exit code would the process have, since test might fail with other reasons
+                exitCodeCheck = TestRunCheck.ExitCode.Expected(0).takeUnless { isExpectedFailure },
            ),
            fileCheckStage = fileCheckStage,
            extras = WithTestRunnerExtras(runnerType = TestRunnerType.DEFAULT)
@@ -221,7 +221,6 @@ internal class StandardTestCaseGroupProvider : TestCaseGroupProvider {
            checks = TestRunChecks(
                computeExecutionTimeoutCheck(settings, expectedTimeoutFailure),
                computeExitCodeCheck(testKind, registeredDirectives, location),
-                expectedFailureCheck = null,
                computeOutputDataFileCheck(testDataFile, registeredDirectives, location),
                lldbSpec?.let { OutputMatcher { output -> lldbSpec.checkLLDBOutput(output, settings.get()) } },
                fileCheckMatcher = null,
@@ -10,6 +10,7 @@ import com.intellij.openapi.util.text.StringUtilRt.convertLineSeparators
 import kotlinx.coroutines.*
 import org.jetbrains.kotlin.konan.target.Architecture
 import org.jetbrains.kotlin.konan.target.KonanTarget
+import org.jetbrains.kotlin.konan.test.blackbox.support.TestCaseId
 import org.jetbrains.kotlin.konan.test.blackbox.support.runner.AbstractRunner.AbstractRun
 import org.jetbrains.kotlin.konan.test.blackbox.support.runner.TestRunCheck.ExecutionTimeout
 import org.jetbrains.kotlin.konan.test.blackbox.support.runner.TestRunCheck.ExitCode
@@ -19,7 +20,6 @@ import org.jetbrains.kotlin.konan.test.blackbox.support.settings.OptimizationMod
 import org.jetbrains.kotlin.konan.test.blackbox.support.settings.configurables
 import org.jetbrains.kotlin.konan.test.blackbox.support.util.TestOutputFilter
 import org.jetbrains.kotlin.util.capitalizeDecapitalize.toUpperCaseAsciiOnly
-import org.junit.jupiter.api.Assertions.fail
 import java.io.*
 import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.time.*
@@ -96,117 +96,123 @@ internal abstract class AbstractLocalProcessRunner<R>(protected val checks: Test
 internal abstract class LocalResultHandler<R>(
    runResult: RunResult,
    private val visibleProcessName: String,
-    protected val checks: TestRunChecks
+    protected val checks: TestRunChecks,
+    private val testCaseId: TestCaseId?,
+    private val expectedFailure: Boolean,
 ) : AbstractResultHandler<R>(runResult) {
    override fun handle(): R {
-        checks.forEach { check ->
-            when (check) {
-                is ExecutionTimeout.ShouldNotExceed -> verifyExpectation(runResult.hasFinishedOnTime) {
-                    "Timeout exceeded during test execution."
-                }
-                is ExecutionTimeout.ShouldExceed -> verifyExpectation(!runResult.hasFinishedOnTime) {
-                    "Test is expected to fail with exceeded timeout, which hasn't happened."
-                }
-                is TestRunCheck.ExpectedFailure -> {
-                    val testReport = runResult.processOutput.stdOut.testReport
-                    verifyExpectation(testReport != null) {
-                        "testReport is expected to be non-null"
-                    }
-                    verifyExpectation(!testReport!!.isEmpty()) {
-                        "testReport is expected to be non-empty"
-                    }
-                    verifyExpectation(testReport.failedTests.isNotEmpty()) {
-                        "Test did not fail as expected"
-                    }
-                    verifyExpectation(testReport.passedTests.isEmpty()) {
-                        "Test unexpectedly passed"
-                    }
-                }
-                is ExitCode -> {
-                    // Don't check exit code if it is unknown.
-                    val knownExitCode: Int = runResult.exitCode ?: return@forEach
-                    when (check) {
-                        is ExitCode.Expected -> verifyExpectation(knownExitCode == check.expectedExitCode) {
-                            "$visibleProcessName exit code is $knownExitCode while ${check.expectedExitCode} was expected."
-                        }
-                        is ExitCode.AnyNonZero -> verifyExpectation(knownExitCode != 0) {
-                            "$visibleProcessName exited with zero code, which wasn't expected."
-                        }
-                    }
-                }
-                is TestRunCheck.OutputDataFile -> {
-                    val expectedOutput = check.file.readText()
-                    val actualFilteredOutput = runResult.processOutput.stdOut.filteredOutput + runResult.processOutput.stdErr
-
-                    // Don't use verifyExpectation(expected, actual) to avoid exposing potentially large test output in exception message
-                    // and blowing up test logs.
-                    verifyExpectation(convertLineSeparators(expectedOutput) == convertLineSeparators(actualFilteredOutput)) {
-                        "Tested process output mismatch. See \"TEST STDOUT\" and \"EXPECTED OUTPUT DATA FILE\" below."
-                    }
-                }
-                is TestRunCheck.OutputMatcher -> {
-                    try {
-                        verifyExpectation(check.match(runResult.processOutput.stdOut.filteredOutput)) {
-                            "Tested process output has not passed validation."
-                        }
-                    } catch (t: Throwable) {
-                        if (t is Exception || t is AssertionError) {
-                            fail<Nothing>(
-                                getLoggedRun().withErrorMessage("Tested process output has not passed validation: " + t.message),
-                                t
+        val diagnostics = buildList<String> {
+            checks.forEach { check ->
+                when (check) {
+                    is ExecutionTimeout.ShouldNotExceed -> if(!runResult.hasFinishedOnTime) add(
+                        "Timeout exceeded during test execution."
+                    )
+                    is ExecutionTimeout.ShouldExceed -> if(runResult.hasFinishedOnTime) add(
+                        "Test is expected to fail with exceeded timeout, which hasn't happened."
+                    )
+                    is ExitCode -> {
+                        // Don't check exit code if it is unknown.
+                        val knownExitCode: Int = runResult.exitCode ?: return@forEach
+                        when (check) {
+                            is ExitCode.Expected -> if (knownExitCode != check.expectedExitCode) add(
+                                "$visibleProcessName exit code is $knownExitCode while ${check.expectedExitCode} was expected."
+                            )
+                            is ExitCode.AnyNonZero -> if (knownExitCode == 0) add(
+                                "$visibleProcessName exited with zero code, which wasn't expected."
+                            )
+                        }
+                    }
+                    is TestRunCheck.OutputDataFile -> {
+                        val expectedOutput = check.file.readText()
+                        val actualFilteredOutput = runResult.processOutput.stdOut.filteredOutput + runResult.processOutput.stdErr
+
+                        // Don't use verifyExpectation(expected, actual) to avoid exposing potentially large test output in exception message
+                        // and blowing up test logs.
+                        if(convertLineSeparators(expectedOutput) != convertLineSeparators(actualFilteredOutput)) add(
+                            "Tested process output mismatch. See \"TEST STDOUT\" and \"EXPECTED OUTPUT DATA FILE\" below."
+                        )
+                    }
+                    is TestRunCheck.OutputMatcher -> {
+                        try {
+                            if(!check.match(runResult.processOutput.stdOut.filteredOutput)) add(
+                                "Tested process output has not passed validation."
+                            )
+                        } catch (t: Throwable) {
+                            if (t is Exception || t is AssertionError) {
+                                add("Tested process output has not passed validation: " + t.message)
+                            } else {
+                                throw t
+                            }
+                        }
+                    }
+                    is TestRunCheck.FileCheckMatcher -> {
+                        val fileCheckExecutable = check.settings.configurables.absoluteLlvmHome + File.separator + "bin" + File.separator +
+                                if (SystemInfo.isWindows) "FileCheck.exe" else "FileCheck"
+                        require(File(fileCheckExecutable).exists()) {
+                            "$fileCheckExecutable does not exist. Make sure Distribution for `settings.configurables` " +
+                                    "was created using `propertyOverrides` to specify development variant of LLVM instead of user variant."
+                        }
+                        val fileCheckDump = runResult.testExecutable.executable.fileCheckDump!!
+                        val fileCheckOut = File(fileCheckDump.absolutePath + ".out")
+                        val fileCheckErr = File(fileCheckDump.absolutePath + ".err")
+
+                        val testTarget = check.settings.get<KotlinNativeTargets>().testTarget
+                        val checkPrefixes = buildList {
+                            add("CHECK")
+                            add("CHECK-${testTarget.abiInfoString}")
+                            add("CHECK-${testTarget.name.toUpperCaseAsciiOnly()}")
+                            if (testTarget.family.isAppleFamily) {
+                                add("CHECK-APPLE")
+                            }
+                        }
+                        val optimizationMode = check.settings.get<OptimizationMode>().name
+                        val checkPrefixesWithOptMode = checkPrefixes.map { "$it-$optimizationMode" }
+                        val commaSeparatedCheckPrefixes = (checkPrefixes + checkPrefixesWithOptMode).joinToString(",")
+
+                        val result = ProcessBuilder(
+                            fileCheckExecutable,
+                            check.testDataFile.absolutePath,
+                            "--input-file",
+                            fileCheckDump.absolutePath,
+                            "--check-prefixes", commaSeparatedCheckPrefixes,
+                            "--allow-deprecated-dag-overlap" // TODO specify it via new test directive for `function_attributes_at_callsite.kt`
+                        ).redirectOutput(fileCheckOut)
+                            .redirectError(fileCheckErr)
+                            .start()
+                            .waitFor()
+                        val errText = fileCheckErr.readText()
+                        val outText = fileCheckOut.readText()
+                        if(!(result == 0 && errText.isEmpty() && outText.isEmpty())) {
+                            val shortOutText = outText.lines().take(100)
+                            val shortErrText = errText.lines().take(100)
+                            add("FileCheck matching of ${fileCheckDump.absolutePath}\n" +
+                                    "with '--check-prefixes $commaSeparatedCheckPrefixes'\n" +
+                                    "failed with result=$result:\n" +
+                                    shortOutText.joinToString("\n") + "\n" +
+                                    shortErrText.joinToString("\n")
                            )
-                        } else {
-                            throw t
                        }
                    }
                }
-                is TestRunCheck.FileCheckMatcher -> {
-                    val fileCheckExecutable = check.settings.configurables.absoluteLlvmHome + File.separator + "bin" + File.separator +
-                            if (SystemInfo.isWindows) "FileCheck.exe" else "FileCheck"
-                    require(File(fileCheckExecutable).exists()) {
-                        "$fileCheckExecutable does not exist. Make sure Distribution for `settings.configurables` " +
-                                "was created using `propertyOverrides` to specify development variant of LLVM instead of user variant."
-                    }
-                    val fileCheckDump = runResult.testExecutable.executable.fileCheckDump!!
-                    val fileCheckOut = File(fileCheckDump.absolutePath + ".out")
-                    val fileCheckErr = File(fileCheckDump.absolutePath + ".err")
-
-                    val testTarget = check.settings.get<KotlinNativeTargets>().testTarget
-                    val checkPrefixes = buildList {
-                        add("CHECK")
-                        add("CHECK-${testTarget.abiInfoString}")
-                        add("CHECK-${testTarget.name.toUpperCaseAsciiOnly()}")
-                        if (testTarget.family.isAppleFamily) {
-                            add("CHECK-APPLE")
-                        }
-                    }
-                    val optimizationMode = check.settings.get<OptimizationMode>().name
-                    val checkPrefixesWithOptMode = checkPrefixes.map { "$it-$optimizationMode" }
-                    val commaSeparatedCheckPrefixes = (checkPrefixes + checkPrefixesWithOptMode).joinToString(",")
-
-                    val result = ProcessBuilder(
-                        fileCheckExecutable,
-                        check.testDataFile.absolutePath,
-                        "--input-file",
-                        fileCheckDump.absolutePath,
-                        "--check-prefixes", commaSeparatedCheckPrefixes,
-                        "--allow-deprecated-dag-overlap" // TODO specify it via new test directive for `function_attributes_at_callsite.kt`
-                    ).redirectOutput(fileCheckOut)
-                        .redirectError(fileCheckErr)
-                        .start()
-                        .waitFor()
-                    val errText = fileCheckErr.readText()
-                    val outText = fileCheckOut.readText()
-                    verifyExpectation(result == 0 && errText.isEmpty() && outText.isEmpty()) {
-                        val shortOutText = outText.lines().take(100)
-                        val shortErrText = errText.lines().take(100)
-                        "FileCheck matching of ${fileCheckDump.absolutePath}\n" +
-                                "with '--check-prefixes $commaSeparatedCheckPrefixes'\n" +
-                                "failed with result=$result:\n" +
-                                shortOutText.joinToString("\n") + "\n" +
-                                shortErrText.joinToString("\n")
-                    }
+            }
+        }
+        if (!expectedFailure) {
+            verifyExpectation(diagnostics.isEmpty()) {
+                diagnostics.joinToString("\n")
+            }
+        } else {
+            val runResultInfo = "TestCaseId: $testCaseId\nExit code: ${runResult.exitCode}\nFiltered test output is${
+                runResult.processOutput.stdOut.filteredOutput.let {
+                    if (it.isNotEmpty()) ":\n$it" else " empty."
                }
+            }"
+            verifyExpectation(diagnostics.isNotEmpty() || runResult.processOutput.stdOut.testReport?.failedTests?.isNotEmpty() == true) {
+                "Test did not fail as expected: $runResultInfo"
+            }
+            println("Test failed as expected.\n$runResultInfo")
+            if (diagnostics.isNotEmpty()) {
+                println("Diagnostics are:")
+                diagnostics.forEach(::println)
            }
        }

@@ -15,7 +15,7 @@ import org.jetbrains.kotlin.utils.addIfNotNull
 internal open class BaseTestRunProvider {
    protected fun createTestRun(testCase: TestCase, executable: TestExecutable, testRunName: String, testName: TestName?): TestRun {
        val runParameters = getTestRunParameters(testCase, testName)
-        return TestRun(displayName = testRunName, executable, runParameters, testCase.id, testCase.checks)
+        return TestRun(displayName = testRunName, executable, runParameters, testCase.id, testCase.checks, testCase.expectedFailure)
    }

    protected fun createSingleTestRun(testCase: TestCase, executable: TestExecutable): TestRun = createTestRun(
@@ -40,7 +40,7 @@ internal class TestNameResultHandler(
    visibleProcessName: String,
    checks: TestRunChecks,
    private val loggedParameters: LoggedData.TestRunParameters
-) : LocalResultHandler<Collection<TestName>>(runResult, visibleProcessName, checks) {
+) : LocalResultHandler<Collection<TestName>>(runResult, visibleProcessName, checks, null, false) {
    override fun getLoggedRun() = LoggedData.TestRun(loggedParameters, runResult)
    override fun doHandle() = GTestListing.parse(runResult.processOutput.stdOut.filteredOutput)
 }
@@ -59,7 +59,7 @@ internal class ResultHandler(
    checks: TestRunChecks,
    private val testRun: TestRun,
    private val loggedParameters: LoggedData.TestRunParameters
-) : LocalResultHandler<Unit>(runResult, visibleProcessName, checks) {
+) : LocalResultHandler<Unit>(runResult, visibleProcessName, checks, testRun.testCaseId, testRun.expectedFailure) {
    override fun getLoggedRun() = LoggedData.TestRun(loggedParameters, runResult)

    override fun doHandle() {
@@ -83,17 +83,8 @@ internal class ResultHandler(
            )
        }

-        if (checks.expectedFailureCheck !is TestRunCheck.ExpectedFailure)
+        if (!testRun.expectedFailure)
            verifyNoSuchTests(testReport.failedTests, "There are failed tests")
-        else {
-            runResult.processOutput.stdOut.filteredOutput.let {
-                if (it.isNotEmpty())
-                    println("Failure is expected. Exit code=${runResult.exitCode}, filtered test output is below:\n$it")
-                else
-                    println("Failure is expected. Exit code=${runResult.exitCode}, filtered test output is empty.")
-            }
-            verifyNoSuchTests(testReport.passedTests, "There are unexpectedly passed tests")
-        }
        assumeFalse(testReport.ignoredTests.isNotEmpty() && testReport.passedTests.isEmpty(), "Test case is disabled")
    }

@@ -55,7 +55,8 @@ internal class TestRun(
    val executable: TestExecutable,
    val runParameters: List<TestRunParameter>,
    val testCaseId: TestCaseId,
-    val checks: TestRunChecks
+    val checks: TestRunChecks,
+    val expectedFailure: Boolean,
 )

 internal sealed interface TestRunParameter {
@@ -22,8 +22,6 @@ internal sealed interface TestRunCheck {
        class Expected(val expectedExitCode: Int) : ExitCode()
    }

-    object ExpectedFailure : TestRunCheck
-
    class OutputDataFile(val file: File) : TestRunCheck

    class OutputMatcher(val match: (String) -> Boolean): TestRunCheck
@@ -34,7 +32,6 @@ internal sealed interface TestRunCheck {
 internal data class TestRunChecks(
    val executionTimeoutCheck: ExecutionTimeout,
    private val exitCodeCheck: ExitCode?,
-    val expectedFailureCheck: ExpectedFailure?,
    val outputDataFile: OutputDataFile?,
    val outputMatcher: OutputMatcher?,
    val fileCheckMatcher: FileCheckMatcher?,
@@ -43,7 +40,6 @@ internal data class TestRunChecks(
    override fun iterator() = iterator {
        yield(executionTimeoutCheck)
        yieldIfNotNull(exitCodeCheck)
-        yieldIfNotNull(expectedFailureCheck)
        yieldIfNotNull(outputDataFile)
        yieldIfNotNull(outputMatcher)
        yieldIfNotNull(fileCheckMatcher)
@@ -55,7 +51,6 @@ internal data class TestRunChecks(
        fun Default(timeout: Duration) = TestRunChecks(
            executionTimeoutCheck = ExecutionTimeout.ShouldNotExceed(timeout),
            exitCodeCheck = ExitCode.Expected(0),
-            expectedFailureCheck = null,
            outputDataFile = null,
            outputMatcher = null,
            fileCheckMatcher = null,