Lucas3oo / sonarlint-gradle-plugin

Gradle plugin for SonarLint code analysis.
MIT License
17 stars 6 forks source link

Document correct Report generate Gradle Kotlin configuration, and small questions #7

Closed scscgit closed 1 month ago

scscgit commented 1 month ago

Documentation should be updated.

On page https://github.com/Lucas3oo/sonarlint-gradle-plugin?tab=readme-ov-file#sonarlint-ci-reports there is an example:

sonarlintMain {
  reports {
    xml.enabled = true // default false
    sarif.enabled = true // default false
  }
}

It doesn't work with Gradle 8.9 when using Kotlin syntax, i.e. file build.gradle.kts. I made it work as follows:

plugins {
    id("se.solrike.sonarlint") version "2.0.0"
}
dependencies {
    sonarlintPlugins("org.sonarsource.java:sonar-java-plugin:8.2.0.36672")
}
tasks.sonarlintMain {
    ignoreFailures = false
    reports {
        reports.create("xml") {
            enabled = true
            outputLocation = layout.buildDirectory.file("reports/sonarlint/sonarlint.xml")
        }
        reports.create("html") {
            enabled = true
            outputLocation = layout.buildDirectory.file("reports/sonarlint/sonarlint.html")
        }
    }
}

(Note: tasks.sonarlintMain<se.solrike.sonarlint.Sonarlint> also works.)

Also, I'd like to ask how is it possible to change the minimum level in failures/reports. For example, I want Major️ and Minor but not Info. Can this be documented?

Lucas3oo commented 1 month ago

Yeah, all the samples for the build file are in groovy except one for how to use with kotlin and only there I used kotlin script. The way to configure plugins in kotlin script is quite different.

Regarding the report, right now it isn't configurable what to include. But I will accept a PR.

scscgit commented 1 month ago

Yeah, hopefully someone will find the time to add that configuration parameter, as increasing minimum level seems like quite an important use-case.

I added PR with the documentation, also added tasks.sonarlintTest there, and removed the <se.solrike.sonarlint.Sonarlint> type because it seems redundant.

But I noticed one more issue:

When I define

tasks.sonarlintTest {
    ignoreFailures = false
}

my test doesn't fail despite it failing for tasks.sonarlintMain

 ./gradlew sonarlintTest

BUILD SUCCESSFUL in 787ms

Am I doing something wrong, or are some setup steps missing? Thx.

Also with this configuration, reportsDir parameter doesn't seem to do anything, I must specify explicit outputLocation if I want the file to be generated at all.

(Also, I'm curious why the default report file is sonarlintMain.xml and not just without Main - is it typical to store a different report for tests? Isn't the code the same? I'm setting up a default for a new project, so I'd like to follow good conventions, which is why I prefer for conventions to be documented here. I'm also not sure if sonarlintMain can be inherited by sonarlintTest.)

Lucas3oo commented 1 month ago

Hi

With kotlin DSL (build.gradle.kts) the so called extension (i.e. sonarlint {}) doesn't work.

ReportsDir will be used if you don't explicitly set the report file per report type.
See https://github.com/Lucas3oo/sonarlint-gradle-plugin/blob/main/src/main/java/se/solrike/sonarlint/impl/ReportAction.java#L61 and https://github.com/Lucas3oo/sonarlint-gradle-plugin/blob/main/src/main/java/se/solrike/sonarlint/impl/ReportAction.java#L184

I must say I haven't tested kotlin DSL that much. But in groovy you can just add:

sonarlintMain {
  reports {
    html.enabled = true // default false
  }
}

If you have many projects you want to configure the I suggest that you do a "convention" plugin that all of your projects depends on. I have my own here: https://github.com/Lucas3oo/solrike-conventions-gradle-plugin

I think most developer would like to have two reports; one for the "real" source code and one for tests code. SonarLint test different things also depending on main or test code. E.g SonarLint check if all your test cases have at least one assert. That check would be pointless for main code.

Lucas3oo commented 1 month ago

I was wrong , extension works it is just some other syntax that must be used. I wil update the docs with more kotlin DSL samples

scscgit commented 1 month ago

Oh, my bad, the tasks.sonarlintTest works correctly - I was testing it without any tests, because I thought that it tests all code (main + tests) as part of testing process. I think this behavior could be mentioned in javadocs, which is currently empty:

image

I guess it makes sense that we'll need to duplicate the configuration if we also want tests to be tested, it gets verbose though, one combined report for the build is often enough. Also, when you run build and sonarlintMain fails, sonarlintTest doesn't run, so its reports are missing. Plus one more observation, I guess that if I don't want to block build, but still want to provide a way to run the assertion, I'll need to add my own run parameter, or task like sonarlintMainAssert. Yeah, the convention plugin makes sense in a long term.

Also, I tested reports without any outputLocation again, and this time it generated the sonarlintMain.html + sonarlintTest.html correctly. I don't understand why it broke for a while, but I guess it behaves fine already. Thanks!

Lucas3oo commented 1 month ago

:-) I actually tested on one of my kotlin projects and I didn't get the report generated. So there are some funny things going around here. I will investigate.

If you do not want to break the build you can set "ignoreFailures = true". The purpose of that is your use case actually.

Lucas3oo commented 1 month ago

Seems like when enabling reports on the "extensions" it doesn't work but setting directly on the task then it works. In Kotlin DSL this works:

tasks.sonarlintMain {
    maxIssues.set(1)
    reports {
        reports.create("xml") {
            enabled.set(true)
        }
    }
}
Lucas3oo commented 1 month ago

I have a lot of javadoc so not sure why it can't be seen in IntelliJ. But also note that the sonarlint task is actually only one task implementation created several times in the grade project.

scscgit commented 1 month ago

I'm curious if you've been able to find a way to also write the sonarlint {} extension in Kotlin yet.

Plus I've noticed that a syntax like excludeRules = ["java:S112"] doesn't work in Kotlin either, but this one does:

Lucas3oo commented 1 month ago

Yeah, I update the readme with more accurate kotlin code. And the sonarlint {} shall work in kolin DSL too. I have some unit tests for that actually.

Normally in Kotlin DSL you need to do sorting like this for lists: tags.set(listOf("search", "tags", "for", "your", "goodbye", "plugin"))

Lucas3oo commented 1 month ago

I just released a new version where the report section for sonarlnt {} works.

scscgit commented 1 month ago

Oh, I'd swear that sonarlint {} syntax didn't work at all, but now it works even in version 2.0.0 (except the report bug). Thanks for the fix.

Btw. if I use outputLocation in a report in sonarlint, it overwrites the same file during build, so only the test report is created :) Maybe this could be handled differently. Though I just realized that SpotBugs has the same behavior if used with tasks.withType<com.github.spotbugs.snom.SpotBugsTask>, because spotbugs {} doesn't allow configuring reports at all.

Lucas3oo commented 1 month ago

Yup, I am pretty sure I didn't ge the extension to work in Kotlin DLS before too. And I am sing the same version of Gradle too.

The outputLocation shall be configured per task and not in the extension, otherwise it will overwrite. The "config" in the extension is used as default in the tasks (regardless of main, test, node or testFixture).

Yeah, I looked a lot on the spot bugs plugin when I made this plugin. Gradles own plugins in this area like Checkstyle are all cheating and are using internal API of Gradle.