Replace Spek with a new test framework

3flex commented 2 years ago

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, https://github.com/spekframework/spek/issues/959#issuecomment-997344639

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:

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.
Don't accept contributions with new Spek tests - only allow the new framework.
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

[X] custom-checks (#4467)
[x] detekt-api (#4521)
[x] detekt-cli (#4519)
[x] detekt-core (#4507)
[X] detekt-formatting (#4508)
[x] detekt-generator (#4494)
[X] detekt-gradle-plugin (#4529)
[X] detekt-metrics (#4511)
[X] detekt-parser (#4512)
[X] detekt-psi-utils (#4515)
[X] detekt-report-html (#4513)
[X] detekt-report-sarif (#4513)
[X] detekt-report-txt (#4513)
[X] detekt-report-xml (#4513)
[X] detekt-rules (#4514)
[x] detekt-rules-complexity (#4566)
[X] detekt-rules-coroutines (#4567)
[X] detekt-rules-documentation (#4568)
[x] detekt-rules-empty (#4522)
[x] detekt-rules-errorprone (#4523)
[x] detekt-rules-exceptions (#4572)
[X] detekt-rules-naming (#4574)
[x] detekt-rules-performance (#4569)
[X] detekt-rules-style (#4575)
[x] detekt-sample-extensions (#4525)
[x] detekt-tooling (#4524)

marschwar commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

I will start with detekt-generator

BraisGabin commented 2 years ago

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 commented 2 years ago

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

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

BraisGabin commented 2 years ago

Done.

marschwar commented 2 years ago

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 commented 2 years ago

Claiming detekt-tooling

BraisGabin commented 2 years ago

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 commented 2 years ago

claiming detekt-cli

marschwar commented 2 years ago

claiming detekt-api

3flex commented 2 years ago

Claiming detekt-gradle-plugin

TWiStErRob commented 2 years ago

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

BraisGabin commented 2 years ago

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

Do we want to rename the classes/files from Spec to Test/s? There is a comment about this here: https://github.com/detekt/detekt/pull/4670#issuecomment-1086975557
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:

No need. I don't think that we should even bother to be consistent here.
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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

cortinico commented 2 years ago

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

@dpreussler you got summoned :)

BraisGabin commented 2 years ago

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.

detekt / detekt