Load Reporters dynamically, using provided ktlint version
#294
Conversation
mateuszkwiecinski
force-pushed
the
run_lint_with_process_isolation
branch
from
203414b to
9c8f09d
Compare
mateuszkwiecinski
force-pushed
the
separate_reporters
branch
3 times, most recently
from
6e21219 to
5b55da6
Compare
| internal fun reporterPathFor(reporterType: ReporterType, output: File, relativeRoot: File) = when (reporterType) { | ||
| ReporterType.Checkstyle, | ||
| ReporterType.Html, | ||
| ReporterType.Json, | ||
| ReporterType.Plain, | ||
| -> output.toRelativeString(base = relativeRoot) | ||
|
|
||
| ReporterType.Sarif -> output.absolutePath | ||
| } |
There was a problem hiding this comment.
I mindlessly copied existing logic with those paths 👀 Old logic did an instance checks, but the plugin now doesn't know what any of Reporter implementations
| assertTrue(reportContent.contains(""""version": "0.47.0"""")) | ||
| assertTrue(reportContent.contains(""""semanticVersion": "0.47.0"""")) |
There was a problem hiding this comment.
I noticed ktlint version appears in generated report so I decided to use to verify the behavior changes when the ktlintVersion gets overriden
| private fun expectedEmptyPlain() = "" | ||
|
|
||
| // language=xml | ||
| private fun expectedEmptyCheckstyle() = """ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <checkstyle version="8.0"> | ||
| </checkstyle> | ||
|
|
||
| """.trimIndent() | ||
|
|
||
| // language=html | ||
| private fun expectedEmptyHtml() = """ | ||
| <html> | ||
| <head> | ||
| <link href="https://fonts.googleapis.com/css?family=Source+Code+Pro" rel="stylesheet" /> | ||
| <meta http-equiv="Content-Type" Content="text/html; Charset=UTF-8"> | ||
| <style> | ||
| body { |
There was a problem hiding this comment.
Asserting on file content isn't necessarily the most reliable way to ensure proper behavior, and we probably should only compare smaller parts of the content. I wanted to avoid making decisions what to test here specifically, and decided to start with full-content comparison and then migrate to either smaller chunks or start parsing the reports? Ideas are welcome
There was a problem hiding this comment.
Yeah, seems a little heavy. If we just check the content for a small known snippet, what's the real chance we'd have a false positive test pass?
src/test/kotlin/org/jmailen/gradle/kotlinter/functional/ReportersTest.kt
Show resolved
Hide resolved
mateuszkwiecinski
force-pushed
the
run_lint_with_process_isolation
branch
from
9c8f09d to
dd2b917
Compare
mateuszkwiecinski
force-pushed
the
separate_reporters
branch
2 times, most recently
from
2947c77 to
55aad23
Compare
mateuszkwiecinski
force-pushed
the
run_lint_with_process_isolation
branch
from
dd2b917 to
1036d33
Compare
mateuszkwiecinski
force-pushed
the
separate_reporters
branch
3 times, most recently
from
2540e51 to
56a2c33
Compare
mateuszkwiecinski
force-pushed
the
run_lint_with_process_isolation
branch
from
6b229e8 to
ff27fdf
Compare
This test should use any other _supported_ ktlint version, the only requirement is it doesn't match the one used by the plugin
mateuszkwiecinski
force-pushed
the
separate_reporters
branch
from
23ebadc to
c6ea95e
Compare
There was a problem hiding this comment.
Looks good. Let's get this one merged into the bigger change along with #289 and we get that big piece landed for 4.0.0.
| private fun expectedEmptyPlain() = "" | ||
|
|
||
| // language=xml | ||
| private fun expectedEmptyCheckstyle() = """ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <checkstyle version="8.0"> | ||
| </checkstyle> | ||
|
|
||
| """.trimIndent() | ||
|
|
||
| // language=html | ||
| private fun expectedEmptyHtml() = """ | ||
| <html> | ||
| <head> | ||
| <link href="https://fonts.googleapis.com/css?family=Source+Code+Pro" rel="stylesheet" /> | ||
| <meta http-equiv="Content-Type" Content="text/html; Charset=UTF-8"> | ||
| <style> | ||
| body { |
There was a problem hiding this comment.
Yeah, seems a little heavy. If we just check the content for a small known snippet, what's the real chance we'd have a false positive test pass?
|
I believe this is also now solved for in 5.0.0 |
Follow up on #279 (comment) - which moves all
ktlintclasses to be loaded in process isolation, within workers. The same wayRuleSetProviderV2s are.