Order & urgency of code quality improvements

[craigfowler]

Now that SonarQube is in and reporting to SonarCloud, it's flagged up issues with existing code. These don't need to be fixed immediately in order for builds to pass because SonarQube will only fail on problems with newly-submitted code.

It's good that we know about them and now we can start thinking about paying some of them down. SonarCloud sort of gives us a priority list to work through (not precisely, but we can infer one). There's no need to "stop the world" until they're all done either, so we can do a few at a time.

The issues SonarCloud reports

SonarQube has shown us that we have:

• 11 "bugs". Where bugs are concerned, SonarQube has a high confidence that the code doesn't do what we wanted it to
• 14 "security hotspots". These are areas where we might have a security issue, but SonarQube can't be certain.
  • They might need work or they might be "no change required".
  • The idea is that we should review them and make a decision on each.
  • SonarCloud provides plenty of guidance to help make that decision.
• 299 "code smells". These are assorted things which it thinks could be done better or more clearly. Thankfully, SonarCloud's website divides these down by severity:
  • 1 has the highest severity: Blocker
  • 112 have the next-highest: Critical
  • 136 have the next-highest: Major
  • 29 have the next-highest: Minor
  • 21 have the lowest: Info

Proposal: Open a few issues to fix the worst of it

I'd suggest raising the following three issues as a "first wave of code improvements" (so we can say we've made things better), to be worked-on in this proposed order:

1. Review all 14 of the security hotspots.
   • If it needs a fix and the fix is quick then just do it immediately in the issue
   • If it needs a fix but it's a serious chunk of work then create a new issue specifically for that security issue
   • Some or even all of them might be "we determined that this isn't a security issue" and just need to be closed on SonarCloud
2. Fix the 11 bugs. There is a chance that any of these could be false-positives but they aren't expected to be.
3. Fix the one blocker code smell.

The rest can probably be left for now. This way we target the few most important issues, pay off some tech debt, but don't try to do or plan so much that we lose sight of our overall goals.

When/priority

I don't have any strong opinions on whether any/all of this should be added to the Elsa 2.0 milestone. On one hand the sooner these are addressed the better, but on the other scope creep in a release is bad, and we're quite close to the planned delivery date.

If someone left it to me I'd probably put all three of those in the v2 release, but I'm more than happy to leave it until afterward.

Any thoughts?

Totally welcome to hear thoughts on any of this. The order we should do it, whether we should even be thinking about this stuff now (before v2 is out of the door) or opinions on the size/scope/priority of issues to be created.

[craigfowler]

A few other minor thoughts while we're thinking about SonarQube. Nothing super-urgent, and we can adopt a wait-and-see stance to them as well if we want.

Is the analysis scope right?

In other words, which files are analysed and which are not, and how they are analysed.

• I left it configured to do code analysis inside of sample projects.
  • Rationale: even if it's only sample code, it should demonstrate best practice
• I excluded test coverage of Exception classes
  • Rationale: They're just constructors and usually pure boilerplate
• I excluded test coverage of sample projects
  • Rationale: Sure it's code, but sample projects probably don't need test coverage

Signs that the analysis scope is wrong is frequently looking at things SonarCloud reports and thinking "oh, but I don't care about this code anyway". Here's an article about improving on that: https://docs.sonarqube.org/latest/project-administration/narrowing-the-focus/

I have a feeling that SonarQube is wrong on coverage stats

It's currently reporting over 30% code coverage off the back of just 18 tests. All of the ones I've written are pure unit tests, and that's the majority of them. Sure they would provide high coverage ratios for the classes tested, but they only test a tiny amount of the codebase. I suspect that something is wrong and it is grossly over-reporting.

I'm not going to fix or even investigate it immediately, because we're not yet including coverage as a pass/fail criteria. Plus getting code analysis has taken up enough time for now, and we have enough benefits from it for now. I'll suggest revisiting after 2.0 is released.

[sfmskywalker]

I think your current assessment of the analysis scope is fair and I agree to leave this as-is for now - it's already providing a good number of benefits just having it configured. Well done.

[sfmskywalker]

The only thing I think we should do before the V2 release is to at least triage the 3 categories: security hotspots, bugs and the one blocker. The issues we find during triage that we deem important to be done before the release we assign the 2.0 milestone.
Then the remaining issues can be picked up by anyone who want to work on them.