-
-
Notifications
You must be signed in to change notification settings - Fork 757
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop Spek #4670
Drop Spek #4670
Conversation
To my understanding (or wishful thinking) we decided to stay with the |
I'm not too bothered personally, it seems the JUnit convention is for Tests (plural) to be the suffix, though that would depend on the number of tests in the class... Also this convention makes sense for inner classes too so perhaps it's not just files and top level classes to rename. I don't think there's anything wrong with Spec though. If the current suffix was Spek that would be different! |
@@ -11,6 +11,9 @@ import org.spekframework.spek2.dsl.Root | |||
import org.spekframework.spek2.lifecycle.CachingMode | |||
import java.nio.file.Path | |||
|
|||
@Deprecated( | |||
"Use @KotlinCoreEnvironmentTest annotation on test classes instead. See detekt's tests for examples." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on wording? This is only relevant if JUnit 5 is being used. Perhaps this message should be test framework agnostic and/or direct readers to the type resolution testing docs which outlines options for different test frameworks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe talks about KotlinCoreEnvironmentTest
and createEnvironment
so we don't force the use of JUnit 5
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, this shouldn't be a deprecation imho but more of a KDoc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't the deprecation suitable? Are we intending to support those extending detekt and testing with Spek indefinitely?
My preference is a deprecation with removal in the near future (with a copy of the extension function in the release notes that users can then use in their own test code if they're still using Spek).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup sorry, I haven't realized this was Spek-specific. Therefore I'm in for deprecating this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look at the updated deprecation message - I've directed readers to the type resolution docs and copied the Spek DSL extension code there so it becomes a self-contained example that doesn't rely on detekt-test-utils
. This will make it easier to remove the deprecated code in a future release.
69246d5
to
ab36488
Compare
Codecov Report
@@ Coverage Diff @@
## main #4670 +/- ##
============================================
+ Coverage 84.42% 84.71% +0.28%
+ Complexity 3436 3423 -13
============================================
Files 493 490 -3
Lines 11342 11255 -87
Branches 2085 2069 -16
============================================
- Hits 9576 9535 -41
+ Misses 704 675 -29
+ Partials 1062 1045 -17 Continue to review full report at Codecov.
|
@@ -11,6 +11,9 @@ import org.spekframework.spek2.dsl.Root | |||
import org.spekframework.spek2.lifecycle.CachingMode | |||
import java.nio.file.Path | |||
|
|||
@Deprecated( | |||
"Use @KotlinCoreEnvironmentTest annotation on test classes instead. See detekt's tests for examples." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, this shouldn't be a deprecation imho but more of a KDoc
I'd like to merge this and prepare 1.20.0 just after it 👍 |
} | ||
} | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to fully drop support for Spek
test? Line 129 - 141 still have some code references.
At least we can update them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry didn't see your note before pushing an update - I've actually expanded the docs to make deprecation of org.spekframework.spek2.dsl.Root.setupKotlinEnvironment
easier for anyone using it in external projects.
The only custom check it included was for Spek, which has been removed from the code base.
The migration has been completed.
These are redundant as Spek is no longer used.
JUnit 5 is recommended, along with the @KotlinCoreEnvironmentTest annotation, to setup a Kotlin environment for testing rules that use type and symbol solving.
Co-authored-by: Goooler <wangzongler@gmail.com>
Co-authored-by: Goooler <wangzongler@gmail.com>
LGTM? I'll let someone else click the button after a final review. |
Tying off some loose ends to close #4450.