Commit f690f873 authored by Michael Comella's avatar Michael Comella Committed by Michael Comella
Browse files

No issue: replace LintUnitTestRunner with detekt implementation. 

This should be more performant and easier to maintain.
parent 57805b82

app/build.gradle

+0 −3
Original line number Diff line number Diff line
@@ -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

buildSrc/src/main/java/org/mozilla/fenix/gradle/tasks/LintUnitTestRunner.kt

deleted100644 → 0
+0 −108
Original line number Diff line number Diff line 
/* 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<File>) {

        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))

    }

}

config/detekt.yml

+2 −0
Original line number Diff line number Diff line
@@ -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

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/CustomRulesetProvider.kt

+2 −1
Original line number Diff line number Diff line
@@ -14,7 +14,8 @@ class CustomRulesetProvider : RuleSetProvider { 
    override fun instance(config: Config): RuleSet = RuleSet(

        ruleSetId,

        listOf(

            MozillaBannedPropertyAccess(config)

            MozillaBannedPropertyAccess(config),

            MozillaCorrectUnitTestRunner(config)

        )

    )

}

mozilla-detekt-rules/src/main/java/org/mozilla/fenix/detektrules/MozillaCorrectUnitTestRunner.kt

0 → 100644
+76 −0
Original line number Diff line number Diff line 
/* 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))

            }

        }

    }

}