[FOLIO-864] Managing Sonarqube exceptions and rule customization Created: 27/Sep/17 Updated: 15/Jan/19 |
|
| Status: | In Progress |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Task | Priority: | P3 |
| Reporter: | John Malconian | Assignee: | John Malconian |
| Resolution: | Unresolved | Votes: | 0 |
| Labels: | ci, core, devops | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | 1 hour | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||
| Sprint: | |||||||||
| Development Team: | Core: Platform | ||||||||
| Description |
|
With Sonarcloud, there are default Quality Profiles for each programming language. These profiles are referred to as the 'Sonar way' and are updated regularly as plugins. There will be instances when we need to override or customize the default Quality Profiles that are used to analyze FOLIO projects. |
| Comments |
| Comment by John Malconian [ 27/Sep/17 ] |
|
I've created two Java Quality Profile called 'FOLIO Java' based on the default Sonar profile. It's not clear to me whether I should create a custom profile for each project or just one for FOLIO. In other words, if we make exceptions in one project, should we apply to all other similar projects? I'm not sure the correct answer to this, but for now, I will maintain one custom Quality Profile for all Java projects and do the same for Javascript projects, etc. The 'FOLIO Java' profile configured as with the default Sonar profile as its parent. The advantage to this is that as Sonar updates rules in its default profile, those rules will trickle down into the custom profile. The disadvantage is that I cannot deactivate specific rules completely. I can only change their thresholds and priority. Adam has a specific rule he wants to add as an exception to the Okapi project - squid:S1192 'String literals should not be duplicated'. I've changed the priority of the rule from 'critical' to 'info'. |
| Comment by John Malconian [ 27/Sep/17 ] |
|
There is no way to change the default Quality Gate in Sonarcloud which consists of 'A' ratings for Reliability, Security, and Maintainability and 80% coverage on new code which is a bit of a bummer. We'd have to install a local instance of Sonarqube in order to customize Quality Gates. |
| Comment by Heikki Levanto [ 28/Sep/17 ] |
|
Any way to reduce the required test coverage to 75% or 60% ? It could be increased slowly once we have reached that in most things. Some of the code it impossible to cover in tests, mostly error handling (catch the exception when the second database operation suddenly fails) |
| Comment by John Malconian [ 28/Sep/17 ] |
|
I've changed the threshold for Sonarqube rule squid:S3776: Cognitive Complexity of methods should not be too high from '15' to '30' and downgraded priority from 'critical' to 'major' in the Sonarqube FOLIO Java quality profile. Should be enough not fail anything. Let's see how it works out. |
| Comment by John Malconian [ 28/Sep/17 ] |
|
Heikki Levanto - Unfortunately, custom Quality Gates (where test coverage is defined) cannot be customized in the hosted version of Sonarqube (Sonarcloud) at this time. |
| Comment by Heikki Levanto [ 28/Sep/17 ] |
|
The thing with cognitive complexity seems too strict. Vertx requires a more complex code. Could we up the limit from 15 to 25? And maybe reduce it to a warning, for now. |
| Comment by shale99 [ 19/Oct/17 ] |
|
i would like the following also reduced to a warning: Extract this nested try block into a separate method. try {
vertxContext.runOnContext(v -> {
try {
PostgresClient postgresClient = PostgresClient.getInstance(vertxContext.owner(),
TenantTool.calculateTenantId(tenantId));
|
| Comment by shale99 [ 19/Oct/17 ] |
|
also, when rmb instantiates a class to use in order to call an implemented meathod, it allows the class to implement a constructor for some minor bootstraping. the constructor contains a vertx and a tenantid parameter - but the implementing class can refer to either both or just one of the arguments - so the rule Remove this unused method parameter "vertx". is problematic as well |
| Comment by Julian Ladisch [ 02/Feb/18 ] |
|
@shale99: "Extract this nested try block into a separate method." should not be reduced to a warning. This mainly pops up in org/folio/rest/impl/*API.java files. Use @SuppressWarnings("squid:S1141") // suppress "Extract this nested try block into a separate method."
public class MaterialTypeAPI implements MaterialTypesResource {
in those files so that this error can be dealt with on an individual basis in all other files. |
| Comment by Julian Ladisch [ 02/Feb/18 ] |
|
shale99 The error/warning "Remove this unused method parameter" can be disabled on a case-by-case basis: @SuppressWarnings("squid:S1172") // suppress "Remove these unused method parameters."
public FixedDueDateSchedulesAPI(Vertx vertx, String tenantId) {
if(schema == null){
initCQLValidation();
}
}
The rule should remain enabled by default in sonar so that the likely programming error is flagged in all other places. |