Skip to content

Commit

Permalink
Check that code can still be parsed after a format (#2742)
Browse files Browse the repository at this point in the history
In rare cases it can happen that formatted code can not be compiled anymore. Add additional checking in `KtLintAssertThat` that linting of formatted code still succeeds during unit tests. For release testing, the CLI has been expanded with a new (hidden) option that forces lint to run after format, and to stop execution as soon as the formatted code can not be compiled anymore.

Note that this still does not guarantee 100% that formatted code will be compilable. The additional tests will only ensure that they won't go unnoticed when the problem occurs in unit tests, or in the sample projects that are used for release testing.

Closes #2691
  • Loading branch information
paul-dingemans authored Jul 23, 2024
1 parent 0b575a0 commit e80d994
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 20 deletions.
24 changes: 12 additions & 12 deletions RELEASE_TESTING.MD
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ Before releasing a new version of KtLint, the release candidate is tested on a s
```shell
# Examples of usage
#
# Do not run ktlint commands with this script. Instead run them direct from the `sample-projects` directory
# ktlint --version && ktlint "**/*.kt" -v --relative --format --force-lint-after-format
#
# Outstanding changes per project
# ./exec-in-each-project.sh "git status"
#
# Rollback outstanding changes per project
# ./exec-in-each-project.sh "git reset --hard"
#
# Run ktlint standard rules
# ktlint --version && ktlint "**/*.kt" -v --relative -F
#
# Commit changes of standard rules:
# ./exec-in-each-project.sh "git commit -m \"ktlint (0.43.2) -F\""
Expand Down Expand Up @@ -109,7 +109,7 @@ Formatting projects in which ktlint is not used may result in a huge amount of f
Note: Ignore all output as this is the old version!
4. Format the sample projects with the previous (*latest released*) ktlint version:
```shell
ktlint-prev -F --baseline=baseline.xml --relative # Do not call this command via the "./exec-in-each-project.sh" script.
ktlint-prev --format --force-lint-after-format --baseline=baseline.xml --relative # Do not call this command via the "./exec-in-each-project.sh" script.
```
Note: Ignore all output as this is the old version!
5. Commit changes:
Expand Down Expand Up @@ -140,7 +140,7 @@ Formatting projects in which ktlint is not used may result in a huge amount of f
```
9. Format with *latest development* version:
```shell
ktlint-dev -F --baseline=baseline.xml --relative # Do not call this command via the "./exec-in-each-project.sh" script as we want to use the one combined baseline.xml file for all projects.
ktlint-dev --format --force-lint-after-format --baseline=baseline.xml --relative # Do not call this command via the "./exec-in-each-project.sh" script as we want to use the one combined baseline.xml file for all projects.
```
Inspect the output carefully:
* If you see an error like below, then this version obviously may *not* be released. It is best to fix this error before continuing with testing and validating!
Expand All @@ -165,14 +165,14 @@ Formatting projects in which ktlint is not used may result in a huge amount of f
Internal Error (...) in file '...' at position '0:0. Please create a ticket at https://github.com/pinterest/ktlint/issues ...
```
12. Rerun lint with *latest development* version and verify that no violations are reported:
```shell
ktlint-dev --baseline=baseline.xml --relative # Do not call this command via the "./exec-in-each-project.sh" script as we want one combined baseline.xml file for all projects.
```
No violations should be reported in this run.
```shell
ktlint-dev --baseline=baseline.xml --relative # Do not call this command via the "./exec-in-each-project.sh" script as we want one combined baseline.xml file for all projects.
```
No violations should be reported in this run.
13. Rerun format with *latest development* version without baseline:
```shell
ktlint-dev -F --relative # Do not call this command via the "./exec-in-each-project.sh" script as we want to use the one combined baseline.xml file for all projects.
```
```shell
ktlint-dev --format --force-lint-after-format --relative # Do not call this command via the "./exec-in-each-project.sh" script as we want to use the one combined baseline.xml file for all projects.
```
As the baseline is removed, thousands of violations are to be expected. Check at least in the summary that no internal errors are thrown like below:
```plain
Internal Error (...) in file '...' at position '0:0. Please create a ticket at https://github.com/pinterest/ktlint/issues ...
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,14 +200,21 @@ internal class KtlintCommandLine :
)

@Deprecated("Remove in Ktlint 1.3 (or later) as some users will skip multiple versions.")
var experimental =
private var experimental =
option("--experimental", hidden = true)
.flag()
.deprecated(
"Option '--experimental' is no longer supported. Set '.editorconfig' property 'ktlint_experimental' instead.",
error = true,
)

private val forceLintAfterFormat: Boolean by
option(
"--force-lint-after-format",
help = "Force to run lint after code has been formatted to check that it still can be successfully parsed",
hidden = true,
).flag()

private val baselinePath: String by
option("--baseline", help = "Defines a baseline file to check against")
.default("")
Expand Down Expand Up @@ -501,6 +508,18 @@ internal class KtlintCommandLine :
}
} ?: NO_AUTOCORRECT
}.also { formattedFileContent ->
if (forceLintAfterFormat && code.content != formattedFileContent) {
// Rerun lint to check that the formatted code can still be successfully parsed.
try {
ktLintRuleEngine.lint(Code.fromSnippet(formattedFileContent, code.script))
} catch (e: KtLintParseException) {
logger.error(e) { "After formatting code in file '${code.filePath}' it cannot be successfully parsed anymore." }
// Intentionally stop formatting other files. The '--force-lint-after-format' is only supposed to be used during
// release testing of Ktlint CLI. Immediate exist forces to fix the issue immediately as release test can
// otherwise not be completed.
exitKtLintProcess(123)
}
}
when {
code.isStdIn -> print(formattedFileContent)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import dev.drewhamilton.poko.Poko
import io.github.oshai.kotlinlogging.KotlinLogging
import org.assertj.core.api.AbstractAssert
import org.assertj.core.api.Assertions.assertThat
import org.assertj.core.api.Assertions.assertThatNoException
import org.junit.jupiter.api.assertAll
import kotlin.io.path.pathString

Expand Down Expand Up @@ -467,18 +468,32 @@ public class KtLintAssertThatAssertable(
},
).isEqualTo(code.content)
},
{
assertThatNoException()
.describedAs("After reformat of code, it can no longer be successfully parsed")
.isThrownBy { createKtLintRuleEngine().lint(Code.fromSnippet(actualFormattedCode, code.script)) }
},
)
}

/**
* Asserts that the code does not contain any [LintViolation]s for the given rule id.
*/
public fun hasNoLintViolationsForRuleId(ruleId: RuleId): KtLintAssertThatAssertable {
val (_, lintErrorsWhenFormatting) = format()
val (actualFormattedCode, lintErrorsWhenFormatting) = format()

assertThat(lintErrorsWhenFormatting.filter { it.ruleId == ruleId })
.describedAs("At least 1 lint violation was found for rule id '${ruleId.value}' while none were expected")
.isEmpty()
assertAll(
{
assertThat(lintErrorsWhenFormatting.filter { it.ruleId == ruleId })
.describedAs("At least 1 lint violation was found for rule id '${ruleId.value}' while none were expected")
.isEmpty()
},
{
assertThatNoException()
.describedAs("After reformat of code, it can no longer be successfully parsed")
.isThrownBy { createKtLintRuleEngine().lint(Code.fromSnippet(actualFormattedCode, code.script)) }
},
)

return this
}
Expand Down Expand Up @@ -593,9 +608,20 @@ public class KtLintAssertThatAssertable(
// Also format the code to be absolutely sure that codes does not get changed
val (actualFormattedCode, _) = format()

assertThat(actualFormattedCode)
.describedAs("Code is formatted as")
.isEqualTo(formattedCode)
assertAll(
{
assertThat(actualFormattedCode)
.describedAs("Code is formatted as")
.isEqualTo(formattedCode)
},
{
assertThatNoException()
.describedAs("After reformat of code, it can no longer be successfully parsed")
.isThrownBy {
createKtLintRuleEngine().lint(Code.fromSnippet(actualFormattedCode, code.script))
}
},
)

return this
}
Expand Down

0 comments on commit e80d994

Please sign in to comment.