search

vaadin / flow-components

Java counterpart of Vaadin Web Components
101 stars 65 forks source link

SonarCloud validation problems #3428

Open sissbruecker opened 2 years ago

sissbruecker commented 2 years ago

With its current setup, the SonarCloud validation is causing a lot of noise, making it mostly useless. It reports issues in code that is not changed by the current PR, causing the validation to fail for every PR. Another problem is that some of the issues that are categorized as "Code smells" are marked as a "Blocker", which causes the validation to fail for problems that are arguably not that important. With so many issues reported on every PR, the tool loses its utility, which would be that devs fix the issues that their PR introduces, and instead the validation is basically ignored.

Validation scope

By scope I refer to the code that should be checked during validation. In web-components the validation is set up so that only the modified code is checked, and the expectation is that the same should happen in flow-components. However as demonstrated in this PR, changing a few lines of code in the select component results in SonarCloud reporting over 1700 code smells all over the codebase. The numbers are also not consistent between PRs, for example:

So it doesn't look like the whole codebase is checked every time, or only the recently changed code, but somehow a range of modules.

I would suggest to check the SonarCloud configuration and modify it so that only the changed code is checked for pull requests.

Code smells reported as blockers

Code smells in SonarCloud have a severity, which can be "Blocker", which causes the validation to fail. Some of these blockers are:

While those are certainly valid points, it's arguable if these should really be categorized as blockers (=block validation / merging). For rules like Add at least one assertion to this test case. this is also not straight-forward as we have some tests where assertions are extracted into utility class methods, which the rule does not seem to recognize.

If we can modify the validation scope to only check the changed code, then it might be OK to keep the rules as is. Otherwise we might want to take a closer look and change the severity of some of these rules in order to reduce noise.

sissbruecker commented 2 years ago

According to this validation summary, there is a warning (top right) that indicates that there is a setup issue that might result in unexpected issues and changes:

Could not find ref 'master' in refs/heads, refs/remotes/upstream or refs/remotes/origin. You may see unexpected issues and changes. Please make sure to fetch this ref before pull request analysis.

manolo commented 2 years ago

It seems that depending on which file is modified it runs different code scan, probably when a pom is modified it does a full java scan in the modules that depend on it, but for a change in the css it does not, because there are no files in the sonar cloud code tab