New SonarCloud quality rules

Development
1

Hi devs,

Currently our quality gate on sonarcloud is: no blocker issues on new code.

However there are 2 topics to discuss:

  1. Sonarqube doesn’t use the concept of blocker issues anymore. They now have High, Medium, Low. Their mapping is defined at Issues (High = the old blocker + critical)

  2. They have introduced the concept of “Clean as you code” which they’re proposing to set up as a quality gate with the following rules:

    Screenshot 2024-09-05 at 17.31.59
    Screenshot 2024-09-05 at 17.31.591110×1528 102 KB

I’m proposing to follow it (it’s on new code only), and to keep the 80% coverage and 3% duplicated lines proposed. Note that it’s always possible to set a rule as a false positive (in case that for some reason the test coverage is badly measured for example, hopefully that shouldn’t happen soon and if it does then we can review our rule).

WDYT?

If you’re worried, we could also decide to test this for 1 month and decide if we keep it or not after that test period.

Thanks

PS1: If you want to see all the High rules: SonarCloud

PS2: We currently have 3 high severity issues on new code for platform: SonarCloud

There’s also 1 in commons: SonarCloud

And 1 in rendering: SonarCloud

2

80% sounds a lot for some cases, but let’s try

3

Fixed

4

+1 to try it out

5

Fixed

6

Would be good if some javascript expert could validate the 2 issues related to javascript (let vs var).

7

They are definitely valid. New JavaScript code shouldn’t use var any more (normally). I saw @CharpentierLucas was assigned to these two issues, so I assumed he’s taking care of it. I don’t know how this assignment is done. Maybe it’s automatic (based on who committed the code) and @CharpentierLucas is not aware of it? Otherwise, I can fix it quickly.

8

Yeah it was assigned automatically and it did not ping me ^^’ . I can check it out after lunch

Thank you for notifying me :slight_smile:

PS: I changed my notification parameters so that hopefully a similar situation wouldn’t happen again

1 Like
9

FTR I’ve fixed the coverage reported by Sonarcloud, see Loading...

See also the question:

Note that a second coverage bug is that we don’t execute the “integration-tests” and “docker” profiles for the quality build. While this affects little commons and rendering, it does affect a lot platform.

This part is not fixed, we first need to decide if we’d be ok to lengthen the build for this.

10

I’ve now enabled the new quality gate rules on sonarcloud:

Screenshot 2024-09-10 at 15.47.54
Screenshot 2024-09-10 at 15.47.541882×1598 221 KB

Let’s try to follow this and see how it goes.

Thanks

11

Hi,

I’m reviving this topic since there was several discussions about it on the chat, and I proposed to disable the rule for now for next release.

For me there’s several problems with that new rule:

  1. it’s looking for an average coverage of all new code in last 30 days: meaning that if you have 100% coverage of code at day 1, and 70% coverage at day 2, quality gate will be ok until day 30 where coverage will drop to 70 and suddenly it’s not ok anymore
  2. the new code definition is really not helpful if we want to make refactoring or move stuff to legacy: any code touched is considered new and impacted by the rule
  3. we can’t easily compute on our machines if we’ll comply with the rule: it’s hard to ensure that raising the coverage will fix properly the gate as it’s on a window and that window doesn’t take into account when the option was enabled…
  4. any breakage on that rule is leading to a quality gate failure, which right now translates to an erroring build

My feeling right now is that adding this rule won’t help to have more coverage, but on the contrary we will just ignore more often erroring builds which is really not good.
Knowing that we also have the jacoco coverage threshold to ensure that we don’t drop coverage, I’d be more in favor of working on those and there’s plenty of things we could do around this:

  1. provide a tool to automatically compute the new average threshold in each module (which we don’t update when adding new code / new tests AFAIK)
  2. fail the build if a new module is added with a threshold below a limit

Another approach could also be to integrate the sonar coverage rule but not as something make the build erroring, but more as something informative: a badge on the build for example.

12

it’s not one new rule but several. It seems you’re referring only to the rule: “80% coverage required on new code”, right?

13

Yes indeed, I’m only referring to that specific rule.

14

FTR I have temporarily disabled the coverage rule while we discuss this further.

15

This would be useless IMO. Right now sonarqube provides tons of information already (and has been for the past 10 years or so) and it’s not helping improve anything. The only thing that matters when you want to improve is something that breaks the build.

This is what I call Active quality vs Passive quality

Idea:

Thx

16

Honestly I’m not sure how it would help here: it was providing nice reports indeed, but it’s exactly same situation as you explained yourself. AFAIK we never used those reports breakage actively, so it’s kind of passive quality.

Taking a wider perspective for me the main problem in terms of quality of XWiki right now is about its size vs the number of contributors: we have more and more best practices and rules to comply to, which is good for the quality of new stuff, but we’re lacking time to apply those new rules in old stuff and to properly do refactoring, especially in parallel of adding new features and improvments.

So for me the Sonar rule is a good idea once we already manage to fix other problems and we only have that to focus on. Which won’t happen soon sadly.

Maybe one thing we could do, to not drop it entirely, is to enable the rule but with a very low expectation bar, just to see how it goes: something like 50% of coverage. It would be interesting to see how we comply with that rule on the long run: hopefully it will never fail, but I’m expecting that from time to time we’d get a failure, because of a refactoring, or something like that. And when it happens it will force us to have a look on our coverage and maybe improve it.

17

Fyi, we’re currently at 35.9% according to SonarCloud. I don’t know if this has been improved since the last build, but it would still fail the build currently.

Generally, I’m not against it, I just want to point out that this would still fail the build currently.

18

Indeed, and that would probably be a good thing as we’re indeed very very low right now (e.g. I can see that we have 300 lines of new code in xwiki-platform-repository-server-api not covered at all). So it proves that the rule would be useful to spot that. But it also shows that growing from 35.9% to 80% would be just too much of work. 50% sounds like a good first start, knowing that our global coverage computed by sonar is currently at 51.2% for xwiki-platform.

19

Keep in mind that integration tests are not taken into account right now.

20

That’s not true. We voted to define the strategy and apply it. See https://www.mail-archive.com/devs@xwiki.org/msg36072.html and especially:

  • The RM is in charge of a release from day 1 to the release day (already the
    case), and is also in charge of making sure that the global coverage job
    failures get addressed before the release day so that we’re ready on the
    release day.

I agree with old stuff, which is why we’ve never set breaking rules on old stuff, only on new stuff.

I’d like to propose and apply the following changes to our CI build:

  • Drop the legacy profile for the quality build. I don’t think it’s very useful and it also reduces our code coverage when we don’t care about increasing it for legacy code.
  • Drop repository-snapshots profile for the quality build (I don’t understand why it’s there TBH). Also it needs to be documented at https://dev.xwiki.org/xwiki/bin/view/Community/Building/#HUsingProfiles (whoever added it, please it there)
  • Run integration-tests & docker tests for the quality build, in order to make sure that we compute the real coverage, i.e.:
        buildInsideNode(
      name: 'Quality Step 2',
      goals: 'jacoco:report sonar:sonar',
      profiles: 'repository-snapshots,quality,legacy,coverage',
      properties: '-Dxwiki.jacoco.itDestFile=`pwd`/target/jacoco-it.exec'
    )
  • In order to not slow down the CI build by running IT tests for coverage, remove integration-tests,docker from the TestRelease build. Since they’d be executed for the quality build.

WDYT?