Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add KtLintAssertThat for asserting unit tests in a fluent style #1444

Merged
merged 8 commits into from
Apr 26, 2022

Conversation

paul-dingemans
Copy link
Collaborator

Description

AssertJ style assertion for verifying KtLint rules. This class is intended to be used as follows:

Create an assertThat assertion for a specific rule or set of rules. If possible define it at the class level

val someRuleAssertThat = SomeRule().assertThat()
val multipleRulesAssertThat = listOf(SomeRule1(), SomeRule2).assertThat()

The created assertThat can than be used in test method similar to the normal assertThat:

@Test
fun `Some test`() {
    val code = .... // Original code to be formatted
    val formattedCode = .... // The code as it should be formatted
    someRuleAssertThat(code)
        .hasLintErrors(
            LintError(1, 1, "some-rule-id", "some-error-description")
        ).isFormattedAs(formattedCode)
}

The ktlintAssertThat provides access to methods like:

  • withEditorConfigOverride
  • asKotlinScript
  • hasNoLintErrors
  • hasNoLintErrorsBeforeFormatting
  • hasLintErrors
  • hasLintErrorsBeforeFormatting
  • isFormattedAs
  • hasNoLintErrorsAfterFormatting
  • hasLintErrorsAfterFormatting

All tests of the IndentationRule, with exception of the tests using the spec file, have been rewritten with ktlintAssertThat. Other classes will be rewritten as well but I would like to get some feedback on the idea.

Checklist

  • PR description added
  • tests are added
  • CHANGELOG.md is updated

In case of adding a new rule:

  • README.md is updated
  • Rule has been applied on Ktlint itself and violations are fixed

@paul-dingemans paul-dingemans added this to the 0.46.0 milestone Apr 3, 2022
@romtsn
Copy link
Collaborator

romtsn commented Apr 3, 2022

Sry, I don't have much time to review this in full, but if you're going to expose this to consumers (looking at public class KtlintAssertThat - you are), we should also make the assertj dependency api instead of implementation, cause people might not use assertj in their custom rulesets.

@paul-dingemans
Copy link
Collaborator Author

Sry, I don't have much time to review this in full, but if you're going to expose this to consumers (looking at public class KtlintAssertThat - you are), we should also make the assertj dependency api instead of implementation, cause people might not use assertj in their custom rulesets.

Done. Learned a again a bit about gradle ;-)
The dependencies for my test rule (the one about indenting raw string literals), now looks like:

    compileOnly "com.pinterest.ktlint:ktlint-core:$ktlintVersion"

    testImplementation 'org.junit.jupiter:junit-jupiter:5.8.2'
    testImplementation "com.pinterest.ktlint:ktlint-test:$ktlintVersion"

Should junit also be exposed as API dependency?

@romtsn
Copy link
Collaborator

romtsn commented Apr 5, 2022

Should junit also be exposed as API dependency?

No, not really, because you are not exposing any junit types as API (in case of assertj - KtLintAssertThat extends AbastractAssert, which will be part of the public API, hence we need to expose it)

# Conflicts:
#	ktlint-ruleset-standard/src/test/kotlin/com/pinterest/ktlint/ruleset/standard/IndentationRuleTest.kt
@paul-dingemans
Copy link
Collaborator Author

Should junit also be exposed as API dependency?

No, not really, because you are not exposing any junit types as API (in case of assertj - KtLintAssertThat extends AbastractAssert, which will be part of the public API, hence we need to expose it)

Tnx for explaining.

= and others added 6 commits April 7, 2022 20:35
* Removed StandardRuleSetIntegrationTest as it is can be written
  as a simple unit test without dependency on the no-used-import
  rule. The test is moved the StringTemplateRuleTest.
@paul-dingemans
Copy link
Collaborator Author

This PR consists of adding a new KtLintAssertThat API plus a massive rewrite of all tests using the new test API. Also all spec files have been replaced with normal unit tests. Due to shere volume of change, I do not expect anybody to be able/willing to review this. So, I will just merge it when builds succeeds.

Note that during this endeavour, I encountered multiple potential bug and issues. I have left TODO remarks in Test classes and added Tests marked with @disabled where applicable and created #1456 as reminder to fix them.

@paul-dingemans paul-dingemans merged commit 882eda1 into pinterest:master Apr 26, 2022
@paul-dingemans paul-dingemans deleted the ktlint-assertThat branch April 26, 2022 16:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants