KT-45777: Add tests for top-level functions

Address review + Minor changes
Fix error when building with -Pbootstrap.local=true
Fix tests failing on Windows due to '/' character
This commit is contained in:
Hung Nguyen
2021-09-23 16:29:22 +01:00
committed by nataliya.valtman
parent e26dc4d574
commit b5f74ef9a6
17 changed files with 121 additions and 61 deletions
@@ -68,33 +68,37 @@ object ClasspathChangesComputer {
/** /**
* Maps the unchanged snapshot files of the current build to the unchanged snapshot files of the previous build (selected from all the * Maps the unchanged snapshot files of the current build to the unchanged snapshot files of the previous build (selected from all the
* snapshot files of the previous build). * snapshot files of the previous build).
*
* Note that the unchanged files of the current build were detected by Gradle, so a mapping must exist for each of them, we only have to
* find it.
*
* IMPORTANT: The alignment algorithm must use the same input normalization that is used for snapshot files in the Gradle task.
* Currently, snapshot files are annotated with `@Classpath` and are regular files (not jars), so (only) their contents and order
* matter.
*/ */
private fun alignUnchangedSnapshotFiles(unchangedCurrentSnapshotFiles: List<File>, previousSnapshotFiles: List<File>): Map<File, File> { private fun alignUnchangedSnapshotFiles(unchangedCurrentSnapshotFiles: List<File>, previousSnapshotFiles: List<File>): Map<File, File> {
var index = 0 val sizeToPreviousFiles: Map<Long, List<IndexedValue<File>>> = previousSnapshotFiles.withIndex().groupBy { it.value.length() }
var startIndexToSearch = 0
return unchangedCurrentSnapshotFiles.associateWith { unchangedCurrentFile -> return unchangedCurrentSnapshotFiles.associateWith { unchangedCurrentFile ->
var candidates = previousSnapshotFiles.subList(index, previousSnapshotFiles.size).filter { previousFile -> val candidates = (sizeToPreviousFiles[unchangedCurrentFile.length()] ?: emptyList()).filter { it.index >= startIndexToSearch }
unchangedCurrentFile.lastModified() == previousFile.lastModified() && unchangedCurrentFile.length() == previousFile.length() val unchangedPreviousFileWithIndex: IndexedValue<File> = if (candidates.size == 1) {
// A matching file must exist, so if there is only one candidate, it is the one.
candidates.single()
} else {
// If there are multiple matching files, select the first one. (Even if it doesn't match Gradle's alignment, it is still a
// correct alignment.)
val unchangedContents = unchangedCurrentFile.readBytes()
candidates.firstOrNull { candidate ->
unchangedContents.contentEquals(candidate.value.readBytes())
} ?: error("Can't find previous snapshot file of unchanged current snapshot file '${unchangedCurrentFile.path}'")
} }
if (candidates.size > 1) { startIndexToSearch = unchangedPreviousFileWithIndex.index + 1
candidates = candidates.filter { candidate -> unchangedPreviousFileWithIndex.value
unchangedCurrentFile.readBytes().contentEquals(candidate.readBytes())
}
}
check(candidates.isNotEmpty()) {
"Can't find previous snapshot file of unchanged current snapshot file '${unchangedCurrentFile.path}'"
}
// If there are multiple candidates, select the first one. (It doesn't have to match Gradle's alignment as long as it's still a
// correct alignment.)
val unchangedPreviousFile = candidates.first()
while (previousSnapshotFiles[index] != unchangedPreviousFile) {
index++
}
index++
unchangedPreviousFile
} }
} }
/** Returns `true` if this snapshot file contains a duplicate class with another snapshot file in the given set. */ /** Returns `true` if this snapshot file contains a duplicate class with another snapshot file in the given list. */
@Suppress("unused", "UNUSED_PARAMETER") @Suppress("unused", "UNUSED_PARAMETER")
private fun File.containsDuplicatesWith(otherSnapshotFiles: List<File>): Boolean { private fun File.containsDuplicatesWith(otherSnapshotFiles: List<File>): Boolean {
// TODO: Implement and optimize this method // TODO: Implement and optimize this method
@@ -129,8 +133,8 @@ object ClasspathChangesComputer {
// - Add previous class snapshots to incrementalJvmCache. // - Add previous class snapshots to incrementalJvmCache.
// - Internally, incrementalJvmCache maintains a set of dirty classes to detect removed classes. Add previous classes to this set // - Internally, incrementalJvmCache maintains a set of dirty classes to detect removed classes. Add previous classes to this set
// to detect removed classes later (see step 2). // to detect removed classes later (see step 2).
// - The final ChangesCollector result will contain all symbols in the previous classes (we actually don't need them, but it's // - The ChangesCollector result will contain symbols in the previous classes (we actually don't need them, but it's part of the
// part of the API's effects). // API's effects).
val unusedChangesCollector = ChangesCollector() val unusedChangesCollector = ChangesCollector()
for (previousSnapshot in previousClassSnapshots) { for (previousSnapshot in previousClassSnapshots) {
when (previousSnapshot) { when (previousSnapshot) {
@@ -161,13 +165,11 @@ object ClasspathChangesComputer {
// Step 2: // Step 2:
// - Add current class snapshots to incrementalJvmCache. This will overwrite any previous class snapshots that have the same // - Add current class snapshots to incrementalJvmCache. This will overwrite any previous class snapshots that have the same
// `JvmClassName`. The previous class snapshots that are not overwritten will be detected as removed class snapshots and will be // `JvmClassName`. The remaining previous class snapshots will be removed in step 3.
// removed from incrementalJvmCache in step 3. // - Internally, incrementalJvmCache will remove current classes from the set of dirty classes. After this, the remaining dirty
// - Internally, incrementalJvmCache will mark these classes as non-dirty. That means unchanged/modified classes that were marked // classes will be classes that are present on the previous classpath but not on the current classpath (i.e., removed classes).
// as dirty in step 1 will now be non-dirty (added classes will also be marked as non-dirty even though they were not marked as // - The intermediate ChangesCollector result will contain symbols in added classes and changed (added/modified/removed) symbols
// dirty before). After this, the remaining dirty classes will be removed classes. // in modified classes. We will collect symbols in removed classes in step 3.
// - The intermediate ChangesCollector result will contain all symbols in added classes and changed (added/modified/removed)
// symbols in modified classes. We will collect all symbols in removed classes in step 3.
val changesCollector = ChangesCollector() val changesCollector = ChangesCollector()
for (currentSnapshot in currentClassSnapshots) { for (currentSnapshot in currentClassSnapshots) {
when (currentSnapshot) { when (currentSnapshot) {
@@ -195,10 +197,10 @@ object ClasspathChangesComputer {
} }
// Step 3: // Step 3:
// - Detect removed classes: they are the remaining dirty classes. // - Detect removed classes: They are the remaining dirty classes.
// - Remove previous class snapshots of removed classes from incrementalJvmCache. // - Remove class snapshots of removed classes from incrementalJvmCache.
// - The final ChangesCollector result will contain all symbols in added classes, changed (added/modified/removed) symbols in // - The final ChangesCollector result will contain symbols in added classes, changed (added/modified/removed) symbols in modified
// modified classes, and all symbols in removed classes. // classes, and symbols in removed classes.
incrementalJvmCache.clearCacheForRemovedClasses(changesCollector) incrementalJvmCache.clearCacheForRemovedClasses(changesCollector)
// Normalize the changes and clean up // Normalize the changes and clean up
@@ -831,15 +831,25 @@ abstract class KotlinCompile @Inject constructor(
return if (fileChanges.isEmpty()) { return if (fileChanges.isEmpty()) {
ClasspathChanges.Available(LinkedHashSet(), LinkedHashSet()) ClasspathChanges.Available(LinkedHashSet(), LinkedHashSet())
} else { } else {
val currentClasspathEntrySnapshotFiles = classpathSnapshotProperties.classpathSnapshot.files.toList() val previousClasspathEntrySnapshotFiles = getPreviousClasspathEntrySnapshotFiles()
val changedCurrentFiles = fileChanges if (previousClasspathEntrySnapshotFiles.isEmpty()) {
.filter { it.changeType == ChangeType.ADDED || it.changeType == ChangeType.MODIFIED } // When this happens, it means that either the previous classpath was empty or there were no source files to compile in the
.map { it.file }.toSet() // previous non-incremental run so the task action was skipped and the classpath snapshot directory was not populated (see
ClasspathChangesComputer.compute( // AbstractKotlinCompile.executeImpl).
currentClasspathEntrySnapshotFiles = currentClasspathEntrySnapshotFiles, // We could improve this handling, but it's fine to return `UnableToCompute` here as it's likely that there are also no
previousClasspathEntrySnapshotFiles = getPreviousClasspathEntrySnapshotFiles(), // source files to compile/recompile in this incremental run.
unchangedCurrentClasspathEntrySnapshotFiles = currentClasspathEntrySnapshotFiles.filter { it !in changedCurrentFiles } ClasspathChanges.NotAvailable.UnableToCompute
) } else {
val currentClasspathEntrySnapshotFiles = classpathSnapshotProperties.classpathSnapshot.files.toList()
val changedCurrentFiles = fileChanges
.filter { it.changeType == ChangeType.ADDED || it.changeType == ChangeType.MODIFIED }
.map { it.file }.toSet()
ClasspathChangesComputer.compute(
currentClasspathEntrySnapshotFiles = currentClasspathEntrySnapshotFiles,
previousClasspathEntrySnapshotFiles = previousClasspathEntrySnapshotFiles,
unchangedCurrentClasspathEntrySnapshotFiles = currentClasspathEntrySnapshotFiles.filter { it !in changedCurrentFiles }
)
}
} }
} }
@@ -859,10 +869,7 @@ abstract class KotlinCompile @Inject constructor(
classpathSnapshotDir.deleteRecursively() classpathSnapshotDir.deleteRecursively()
classpathSnapshotDir.mkdirs() classpathSnapshotDir.mkdirs()
snapshotFiles.forEachIndexed { index, snapshotFile -> snapshotFiles.forEachIndexed { index, snapshotFile ->
val targetFile = File("$classpathSnapshotDir/$index/${snapshotFile.name}") snapshotFile.copyTo(File("$classpathSnapshotDir/$index/${snapshotFile.name}"))
snapshotFile.copyTo(targetFile, overwrite = true)
// Preserve timestamp to find out unchanged files later (see `ClasspathChangesComputer.alignUnchangedSnapshotFiles`)
targetFile.setLastModified(snapshotFile.lastModified())
} }
} }
@@ -83,12 +83,20 @@ class KotlinClassesClasspathChangesComputerTest : ClasspathChangesComputerTest()
LookupSymbol(name = "b3", scope = "com.example.B"), LookupSymbol(name = "b3", scope = "com.example.B"),
LookupSymbol(name = "b4", scope = "com.example.B"), LookupSymbol(name = "b4", scope = "com.example.B"),
LookupSymbol(name = "C", scope = "com.example"), LookupSymbol(name = "C", scope = "com.example"),
LookupSymbol(name = "D", scope = "com.example") LookupSymbol(name = "D", scope = "com.example"),
LookupSymbol(name = SAM_LOOKUP_NAME.asString(), scope = "com.example"),
LookupSymbol(name = "topLevelFuncB", scope = "com.example"),
LookupSymbol(name = "topLevelFuncC", scope = "com.example"),
LookupSymbol(name = "topLevelFuncD", scope = "com.example"),
LookupSymbol(name = "topLevelFuncInCKtMovedToDKt", scope = "com.example"),
LookupSymbol(name = "CKt", scope = "com.example"),
), ),
fqNames = setOf( fqNames = setOf(
FqName("com.example.B"), FqName("com.example.B"),
FqName("com.example.C"), FqName("com.example.C"),
FqName("com.example.D") FqName("com.example.D"),
FqName("com.example"),
FqName("com.example.CKt"),
) )
), ),
changes changes
@@ -169,18 +177,21 @@ private fun snapshotClasspath(classpathSourceDir: File, tmpDir: TemporaryFolder)
.sortedBy { it } .sortedBy { it }
val sourceFiles = relativePathsInDir.map { relativePath -> val sourceFiles = relativePathsInDir.map { relativePath ->
if (relativePath.endsWith(".kt")) { if (relativePath.endsWith(".kt")) {
val preCompiledClassFilesRoot = classpathEntrySourceDir.path.let {
File(it.substringBeforeLast("src") + "classes" + it.substringAfterLast("src"))
}.also { check(it.exists()) }
KotlinSourceFile( KotlinSourceFile(
classpathEntrySourceDir, relativePath, classpathEntrySourceDir, relativePath,
preCompiledClassFile = ClassFile( preCompiledClassFiles = listOf(
File(classpathEntrySourceDir.path.replace("src/kotlin", "classes/kotlin")), ClassFile(preCompiledClassFilesRoot, relativePath.replace(".kt", ".class")),
relativePath.replace(".kt", ".class") ClassFile(preCompiledClassFilesRoot, relativePath.replace(".kt", "Kt.class"))
) ).filter { File(it.classRoot, it.unixStyleRelativePath).exists() }
) )
} else { } else {
SourceFile(classpathEntrySourceDir, relativePath) SourceFile(classpathEntrySourceDir, relativePath)
} }
} }
val classFiles = sourceFiles.map { TestSourceFile(it, tmpDir).compile() } val classFiles = sourceFiles.flatMap { TestSourceFile(it, tmpDir).compileAll() }
ClasspathEntrySnapshot( ClasspathEntrySnapshot(
classSnapshots = classFiles.map { it.unixStyleRelativePath to it.snapshot() }.toMap(LinkedHashMap()) classSnapshots = classFiles.map { it.unixStyleRelativePath to it.snapshot() }.toMap(LinkedHashMap())
) )
@@ -36,7 +36,12 @@ abstract class ClasspathSnapshotTestCommon {
fun asFile() = File(baseDir, unixStyleRelativePath) fun asFile() = File(baseDir, unixStyleRelativePath)
} }
class KotlinSourceFile(baseDir: File, relativePath: String, val preCompiledClassFile: ClassFile) : SourceFile(baseDir, relativePath) class KotlinSourceFile(baseDir: File, relativePath: String, val preCompiledClassFiles: List<ClassFile>) :
SourceFile(baseDir, relativePath) {
constructor(baseDir: File, relativePath: String, preCompiledClassFile: ClassFile) :
this(baseDir, relativePath, listOf(preCompiledClassFile))
}
/** Same as [SourceFile] but with a [TemporaryFolder] to store the results of operations on the [SourceFile]. */ /** Same as [SourceFile] but with a [TemporaryFolder] to store the results of operations on the [SourceFile]. */
open class TestSourceFile(val sourceFile: SourceFile, private val tmpDir: TemporaryFolder) { open class TestSourceFile(val sourceFile: SourceFile, private val tmpDir: TemporaryFolder) {
@@ -73,12 +78,25 @@ abstract class ClasspathSnapshotTestCommon {
} }
private fun compileKotlin(): List<ClassFile> { private fun compileKotlin(): List<ClassFile> {
// TODO: Call Kotlin compiler to generate classes, for example:
// org.jetbrains.kotlin.test.MockLibraryUtil.compileKotlin(sourceFile.asFile().path, sourceFile.preCompiledClassFile.classRoot)
// However, currently it is not possible (see https://github.com/JetBrains/kotlin/pull/4512#discussion_r679432232), so we use
// the precompiled classes in the test data instead.
sourceFile as KotlinSourceFile sourceFile as KotlinSourceFile
return listOf(sourceFile.preCompiledClassFile)
// TODO: Call Kotlin compiler to generate classes.
// If <kotlin-repo>/dist/kotlinc/lib/kotlin-compiler.jar is available (e.g., by running ./gradlew dist), we will be able to
// call the Kotlin compiler to generate classes from here. For example:
// val classesDir = tmpDir.newFolder()
// org.jetbrains.kotlin.test.MockLibraryUtil.compileKotlin(sourceFile.asFile().path, classesDir)
// val classFiles = classesDir.walk()
// .filter { it.isFile && !it.toRelativeString(classesDir).startsWith("META-INF/") }
// .map { ClassFile(classesDir, it.toRelativeString(classesDir)) }
// .sortedBy { it.unixStyleRelativePath.substringBefore(".class") }
// .toList()
// kotlin.test.assertEquals(classFiles.size, sourceFile.preCompiledClassFiles.size)
// sourceFile.preCompiledClassFiles.forEach {
// File(classesDir, it.unixStyleRelativePath).copyTo(File(it.classRoot, it.unixStyleRelativePath), overwrite = true)
// }
// However, kotlin-compiler.jar is currently not available in CI builds, so we need to pre-compile the classes, put them in the
// test data, and use them here instead.
return sourceFile.preCompiledClassFiles
} }
private fun compileJava(): List<ClassFile> { private fun compileJava(): List<ClassFile> {
@@ -1,6 +1,6 @@
package com.example; package com.example;
// Will be removed // To-be-removed class
public class C { public class C {
public int c = 0; public int c = 0;
} }
@@ -4,3 +4,6 @@ package com.example
class A { class A {
val a: Int = 0 val a: Int = 0
} }
// Unchanged top-level function
fun topLevelFuncA() {}
@@ -7,3 +7,6 @@ public class B {
//val b3: Int = 0 // Removed field //val b3: Int = 0 // Removed field
val b4: Int = 0 // Added field val b4: Int = 0 // Added field
} }
// Modified top-level function
fun topLevelFuncB(): Int = 0
@@ -4,3 +4,9 @@ package com.example
public class D { public class D {
val d: Int = 0 val d: Int = 0
} }
// Added top-level function
fun topLevelFuncD() {}
// Moved top-level function
fun topLevelFuncInCKtMovedToDKt() {}
@@ -3,3 +3,5 @@ package com.example
class A { class A {
val a: Int = 0 val a: Int = 0
} }
fun topLevelFuncA() {}
@@ -5,3 +5,5 @@ public class B {
val b2: Int = 0 val b2: Int = 0
val b3: Int = 0 val b3: Int = 0
} }
fun topLevelFuncB() {}
@@ -1,6 +1,12 @@
package com.example package com.example
// Will be removed // To-be-removed class
public class C { public class C {
val c: Int = 0 val c: Int = 0
} }
// To-be-removed top-level function
fun topLevelFuncC() {}
// To-be-moved top-level function
fun topLevelFuncInCKtMovedToDKt() {}