Sonarcloud

[pieter-bos]

In my opinion, Sonarcloud is currently failing in its role as a useful step in our continuous integration, and we need it to change drastically. Please note that I'm not against linting in general, but these issues have become too annoying to leave them be. Some issues from the top of my head:

• Coverage is measured via the sbt test suite, instead of our normal examples test suite. All PRs fail the sonarcloud check because of this
• We haven't encoded local naming conventions properly. For example, StandardOperator uses a different naming convention to regular enum's, causing sonarcloud to fail any PR with a new operator, new ast factory method, etc.
• Linting is too strict when changing a file someone else wrote. "Fixing stuff locally" is fine, but I think we should be careful reformatting large files multiple people may be working on, leading to large merge conflicts (think AbstractRewriter, Main, that sort of thing)
• Sonarcloud does not work in forks, so it fails any PRs from forks. Those typically are merged without checking within the vercors project.

This leads to a situation where in almost any non-trivial pull request Sonarcloud fails the pull request. When deciding whether to merge the conversation usually is "oh it's just sonarcloud complaining? let's just merge it," so we merge pull requests even though the CI fails it. I think this, at a bare minimum, should not be the case.

Here are a few solutions:

• We fix all the sonarcloud issues, leaving it as a requirement for PRs. I think this solution is not viable, as these issues have been long-standing and somewhat "boring," and the cost-benefit ratio is not really there.
• We investigate whether we can make sonarcloud optional, so it shows up as a CI check, but it acts as a warning in that errors do not fail the PR.
• We disable sonarcloud altogether.

I'm curious to hear your opinions!

[bobismijnnaam]

Maybe one more solution that can be considered is further tweaking sonarcloud. This would mean properly hooking up our tests to sonarcloud, telling sonarcloud about our naming conventions, and white/blacklisting files/sections of files that we do not want to be linted (took me a while to find, but sonarcloud supports it: https://sonarcloud.io/documentation/project-administration/narrowing-the-focus/). The only real blocker that you mentioned, in my opinion, is that sonarcloud does not work in forks. This could be worked around by only considering pull requests from local branches (something I as a fork user wouldn't mind), but this would still rule out checking forks with sonarcloud.

We could also look at lighter alternatives that are easier to work with or can be run locally. But sonarcloud seems to be more or less the standard at the moment.

(edit: CheckStyle comes to mind, but I do not have very good memories about that tool... Unless they've improved)

[bobismijnnaam]

One thing that I initially liked sonarcloud for is for insight in coverage. However, as that has been implemented in the vercors test suite, we don't necessarily need sonarcloud to check if the coverage is above a certain value. Besides that, if we want to do formatting/linting, we can also settle for scalafmt (and maybe checkstyle for java) or something similar.

(Another thing I liked sonar cloud for is its static analysis capabilities, which I think we, as static analysis researchers, should employ. But sonarcloud only seems to catch the most mundane bugs, so we're not missing too much I think.)

[bobismijnnaam]

@Vescatur If you still remember, could you summarize what changes you have applied to achieve closing this issue? I remember you made sonarcloud more lenient but not exactly how anymore (might be nice for Pieter to read back later, and also for future reference)

[Vescatur]

Changes to sonar are:

1. Give sonar more source control management information by also giving it the git history. (Before we were only doing a shallow copy)
2. The rules related to coding conventions have been given a severity level blocker. This makes them more visible. Sonar has been configured to fail if a rule with severity level blocker is broken. These two changes ensure that all the new code must comply with the coding convention.
3. The rule regarding todo comments has been removed.
4. Updated the wiki to give information about sonarlint. SonarLint is a linter integrated in the ide which synchronizes with SonarCloud. https://github.com/utwente-fmt/vercors/wiki/IDE-Import#configure-sonarlint-for-vercors-in-intellij