Skip to content
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

Setup the correct pipeline for integration test of detekt gradle plugin #3324

Closed
schalkms opened this issue Dec 26, 2020 · 9 comments · Fixed by #3359
Closed

Setup the correct pipeline for integration test of detekt gradle plugin #3324

schalkms opened this issue Dec 26, 2020 · 9 comments · Fixed by #3359

Comments

@schalkms
Copy link
Member

@chao2zhang and me had a discussion about adding more end-to-end like tests to detekt's code base. This is definitely beneficial in terms of code quality.
Hence, the quote from PR #3319 is included in this issue. It contains a good idea, which might be followed. Of course this is an opened discussion. So please feel free to submit your ideas inside this issue.

In my mind, such end-to-end tests should be a separate module or repo that applies detekt gradle plugin, runs against some kotlin code with violations, and validates the error can be reported. This does not exist today but I definitely can help sparkle the discussion

@BraisGabin
Copy link
Member

Other option (no exclusive): checkout some opensource projects that already use detekt and run them.

Problem: we need to be carefull with false positives if they break their build.
Pros: we test agains real code.

Some options: Firefox & LeakCanary.

@schalkms
Copy link
Member Author

Problem: we need to be carefull with false positives if they break their build.
Pros: we test agains real code.

I guess this is hard to automate, since the tests directly depend on 3rd party code. If 3rd party gets changed, those tests have to be adapted in the detekt repository. For testing purposes it's enough to have a few files containing some issues and folders to test detekt end-to-end.
For rules testing your approach is fine though. This repo already contains a script to verify rule behavior on 3rd party GitHub code. However, this topic here is strictly not about testing rules.

@chao2zhang
Copy link
Member

Such integration tests need to keep some code with violation of detekt rules. I doubt a 3rd party repo would satisfy our need: Well-maintained repos tend to have good hygiene to remove detekt violations quickly.

@chao2zhang
Copy link
Member

My statement was based on out-dated information. Please allow me to correct the statement here. In addition to the common unit testing, this project embraces two ways to test our Gradle plugins:

  • Using ProjectBuilder from gradle-api, we can assert on specific Task this way.
  • Using GradleRunner from gradle-testkit, we can perform assertions based on the input and output of Gradle.

The end-to-end would probably use GradleRunner, but the current test does not seem to have high coverage. For example:

  • DslGradleRunner.ktFileContent only generates Kotlin code violating MagicNumber.
  • AutoCorrect is not verified.

End-to-end would be exceeding beneficial as we are introducing tooling and lower-level changes.

@chao2zhang
Copy link
Member

Problem

One of the problems that I encountered when prototype #3327:

  • Because the integration tests using gradle-testkit are all under detekt-gradle-plugin/src/test
  • Because AGP is introduced as compileOnly dependency in detekt-gradle-plugin
  • Problem: testing android gradle plugin using gradle-testkit does not work out of the box

Some technical lower-level details: JavaGradlePluginPlugin:204 uses main/runtimeClassPath which does not include AGP. Therefore, the tests will fail at run time. The exact exception is:

* What went wrong:
    Plugin [id: 'com.android.library'] was not found in any of the following sources:

    - Gradle Core Plugins (plugin is not in 'org.gradle' namespace)
    - Gradle TestKit (classpath: /Users/cazhang/detekt/detekt-gradle-plugin/build/classes/java/main:/Users/cazhang/detekt/detekt-gradle-plugin/build/classes/kotlin/main:/Users/cazhang/detekt/detekt-gradle-plugin/build/resources/main:/Users/cazhang/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-gradle-plugin-api/1.4.10/c06dc95764119b3188034c644805de9b76148806/kotlin-gradle-plugin-api-1.4.10.jar:/Users/cazhang/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib/1.4.10/ea29e063d2bbe695be13e9d044dcfb0c7add398e/kotlin-stdlib-1.4.10.jar:/Users/cazhang/.gradle/caches/modules-2/files-2.1/org.jetbrains.kotlin/kotlin-stdlib-common/1.4.10/6229be3465805c99db1142ad75e6c6ddeac0b04c/kotlin-stdlib-common-1.4.10.jar:/Users/cazhang/.gradle/caches/modules-2/files-2.1/org.jetbrains/annotations/13.0/919f0dfe192fb4e063e7dacadee7f8bb9a2672a9/annotations-13.0.jar)
    - Plugin Repositories (plugin dependency must include a version number for this source)

Note that the dependencies in Gradle TestKit are exactly from main/runtimeClasspath.

Solution

  1. Move all integration tests (gradle-testkit) of detekt-gradle-plugin into a separate module, which also resolves the issue mentioned here. This allows us to configure a publicly invisible module and make our tests work out of the box.
  2. Separate android plugin into a separate module. This requires a fair amount of refactoring.
  3. Customize testkit dependencies following https://docs.gradle.org/current/userguide/test_kit.html#sub:test-kit-classpath-injection. I have spent half of my day on this and due to my limited gradle knowledge, I couldn't get it fully working.

Recommendation

Solution 1 can kill two birds with one stone. We can create a detekt-gradle-integration-tests module, put some code with detekt violations and build tests verifying detekt end-to-end, including the android variant of gradle plugin.

@schalkms
Copy link
Member Author

Thank you very much for this further elaboration. I completely agree with your recommendation (solution 1). This solves the problem. I don’t think a big refactoring for solution 2 is beneficial looking towards detekt v2.0.

  1. Move all integration tests (gradle-testkit) of detekt-gradle-plugin into a separate module, which also resolves the issue mentioned here. This allows us to configure a publicly invisible module and make our tests work out of the box.

@chao2zhang
Copy link
Member

@schalkms if possible, would you mind changing the issue title to be Setup the correct pipeline for integration test of detekt gradle plugin? This issue mentioned in #3324 (comment) now happens quite often for my recent PRs.

@schalkms schalkms changed the title Adding more end-to-end like tests to detekt Setup the correct pipeline for integration test of detekt gradle plugin Jan 8, 2021
@schalkms
Copy link
Member Author

schalkms commented Jan 8, 2021

@chao2zhang I updated the title.

@chao2zhang
Copy link
Member

This issue was also discovered earlier as described in the main summary of #2493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants