Replace Spek with a new test framework

[3flex]

Expected Behavior

We use a test framework that is properly maintained.

Current Behavior

About a month since IntelliJ 2021.3 was released the Spek plugin remains incompatible. Also, spekframework/spek#959 (comment)

Context

As a contributor this is a point of friction when using new IDE versions. We also can't expect any improvements to be made to Spek itself anymore as it's effectively abandoned by the original maintainer and unfortunately nobody else has stepped up.

I would suggest a transition away from Spek, as painful as this will be. This could be done in phases:

1. Pick a new test framework. Suggest JUnit 5 as it's the de facto standard and works perfectly well out of the box with Kotlin, Gradle & IntelliJ.
2. Don't accept contributions with new Spek tests - only allow the new framework.
3. Over time, rewrite existing tests using the new test framework (I had experimented with IntelliJ's structural search and replace to assist with this, but didn't have much luck).

TODO

• custom-checks (Migrate custom-checks tests to JUnit 5 #4467)
• detekt-api (migrate tests in detekt-api to junit #4521)
• detekt-cli (Migrate tests in detekt-cli to junit #4519)
• detekt-core (migrate tests in detekt-core to junit #4507)
• detekt-formatting (migrate tests in detekt-formatting to junit #4508)
• detekt-generator (Transform tests in detekt-generator to junit #4494)
• detekt-gradle-plugin (Migrate detekt-gradle-plugin tests to JUnit #4529)
• detekt-metrics (Migrate tests in detekt-metrics to junit #4511)
• detekt-parser (migrate tests in detekt-parser to junit #4512)
• detekt-psi-utils (Migrate detekt-psi-utils to junit #4515)
• detekt-report-html (Migrate tests in detekt-report-xxx to junit #4513)
• detekt-report-sarif (Migrate tests in detekt-report-xxx to junit #4513)
• detekt-report-txt (Migrate tests in detekt-report-xxx to junit #4513)
• detekt-report-xml (Migrate tests in detekt-report-xxx to junit #4513)
• detekt-rules (Migrate detekt-rules tests to JUnit #4514)
• detekt-rules-complexity (Migrate detekt-rules-complexity tests to JUnit #4566)
• detekt-rules-coroutines (Migrate detekt-rules-coroutines tests to JUnit #4567)
• detekt-rules-documentation (Migrate detekt-rules-documentation tests to JUnit #4568)
• detekt-rules-empty (Migrate tests in detekt-rules-empty to junit #4522)
• detekt-rules-errorprone (Migrate tests in detekt-rules-errorprone to junit #4523)
• detekt-rules-exceptions (Migrate detekt-rules-exceptions tests to JUnit #4572)
• detekt-rules-naming (Migrate detekt-rules-naming tests to JUnit #4574)
• detekt-rules-performance (Migrate detekt-rules-performance tests to JUnit #4569)
• detekt-rules-style (Migrate detekt-rules-style tests to JUnit #4575)
• detekt-sample-extensions (Migrate detekt-sample-extensions tests to JUnit #4525)
• detekt-tooling (Migrate detekt-tooling tests to JUnit 5 #4524)

[marschwar]

detekt was my first encounter with spek and over time I have grown to like it. Nevertheless I think the transition to junit5 is a good idea.

I have come across many different styles (naming, structuring, parameterizations) of junit tests over the years. Do you think we should have guide lines for junit tests in place? Personally I would also be fine with just starting and creating guide lines along the way if necessary.

[severn-everett]

I think it would be good to establish some kind of guidelines beforehand. Five different developers could produce five entirely different styles of testing throughout the codebase, increasing the cognitive load for whomever has to maintain the code in the future.

[severn-everett]

For example, how much would parameterization be encouraged? The majority of the rule tests are basically "test for whether snippet of code X triggers rule Y", so somebody could theoretically really go to town and write one test for a rule with loads and loads of parameters being passed in. Obviously, that would be really hard to read, so maybe create a rule that states that test parameterization be limited to only variations of a specific test case.

[3flex]

I'm literally now working on migrating custom-checks as it's self-contained and tests a rule, and rule tests obviously make up the majority of the tests. It should provide a good template. I'll submit a PR shortly for feedback.

I think guidelines are worthy for new contributions (and can be added to the contributions guide), but for the initial migration I think it's better to do as simple a migration as possible without having to refactor more than is necessary.

[3flex]

@BraisGabin if you update your conversion script please post it in this thread.

If anyone would like to take ownership of converting any specific module then I suggest making a comment here declaring that to avoid duplicating work.

[marschwar]

I will start with detekt-generator

[BraisGabin]

I updated a bit the script and copied here to make it easier to find it.

#!/usr/bin/env bash

base_path="detekt-core/src/test/"

replace_all() {
  find $base_path -name "*Spec.kt" -type f -print0 | xargs -0 gsed -i -E "$1"
}

replace_all '/ (it|test|context|describe)\(.*\)/s/[.:`>]/_/g'
replace_all 's/(object|class) (.*) : Spek\(\{/class \2 {/g'
replace_all 's/^\}\)$/}/g'
replace_all 's/describe\("(.*)"\)/@Nested inner class `\1`/g'
replace_all 's/context\("(.*)"\)/@Nested inner class `\1`/g'
replace_all 's/it\("(.*)"\)/@Test fun `\1`()/g'
replace_all 's/test\("(.*)"\)/@Test fun `\1`()/g'
replace_all 's/beforeEachTest \{/@BeforeEach fun setUp() {/g'
replace_all 's/afterEachTest \{/@AfterEach fun tearDown() {/g'
replace_all '/^package /a \\nimport org.junit.jupiter.api.Nested\nimport org.junit.jupiter.api.Test\nimport org.junit.jupiter.api.BeforeEach\nimport org.junit.jupiter.api.AfterEach'
replace_all '/^import org.spekframework.spek2/d'
replace_all 's/val ([[:alpha:]]*) by memoized(\([[:alpha:]]*\))? \{(.*)\}.*/private val \1 =\3/g'

[marschwar]

replace_all 's/^class (.*)Spec/internal class \1Spec/g'

Should we remove this line? Why should the test classes be internal?

[BraisGabin]

Done.

[marschwar]

Done.

One more question/request: Should we add this to the end of the script?

replace_all 's/val ([[:alpha:]]*) by memoized(\([[:alpha:]]*\))? \{(.*)\}.*/private val \1 =\3/g'

[3flex]

Claiming detekt-tooling

[BraisGabin]

One more question/request: Should we add this to the end of the script?

replace_all 's/val ([[:alpha:]]*) by memoized(\([[:alpha:]]*\))? \{(.*)\}.*/private val \1 =\3/g'

Done

[marschwar]

claiming detekt-cli

[marschwar]

claiming detekt-api

[3flex]

Claiming detekt-gradle-plugin

[TWiStErRob]

This is one amazing effort, well played! I suspect by this time you're pretty comfortable with JUnit 5 😵.

[BraisGabin]

Now that the port is nearly done we can talk about two missing topics:

1. Do we want to rename the classes/files from Spec to Test/s?
   There is a comment about this here: Drop Spek #4670 (comment)
2. The script did its job but it left some "code debt". For example, we have a lot of unnecesary top-level @Nest. Do we want to trackle those down?

My opinion:

1. No need. I don't think that we should even bother to be consistent here.
2. I have mixed feelings. I would like to remove those (I think that I would add a rule on my junit-rule-set to flag this). BUT if we do that all the indentation of our tests will change so we would lose a lot of git history. BUT if we don't do it external contributors will fix these kind of things little by little and ask them to undo that feels odd. So I think that I prefer to fix these cases upfront.

[3flex]

Personally I don't think either of these are major issues. If there was a detekt rule to identify redundant nesting that would be great otherwise I think things will improve organically over time, or perhaps someone will come in and make the required changes if they have an itch to scratch.

I don't think we should track this as an issue though.

[marschwar]

I am fine with the Spec suffix, but I do think we should be consistent here. Otherwise it is really hard for new contributers to know where to copy and paste from. I can provide a PR for this.

As for the top level nested I am with @3flex, this will clean itself over time. The rule would be awesome but not mandatory in my opinion.

[cortinico]

As for the top level nested I am with @3flex, this will clean itself over time.

My 2 cents here: I actually believe this should be cleaned. It's not urgent but it won't fix by itself. People will just copy over a test with a top level @Nested and will assume it's needed.

Nothing super critical though, but something we should look into doing sometime in the future.

[TWiStErRob]

Agree with @marschwar consistent test naming is really helpful, it guides users, e.g. I want to know what UnusedPrivateMember detekts, open UnusedPrivateMemberTest from GitHub, don't even need to clone the project.

---

I would imagine renaming tests is quite simple (no dependencies, no usages), I would think a simple find / mv / sed trio might do the job, and it would clear the leftover of "Spek" which is not relevant any more.

Why are your state of the art JUnit 5 tests called "Spec"?

"Historical reasons" is never a good answer ;)

[TWiStErRob]

Re nesting, have a look at this: https://youtu.be/5fIkkoPtPaw?t=907

[cortinico]

Re nesting, have a look at this: youtu.be/5fIkkoPtPaw?t=907

@dpreussler you got summoned :)

[BraisGabin]

Now that this is closed, I think that what "we" did here (I did nearly nothing) deserves a blog post. If someone is willing to write it.