Fixes in kotlin.test (#2453)

JUnit and TestNG have some behavioral features that were not reflected
in the kotlin.test implementation for Kotlin/Native:

 - Any method (even a method which has no its own annotations) having
   a super method marked by any test annotation (e.g. @Test or
   @BeforeTest) is still executed during the test run unless
   the super method doesn't belong to an interface.
 - Annotation @Ignore makes sense only in a pair with the @Test
   annotation. In other words, if a super method has only the @Test
   annotation and the overriding method has only @Ignore annotation,
   the overriding method will still be executed.

This patch makes kotlin.test behavior corresponding to these
features.

backend.native/compiler/ir/backend.native/src/org/jetbrains/kotlin/backend/konan/lower/TestProcessor.kt

@@ -46,6 +46,7 @@ import org.jetbrains.kotlin.name.Name
 import org.jetbrains.kotlin.resolve.descriptorUtil.fqNameSafe
 import org.jetbrains.kotlin.storage.LockBasedStorageManager
 import org.jetbrains.kotlin.types.KotlinType
+import org.jetbrains.kotlin.utils.addToStdlib.firstNotNullResult
 
 internal class TestProcessor (val context: KonanBackendContext) {
 
@@ -82,10 +83,16 @@ internal class TestProcessor (val context: KonanBackendContext) {
 
     private fun MutableList<TestFunction>.registerFunction(
             function: IrFunctionSymbol,
-            kinds: Collection<FunctionKind>) = kinds.forEach { add(TestFunction(function, it)) }
+            kinds: Collection<Pair<FunctionKind, /* ignored: */ Boolean>>) =
+        kinds.forEach { (kind, ignored) ->
+            add(TestFunction(function, kind, ignored))
+        }
 
-    private fun MutableList<TestFunction>.registerFunction(function: IrFunctionSymbol, kind: FunctionKind) =
+    private fun MutableList<TestFunction>.registerFunction(
-            add(TestFunction(function, kind))
+        function: IrFunctionSymbol,
+        kind: FunctionKind,
+        ignored: Boolean
+    ) = add(TestFunction(function, kind, ignored))
 
     private fun <T: IrElement> IrStatementsBuilder<T>.generateFunctionRegistration(
             receiver: IrValueDeclaration,
@@ -161,19 +168,22 @@ internal class TestProcessor (val context: KonanBackendContext) {
     private val FunctionKind.runtimeKind: IrEnumEntrySymbol
         get() = symbols.getTestFunctionKind(this)
 
-    private data class TestFunction(val function: IrFunctionSymbol, val kind: FunctionKind) {
+    private data class TestFunction(
+        val function: IrFunctionSymbol,
+        val kind: FunctionKind,
         val ignored: Boolean
-            get() = function.descriptor.annotations.hasAnnotation(IGNORE_FQ_NAME)
+    )
-    }
 
     private inner class TestClass(val ownerClass: IrClassSymbol) {
         var companion: IrClassSymbol? = null
         val functions = mutableListOf<TestFunction>()
 
-        fun registerFunction(function: IrFunctionSymbol, kinds: Collection<FunctionKind>) =
+        fun registerFunction(
-                functions.registerFunction(function, kinds)
+            function: IrFunctionSymbol,
-        fun registerFunction(function: IrFunctionSymbol, kind: FunctionKind) =
+            kinds: Collection<Pair<FunctionKind, /* ignored: */ Boolean>>
-                functions.registerFunction(function, kind)
+        ) = functions.registerFunction(function, kinds)
+        fun registerFunction(function: IrFunctionSymbol, kind: FunctionKind, ignored: Boolean) =
+                functions.registerFunction(function, kind, ignored)
     }
 
     private inner class AnnotationCollector(val irFile: IrFile) : IrElementVisitorVoid {
@@ -192,27 +202,53 @@ internal class TestProcessor (val context: KonanBackendContext) {
 
         fun IrFunctionSymbol.hasAnnotation(fqName: FqName) = descriptor.annotations.any { it.fqName == fqName }
 
+        /**
+         * Checks if [this] or any of its parent functions has the annotation with the given [testAnnotation].
+         * If [this] contains the given annotation, returns [this].
+         * If one of the parent functions contains the given annotation, returns the [IrFunctionSymbol] for it.
+         * If the annotation isn't found or found only in interface methods, returns null.
+         */
+        fun IrFunctionSymbol.findAnnotatedFunction(testAnnotation: FqName): IrFunctionSymbol? {
+            val owner = this.owner
+            val parent = owner.parent
+            if (parent is IrClass && parent.isInterface) {
+                return null
+            }
+
+            if (hasAnnotation(testAnnotation)) {
+                return this
+            }
+
+            return (owner as? IrSimpleFunction)
+                ?.overriddenSymbols
+                ?.firstNotNullResult {
+                    it.findAnnotatedFunction(testAnnotation)
+                }
+        }
+
         fun registerClassFunction(classDescriptor: ClassDescriptor,
                                   function: IrFunctionSymbol,
-                                  kinds: Collection<FunctionKind>) {
+                                  kinds: Collection<Pair<FunctionKind, /* ignored: */ Boolean>>) {
 
             fun warn(msg: String) = context.reportWarning(msg, irFile, function.owner)
 
-            kinds.forEach { kind ->
+            kinds.forEach { (kind, ignored) ->
                 val annotation = kind.annotationFqName
                 when (kind) {
                     in FunctionKind.INSTANCE_KINDS -> with(classDescriptor) {
                         when {
                             isInner ->
                                 warn("Annotation $annotation is not allowed for methods of an inner class")
-                            isAbstract() ->
+                            isAbstract() -> {
-                                warn("Annotation $annotation is not allowed for methods of an abstract class")
+                                // We cannot create an abstract test class but it's allowed to mark its methods as
+                                // tests because the class can be extended. So skip this case without warnings.
+                            }
                             isCompanionObject ->
                                 warn("Annotation $annotation is not allowed for methods of a companion object")
                             constructors.none { it.valueParameters.size == 0 } ->
                                 warn("Test class has no default constructor: ${fqNameSafe}")
                             else ->
-                                testClasses.getTestClass(classDescriptor).registerFunction(function, kind)
+                                testClasses.getTestClass(classDescriptor).registerFunction(function, kind, ignored)
                         }
                     }
                     in FunctionKind.COMPANION_KINDS ->
@@ -221,10 +257,10 @@ internal class TestProcessor (val context: KonanBackendContext) {
                                 val containingClass = classDescriptor.containingDeclaration as ClassDescriptor
                                 val testClass = testClasses.getTestClass(containingClass)
                                 testClass.companion = symbols.symbolTable.referenceClass(classDescriptor)
-                                testClass.registerFunction(function, kind)
+                                testClass.registerFunction(function, kind, ignored)
                             }
                             classDescriptor.kind == ClassKind.OBJECT -> {
-                                testClasses.getTestClass(classDescriptor).registerFunction(function, kind)
+                                testClasses.getTestClass(classDescriptor).registerFunction(function, kind, ignored)
                             }
                             else -> warn("Annotation $annotation is only allowed for methods of an object " +
                                     "(named or companion) or top level functions")
@@ -249,12 +285,44 @@ internal class TestProcessor (val context: KonanBackendContext) {
             }
         }
 
+        private fun warnAboutInheritedAnnotations(
+            kind: FunctionKind,
+            function: IrFunctionSymbol,
+            annotatedFunction: IrFunctionSymbol
+        ) {
+            if (function.owner != annotatedFunction.owner) {
+                context.reportWarning(
+                    "Super method has a test annotation ${kind.annotationFqName} but the overriding method doesn't. " +
+                            "Note that the overriding method will still be executed.",
+                    irFile,
+                    function.owner
+                )
+            }
+        }
+
+        private fun warnAboutLoneIgnore(functionSymbol: IrFunctionSymbol): Unit = with(functionSymbol) {
+            if (hasAnnotation(IGNORE_FQ_NAME) && !hasAnnotation(FunctionKind.TEST.annotationFqName)) {
+                context.reportWarning(
+                    "Unused $IGNORE_FQ_NAME annotation (not paired with ${FunctionKind.TEST.annotationFqName}).",
+                    irFile,
+                    owner
+                )
+            }
+        }
+
         // TODO: Use symbols instead of containingDeclaration when such information is available.
         override fun visitFunction(declaration: IrFunction) {
             val symbol = declaration.symbol
             val owner = declaration.descriptor.containingDeclaration
 
-            val kinds = FunctionKind.values().filter { symbol.hasAnnotation(it.annotationFqName)  }
+            warnAboutLoneIgnore(symbol)
+            val kinds = FunctionKind.values().mapNotNull { kind ->
+                symbol.findAnnotatedFunction(kind.annotationFqName)?.let { annotatedFunction ->
+                    warnAboutInheritedAnnotations(kind, symbol, annotatedFunction)
+                    kind to (kind == FunctionKind.TEST && annotatedFunction.hasAnnotation(IGNORE_FQ_NAME))
+                }
+            }
+
             if (kinds.isEmpty()) {
                 return
             }

backend.native/tests/build.gradle

@@ -88,6 +88,13 @@ konanArtifacts {
         }
         srcDir 'testLibrary'
     }
+    def target = project.testTarget ?: 'host'
+    library('baseTestClass', targets: [target]) {
+        if (!useCustomDist) {
+            dependsOn ':dist'
+        }
+        srcFiles 'testing/library.kt'
+    }
 }
 
 task installTestLibrary(type: KlibInstall) {
@@ -2761,6 +2768,15 @@ task testing_custom_main(type: RunStandaloneKonanTest) {
             'Testing finished\n'
 }
 
+task testing_library_usage(type: RunStandaloneKonanTest) {
+    def target = project.testTarget ?: 'host'
+    dependsOn konanArtifacts.baseTestClass.getByTarget(target)
+    def klib = konanArtifacts.baseTestClass.getArtifactByTarget(target)
+    source = 'testing/library_user.kt'
+    flags = ['-tr', '-e', 'main', '-l', klib.absolutePath]
+    arguments = ['--ktest_logger=SILENT']
+}
+
 // Just check that the driver is able to produce runnable binaries.
 task driver0(type: RunDriverKonanTest) {
     goldValue = "Hello, world!\n"

backend.native/tests/testing/library.kt

@@ -0,0 +1,66 @@
+package library
+
+import kotlin.test.*
+
+val output = mutableListOf<String>()
+
+interface I1 {
+    @Test
+    fun foo()
+}
+
+interface I2 {
+    @Test
+    fun foo()
+}
+
+open class A {
+
+    @BeforeTest
+    open fun before() {
+        output.add("A.before")
+    }
+
+    @AfterTest
+    open fun after() {
+        output.add("A.after")
+    }
+
+    @Test
+    open fun test0() {
+        output.add("A.test0")
+    }
+
+    @Test
+    open fun test1() {
+        output.add("A.test1")
+    }
+
+    @Test
+    open fun test2() {
+        output.add("A.test2")
+    }
+
+    @Test
+    open fun test3() {
+        output.add("A.test3")
+    }
+
+    @Ignore
+    @Test
+    open fun ignored0() {
+        output.add("A.ignored0")
+    }
+
+    @Ignore
+    @Test
+    open fun ignored1() {
+        output.add("A.ignored1")
+    }
+
+    @Ignore
+    @Test
+    open fun ignored2() {
+        output.add("A.ignored2")
+    }
+}

backend.native/tests/testing/library_user.kt

@@ -0,0 +1,74 @@
+import kotlin.native.internal.test.*
+import kotlin.test.*
+import library.*
+
+open class B : A() {
+    // Override test methods without a test annotation.
+    // We should run these methods anyway.
+
+    override fun before() {
+        output.add("B.before")
+    }
+
+    // test0 should be executed.
+
+    // Should be executed.
+    override fun test1() {
+        output.add("B.test1")
+    }
+
+    // Should be executed
+    @Ignore
+    override fun test2() {
+        output.add("B.test2")
+    }
+
+    // Should be ignored.
+    @Ignore
+    @Test
+    override fun test3() {
+        output.add("B.test3")
+    }
+
+    // ignored0 should be ignored.
+
+    // Should be ignored.
+    override fun ignored1() {
+        output.add("B.ignored1")
+    }
+
+    // Should be executed.
+    @Test
+    override fun ignored2() {
+        output.add("B.ignored2")
+    }
+}
+
+// All test methods from B should be executed for C.
+class C: B() {}
+
+class D: I1, I2 {
+    // This method shouldn't be executed because its parent methods annotated
+    // with @Test belong to an interface instead of an abstract class.
+    override fun foo(){
+        output.add("D.foo")
+    }
+}
+
+fun main(args: Array<String>) {
+    testLauncherEntryPoint(args)
+    output.forEach(::println)
+    assertEquals(8, output.count { it == "A.after" })
+    assertEquals(8, output.count { it == "B.before" })
+
+    assertEquals(2, output.count { it == "A.test0" })
+    assertEquals(2, output.count { it == "B.test1" })
+    assertEquals(2, output.count { it == "B.test2" })
+    assertEquals(2, output.count { it == "B.ignored2" })
+
+    assertTrue(output.none { it == "B.test3" })
+    assertTrue(output.none { it == "A.ignored0" })
+    assertTrue(output.none { it == "B.ignored1" })
+
+    assertTrue(output.none { it == "D.foo" })
+}

runtime/src/main/kotlin/kotlin/test/Annotation.kt

@@ -8,28 +8,24 @@ package kotlin.test
 /**
  * Marks a function as a test.
  */
-@Retention(AnnotationRetention.SOURCE)
 @Target(AnnotationTarget.FUNCTION)
 public annotation class Test
 
 /**
  * Marks a function to be executed before a suite. Not supported in Kotlin/Common.
  */
-@Retention(AnnotationRetention.SOURCE)
 @Target(AnnotationTarget.FUNCTION)
 public annotation class BeforeClass
 
 /**
  * Marks a function to be executed after a suite. Not supported in Kotlin/Common.
  */
-@Retention(AnnotationRetention.SOURCE)
 @Target(AnnotationTarget.FUNCTION)
 public annotation class AfterClass
 
 /**
  * Marks a function to be executed before a test.
  */
-@Retention(AnnotationRetention.SOURCE)
 @Target(AnnotationTarget.FUNCTION)
 public annotation class BeforeEach
 
@@ -37,14 +33,12 @@ public annotation class BeforeEach
 /**
  * Marks a function to be executed after a test.
  */
-@Retention(AnnotationRetention.SOURCE)
 @Target(AnnotationTarget.FUNCTION)
 public annotation class AfterEach
 
 /**
  * Marks a test or a suite as ignored.
  */
-@Retention(AnnotationRetention.SOURCE)
 @Target(AnnotationTarget.CLASS, AnnotationTarget.FUNCTION)
 public annotation class Ignore