diff --git a/app/build.gradle b/app/build.gradle index 88c9a8b72..01f050d80 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -14,7 +14,6 @@ import com.android.build.OutputFile import groovy.json.JsonOutput import org.gradle.internal.logging.text.StyledTextOutput.Style import org.gradle.internal.logging.text.StyledTextOutputFactory -import org.mozilla.fenix.gradle.tasks.LintUnitTestRunner import static org.gradle.api.tasks.testing.TestResult.ResultType @@ -581,8 +580,6 @@ task buildTranslationArray { android.defaultConfig.buildConfigField "String[]", "SUPPORTED_LOCALE_ARRAY", foundLocalesString } -tasks.register('lintUnitTestRunner', LintUnitTestRunner) - afterEvaluate { // Format test output. Ported from AC #2401 diff --git a/buildSrc/src/main/java/org/mozilla/fenix/gradle/tasks/LintUnitTestRunner.kt b/buildSrc/src/main/java/org/mozilla/fenix/gradle/tasks/LintUnitTestRunner.kt deleted file mode 100644 index 0082d361f..000000000 --- a/buildSrc/src/main/java/org/mozilla/fenix/gradle/tasks/LintUnitTestRunner.kt +++ /dev/null @@ -1,108 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -package org.mozilla.fenix.gradle.tasks - -import org.gradle.api.DefaultTask -import org.gradle.api.GradleException -import org.gradle.api.tasks.TaskAction -import org.gradle.api.tasks.TaskExecutionException -import java.io.File - -private val INVALID_TEST_RUNNERS = setOf( - // When updating this list, also update the violation message. - "AndroidJUnit4", - "RobolectricTestRunner" -) - -/** - * A lint check that ensures unit tests do not specify an invalid test runner. See the error message - * and the suggested replacement class' kdoc for details. - * - * Performance note: AS indicates this task takes 50ms-100ms to run. This isn't too concerning because - * it's one task and it only runs for unit test compilation. However, if we add additional lint checks, - * we should considering aggregating them - e.g. this task reads the unit test file tree and we can - * combine all of our tasks such that the file tree only needs to be read once - or finding more - * optimal solutions - e.g. only running on changed files. - */ -open class LintUnitTestRunner : DefaultTask() { - init { - group = "Verification" - description = "Ensures unit tests do not specify an invalid test runner" - attachToCompilationTasks() - } - - private fun attachToCompilationTasks() { - project.gradle.projectsEvaluated { project.tasks.let { tasks -> - // We make compile tasks, rather than assemble tasks, depend on us because some tools, - // such as AS' test runner, call the compile task directly rather than going through assemble. - val compileUnitTestTasks = tasks.filter { - it.name.startsWith("compile") && it.name.contains("UnitTest") - } - compileUnitTestTasks.forEach { it.dependsOn(this@LintUnitTestRunner) } - - // To return feedback as early as possible, we run before all compile tasks including - // compiling the application. - val compileAllTasks = tasks.filter { it.name.startsWith("compile") } - compileAllTasks.forEach { it.mustRunAfter(this@LintUnitTestRunner) } - } } - } - - @TaskAction - fun lint() { - val unitTestDir = File(project.projectDir, "/src/test") - check(unitTestDir.exists()) { - "Error in task impl: expected test directory - ${unitTestDir.absolutePath} - to exist" - } - - val unitTestDirFileWalk = unitTestDir.walk().onEnter { true /* enter all dirs */ }.asSequence() - val kotlinFileWalk = unitTestDirFileWalk.filter { it.name.endsWith(".kt") && it.isFile } - check(kotlinFileWalk.count() > 0) { "Error in task impl: expected to walk > 0 test files" } - - val violatingFiles = kotlinFileWalk.filter { file -> - file.useLines { lines -> lines.any(::isLineInViolation) } - }.sorted().toList() - - if (violatingFiles.isNotEmpty()) { - throwViolation(violatingFiles) - } - } - - private fun isLineInViolation(line: String): Boolean { - val trimmed = line.trimStart() - return INVALID_TEST_RUNNERS.any { invalid -> - trimmed.startsWith("@RunWith($invalid::class)") - } - } - - private fun throwViolation(files: List) { - val failureHeader = """Lint failure: saw unexpected unit test runners. The following code blocks: - | - | @RunWith(AndroidJUnit4::class) - | @Config(application = TestApplication::class) - |OR - | @RunWith(RobolectricTestRunner::class) - | @Config(application = TestApplication::class) - | - |should be replaced with: - | - | @RunWith(FenixRobolectricTestRunner::class) - | - |To reduce redundancy of setting @Config. No @Config specification is necessary because - |the FenixRobolectricTestRunner sets it automatically. - | - |Relatedly, adding robolectric to a test increases its runtime non-trivially so please - |ensure Robolectric is necessary before adding it. - | - |The following files were found to be in violation: - """.trimMargin() - - val filesInViolation = files.map { - " ${it.relativeTo(project.rootDir)}" - } - - val errorMsg = (listOf(failureHeader) + filesInViolation).joinToString("\n") - throw TaskExecutionException(this, GradleException(errorMsg)) - } -} diff --git a/config/detekt.yml b/config/detekt.yml index 2568a929e..180847b99 100644 --- a/config/detekt.yml +++ b/config/detekt.yml @@ -112,6 +112,8 @@ mozilla-detekt-rules: # with the debuggable flag or not. Use a check for different build variants, # instead. bannedProperties: "BuildConfig.DEBUG" + MozillaCorrectUnitTestRunner: + active: true empty-blocks: active: true diff --git a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt index 7899a60b3..1a4c0f67e 100644 --- a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt +++ b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt @@ -14,7 +14,8 @@ class CustomRulesetProvider : RuleSetProvider { override fun instance(config: Config): RuleSet = RuleSet( ruleSetId, listOf( - MozillaBannedPropertyAccess(config) + MozillaBannedPropertyAccess(config), + MozillaCorrectUnitTestRunner(config) ) ) } diff --git a/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaCorrectUnitTestRunner.kt b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaCorrectUnitTestRunner.kt new file mode 100644 index 000000000..25834076b --- /dev/null +++ b/mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaCorrectUnitTestRunner.kt @@ -0,0 +1,76 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.fenix.detektrules + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +import io.gitlab.arturbosch.detekt.api.Entity +import io.gitlab.arturbosch.detekt.api.Issue +import io.gitlab.arturbosch.detekt.api.Rule +import io.gitlab.arturbosch.detekt.api.Severity +import io.gitlab.arturbosch.detekt.api.internal.PathFilters +import org.jetbrains.kotlin.psi.KtAnnotationEntry + +private val BANNED_TEST_RUNNERS = setOf( + // When updating this list, also update the violation message. + "AndroidJUnit4", + "RobolectricTestRunner" +) + +// There is a change to how we output violations in a different PR so the formatting for message +// might be weird but it should still be readable. +private val VIOLATION_MSG = """The following code blocks: + | + | @RunWith(AndroidJUnit4::class) + | @Config(application = TestApplication::class) + | OR + | @RunWith(RobolectricTestRunner::class) + | @Config(application = TestApplication::class) + | + | should be replaced with: + | + | @RunWith(FenixRobolectricTestRunner::class) + | + | To reduce redundancy of setting @Config. No @Config specification is necessary because + | the FenixRobolectricTestRunner sets it automatically. + | + | Relatedly, adding robolectric to a test increases its runtime non-trivially so please + | ensure Robolectric is necessary before adding it.""".trimMargin() + +/** + * Ensures we're using the correct Robolectric unit test runner. + */ +class MozillaCorrectUnitTestRunner(config: Config) : Rule(config) { + override val issue = Issue( + MozillaCorrectUnitTestRunner::class.simpleName!!, + Severity.Defect, + "Verifies we're using the correct Robolectric unit test runner", + Debt.FIVE_MINS + ) + + override val filters: PathFilters? + get() = PathFilters.of( + includes = listOf("**/test/**"), // unit tests only. + excludes = emptyList() + ) + + override fun visitAnnotationEntry(annotationEntry: KtAnnotationEntry) { + super.visitAnnotationEntry(annotationEntry) + + // Example of an annotation: @RunWith(FenixRobolectricUnitTestRunner::class) + val annotationClassName = annotationEntry.shortName?.identifier + if (annotationClassName == "RunWith") { + // Example arg in parens: "(FenixRobolectricUnitTestRunner::class)" + val argInParens = annotationEntry.lastChild.node.chars + val argClassName = argInParens.substring(1 until argInParens.indexOf(":")) + + val isInViolation = BANNED_TEST_RUNNERS.any { it == argClassName } + if (isInViolation) { + report(CodeSmell(issue, Entity.from(annotationEntry), VIOLATION_MSG)) + } + } + } +}