Modularize test module #2720

arturbosch · 2020-05-22T21:32:50Z

This "huge" PR (mostly import changes) splits the detekt-test module into two modules.
detekt-test remains the module used for testing rules.
The new detekt-test-utils module is more lightweight in module dependencies.
This module specializes in utility classes and parsing kotlin files for tests.

The problem with the detekt-test module:
detekt-test depends on "all" compile tasks of other detekt-modules but is the requirement for all test compile tasks! This means it is not only the bottle-neck for parallel build but also triggers all tests on nearly any change (especially gradle-plugin tests with runtime of 2.5m!!!)!

This first change in test dependencies gets rid of the detekt-test dependency on generator, gradle-plugin and metrics modules.
This leads to less triggering of the gradle tests (!!!) and these modules can be executed earlier in parallel builds (20s earlier gradle plugin testing, see screenshot).

In further non-breaking modularization effors for #2680, I want to get rid of the detekt-test dependency for cli, core and the api modules leading to less retriggering of tests.

While this change does not necessary lead to performance improvements for a clean build, it will for incremental builds. This may improve CI build times as more test tasks can be cached.

In a breaking 2.0 release we could rename detekt-test to detekt-rule-testing-support or something to make it's name more clearer.

…dule

… modules

codecov · 2020-05-22T21:43:39Z

Codecov Report

Merging #2720 into master will decrease coverage by 0.00%.
The diff coverage is 62.50%.

@@             Coverage Diff              @@
##             master    #2720      +/-   ##
============================================
- Coverage     80.55%   80.55%   -0.01%     
- Complexity     2317     2323       +6     
============================================
  Files           378      380       +2     
  Lines          6928     6941      +13     
  Branches       1255     1256       +1     
============================================
+ Hits           5581     5591      +10     
- Misses          721      723       +2     
- Partials        626      627       +1

Impacted Files	Coverage Δ	Complexity Δ
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt	`27.27% <0.00%> (ø)`	`1.00 <0.00> (ø)`
...lab/arturbosch/detekt/api/internal/PathMatchers.kt	`60.00% <ø> (+4.44%)`	`0.00 <0.00> (ø)`
...gitlab/arturbosch/detekt/cli/runners/AstPrinter.kt	`100.00% <ø> (ø)`	`4.00 <0.00> (ø)`
...in/kotlin/io/gitlab/arturbosch/detekt/core/Junk.kt	`45.45% <ø> (-4.55%)`	`0.00 <0.00> (ø)`
...io/gitlab/arturbosch/detekt/core/KtFileModifier.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...io/gitlab/arturbosch/detekt/core/KtTreeCompiler.kt	`69.23% <ø> (ø)`	`12.00 <0.00> (ø)`
...itlab/arturbosch/detekt/core/ProcessingSettings.kt	`80.55% <ø> (ø)`	`17.00 <0.00> (ø)`
...n/kotlin/io/github/detekt/parser/DetektPomModel.kt	`70.58% <ø> (ø)`	`5.00 <0.00> (?)`
.../io/github/detekt/parser/KotlinEnvironmentUtils.kt	`84.90% <ø> (ø)`	`0.00 <0.00> (?)`
...ls/src/main/kotlin/io/github/detekt/psi/KtFiles.kt	`33.33% <33.33%> (ø)`	`0.00 <0.00> (?)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bff3d64...51c775a. Read the comment docs.

github-actions · 2020-05-22T21:51:34Z

	Fails
🚫	`danger-kotlin` failed.

Log

Executing kotlinc -script-templates systems.danger.kts.DangerFileScript -cp /usr/local/lib/danger/danger-kotlin.jar -script /Dangerfile.df.kts /tmp/danger-dsl.json danger_out.json - pid 377
Uncaught Kotlin exception: kotlin.Exception: Command kotlinc -script-templates systems.danger.kts.DangerFileScript -cp /usr/local/lib/danger/danger-kotlin.jar -script /Dangerfile.df.kts /tmp/danger-dsl.json danger_out.json exited with code 768
        at kfun:systems.danger.cmd.Cmd.exec() (0x21c739)
        at kfun:systems.danger.cmd.dangerfile.DangerFile.execute(kotlin.String;kotlin.String) (0x21d227)
        at kfun:systems.danger.DangerKotlin.run() (0x21c0e6)
        at Init_and_run_start (0x21f472)
        at __libc_start_main (0x7f8f290cdb97)
        at  (0x20f029)
        at  ((nil))

Generated by 🚫 dangerJS against fb6c374

BraisGabin

Why do you need an empty .css in the test resources? detekt-parser/src/test/resources/css/test.css

There are not enough reactions to show the joy of this PR. Great boost!

BraisGabin · 2020-05-22T22:00:16Z

detekt-parser/src/test/resources/cases/Default.kt

+ * A comment
+ */
+@Suppress("Unused")
+class Default


Shouldn't be better to inline this code?

KtCompiler works on Path objects.

The empty css file is also to test KtCompiler ^^

detekt-test/build.gradle.kts

BraisGabin · 2020-05-22T22:03:58Z

detekt-test/build.gradle.kts

-    implementation(kotlin("script-util"))
-    implementation(kotlin("scripting-compiler-embeddable"))
-    implementation("org.assertj:assertj-core:${Versions.ASSERTJ}")
+    api(project(":detekt-test-utils"))


Probably same here. If you need both you can always add both as dependencies. I think that it's better to be explicit.

I considered this but our tests use to many dependencies from test-utils for now.
I consider hiding this dependency from the sample-extension build file vital for now.

BraisGabin · 2020-05-22T22:05:56Z

detekt-test/src/main/kotlin/io/gitlab/arturbosch/detekt/test/Resources.kt

-    requireNotNull(resource) { "Make sure the resource '$name' exists!" }
-    return resource.toURI()
-}
+fun resource(name: String): URI = io.github.detekt.test.utils.resource(name)


This function is here because you don't want to create a BC, right?

Hm, no just to not need to modify all test files for now.
We never said that detekt-test is api or publicly supported, just mentioned it in the sample-extension build file :/.
I will migrate the resource usages in another PR.
We should try not to break functions like Rule.lint as they are more likely used.

BraisGabin · 2020-05-22T22:07:41Z

detekt-test-utils/build.gradle.kts

@@ -0,0 +1,9 @@
+dependencies {
+    api(kotlin("stdlib-jdk8"))
+    api("org.assertj:assertj-core:${Versions.ASSERTJ}")


Why don't just use implementation?

AssetJ is our assertion library and therefore is used in every module.
We would have to declare it in 10+ modules.

github-actions · 2020-05-23T17:37:19Z

	Fails
🚫	`danger-kotlin` failed.

Log

Executing kotlinc -script-templates systems.danger.kts.DangerFileScript -cp /usr/local/lib/danger/danger-kotlin.jar -script /Dangerfile.df.kts /tmp/danger-dsl.json danger_out.json - pid 378
Uncaught Kotlin exception: kotlin.Exception: Command kotlinc -script-templates systems.danger.kts.DangerFileScript -cp /usr/local/lib/danger/danger-kotlin.jar -script /Dangerfile.df.kts /tmp/danger-dsl.json danger_out.json exited with code 768
        at kfun:systems.danger.cmd.Cmd.exec() (0x21c739)
        at kfun:systems.danger.cmd.dangerfile.DangerFile.execute(kotlin.String;kotlin.String) (0x21d227)
        at kfun:systems.danger.DangerKotlin.run() (0x21c0e6)
        at Init_and_run_start (0x21f472)
        at __libc_start_main (0x7ff694d82b97)
        at  (0x20f029)
        at  ((nil))

Generated by 🚫 dangerJS against 51c775a

Whathecode · 2020-07-14T15:34:14Z

After upgrading to 1.10.0, due to this change I expect, I got the following error:

e: ...MyRuleTest.kt: (158, 39): Cannot access 'io.github.detekt.parser.KtCompiler' which is a supertype of 'io.github.detekt.test.utils.KtTestCompiler'. Check your module classpath for missing or conflicting

I had to add a dependency on detekt-parser to use KtTestCompiler in the test sources of my custom rules:

    testImplementation "io.gitlab.arturbosch.detekt:detekt-test:1.10.0"
    testImplementation "io.gitlab.arturbosch.detekt:detekt-parser:1.10.0"

Don't know whether this was intentional. Just thought I'd share this problem I ran into in case others encounter it.

arturbosch · 2020-07-14T16:15:37Z

Thanks for reporting.
What is your usecase with KtTestCompiler ?

If its parsing content you could use compileContentForTest or compileForTest without needing the dependency.
Are you using createEnvironment ?

Whathecode · 2020-07-14T16:46:37Z

I was just using KtTestCompiler.compileFromContent. Your recommendation worked perfectly. Thank you!

EDIT:
Oh, I am in fact using createEnvironment on a separate branch for a new rule I am adding. Details here: #2857 (comment) Will this give me problems? I will continue work on this on Thursday.

detekt/detekt#2720 (comment)

arturbosch · 2020-07-14T18:12:03Z

@Whathecode are you using Spek as testing framework?
If yes, there is fun Root.setupKotlinEnvironment() which can be used as in

detekt/detekt-rules-errorprone/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/ImplicitUnitReturnTypeSpec.kt

Line 16 in b6e88a9

val env: KotlinCoreEnvironment by memoized()

I've created #2871 to hide KtTestCompiler from users. There will be an alternative for testing rules which need type resolution.

Whathecode · 2020-07-14T19:51:40Z

@Whathecode are you using Spek as testing framework?

Nope, I am using kotlin.test.

arturbosch added 4 commits May 22, 2020 21:59

Extract KtCompiler to detekt-parser preparing a minimal test utils mo…

eec9648

…dule

Fix potential memory leak in tests not closing settings

d6f48f4

Move parsing and generic test utils to own module

ceacbb9

Do not depend on test module for metrics, gradle plugin and generator…

116a6bf

… modules

arturbosch added this to the 1.10.0 milestone May 22, 2020

Exclude test-utils from coverage report as we do with test module

fb6c374

detekt deleted a comment from github-actions bot May 22, 2020

BraisGabin reviewed May 22, 2020

View reviewed changes

schalkms approved these changes May 23, 2020

View reviewed changes

Explicitly depend on core-module in tests

51c775a

arturbosch merged commit fdebbc4 into master May 23, 2020

arturbosch deleted the modularize-test-module branch May 23, 2020 17:51

arturbosch added the housekeeping Marker for housekeeping tasks and refactorings label Jun 4, 2020

Whathecode added a commit to cph-cachet/carp.core-kotlin that referenced this pull request Jul 14, 2020

Remove unnecessary detekt-parser dependency.

8c5b4d4

detekt/detekt#2720 (comment)

arturbosch mentioned this pull request Jul 14, 2020

Hide KtTestCompiler as internal #2871

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularize test module #2720

Modularize test module #2720

arturbosch commented May 22, 2020 •

edited

Loading

codecov bot commented May 22, 2020 •

edited

Loading

github-actions bot commented May 22, 2020

BraisGabin left a comment

BraisGabin May 22, 2020

arturbosch May 23, 2020

arturbosch May 23, 2020

BraisGabin May 22, 2020

arturbosch May 23, 2020

BraisGabin May 22, 2020

arturbosch May 23, 2020

BraisGabin May 22, 2020

arturbosch May 23, 2020

github-actions bot commented May 23, 2020

Whathecode commented Jul 14, 2020

arturbosch commented Jul 14, 2020

Whathecode commented Jul 14, 2020 •

edited

Loading

arturbosch commented Jul 14, 2020

Whathecode commented Jul 14, 2020 •

edited

Loading

Modularize test module #2720

Modularize test module #2720

Conversation

arturbosch commented May 22, 2020 • edited Loading

codecov bot commented May 22, 2020 • edited Loading

Codecov Report

github-actions bot commented May 22, 2020

Log

BraisGabin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2020

Log

Whathecode commented Jul 14, 2020

arturbosch commented Jul 14, 2020

Whathecode commented Jul 14, 2020 • edited Loading

arturbosch commented Jul 14, 2020

Whathecode commented Jul 14, 2020 • edited Loading

arturbosch commented May 22, 2020 •

edited

Loading

codecov bot commented May 22, 2020 •

edited

Loading

Whathecode commented Jul 14, 2020 •

edited

Loading

Whathecode commented Jul 14, 2020 •

edited

Loading