[FOLIO-1049] Reject pull request if new code doesn't pass sonar's quality gate (backend modules) Created: 02/Feb/18  Updated: 18/Jan/19

Status: Open
Project: FOLIO
Components: None
Affects versions: None
Fix versions: None

Type: New Feature Priority: P3
Reporter: Julian Ladisch Assignee: Jakub Skoczen
Resolution: Unresolved Votes: 0
Labels: ci, for-next-sprint
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original estimate: Not Specified

Issue links:
Blocks
is blocked by RMB-130 Use target/generated-sources for gene... Closed
is blocked by RMB-131 PostgresClient: asynchronous function... Closed
Relates
relates to FOLIO-858 Increase Sonarqube ratings to 'A' for... Open
relates to FOLIO-844 integrate SonarQube test coverage int... Closed
relates to FOLIO-864 Managing Sonarqube exceptions and rul... In Progress
relates to FOLIO-1076 Make SQ ignore generated sources Closed
Sprint:
Development Team: Core: Platform

 Description   

For each backend module (mod-*, okapi, raml-module-builder, cql2pgjson-java) add a requirement in Jenkins that a pull request is rejected until the new code of the pull request passes sonar's quality gate:
https://sonarcloud.io/organizations/folio-org/quality_gates/show/9

  • Test Coverage on New Code >= 80%
  • Duplicated Lines on New Code <= 3%
  • Maintainability Rating on New Code is A
  • Reliability Rating on New Code is A
  • Security Rating on New Code is A

Jakub suggested this in his talk "How we work" Montreal. This will incrementally increase the code quality and testability: https://sonarcloud.io/organizations/folio-org/projects?sort=name

This applys to the new code of the pull request only; old existing code that is not changed can remain with issues that would fail in the quality gate.

One can suppress false positives using

@SuppressWarnings

like in https://github.com/folio-org/cql2pgjson-java/blob/5b4534b82087701a82429bbb7e94ffd02f271e5f/src/main/java/org/z3950/zing/cql/cql2pgjson/Schema.java#L200

A code coverage of 80 % is easy to achieve. 13 out of 17 modules already have a code coverage of more than 79.5% in the new code of the last 30 days: https://sonarcloud.io/organizations/folio-org/projects?sort=new_coverage&view=leak Sometimes code changes are needed for testing. These changes usually make the code easier to understand and to maintain. Many developers are happy to help with test coverage, ask on #developer slack channel.



 Comments   
Comment by Jakub Skoczen [ 05/Feb/18 ]

I am OK doing this, what do others think?

Comment by Marc Johnson [ 05/Feb/18 ]

Julian Ladisch Have you tried the pull request functionality?

My understanding of the typical windowing uses time or version, is there a special mode that uses the difference between the branch and master (Wayne Schneider and John Malconian you might have experience of this)?

Comment by Julian Ladisch [ 05/Feb/18 ]

Marc Johnson: Sonar's quality gate can be applied on different code:

When integrated into Jenkins and GitHub's pull request requirements (as proposed by this Jira) a pull request is rejected only if new or changed code of that pull request doesn't pass the quality gate.

Comment by Alfons Seelen [ 05/Feb/18 ]

Short answer: please do! Another option would be to set that no one can merge their own PRs into master without having it approved from another team member. Personally I would prefer to go further and create tickets for cleaning/refactoring/testing existing code and have it clean by let's say v2/gamma.

Long answer: I believe this would be very helpful to the project and that SonarLint should be a little bit more than a friendly reminder. The time you spend on testing whether your code does what it should (you might want to start writing the test) and on readability for others on the project and even for yourself in the future is IMHO nothing compared to the cost and time you save for the future. It leads automatically to less bugs, other bugs can be tracked more easily, implementing new functionality and refactoring will be possible, and personally I think it is much more fun to work in a clean environment. BTW, it makes no sense keeping your own code clean if the rest isn't really. It gets worse over time. Think of it as toilet. Especially since I guess this is a long term project that has to be maintained for quite a time, code quality is probably the most important issue on the development side. The earlier we start with it, the better in the end. Side-effects are that POs and stakeholders have to wait a bit longer for their functionality to be implemented, that tickets take more time.

Comment by Marc Johnson [ 05/Feb/18 ]

Julian Ladisch Do you know if the current pull request integration produces an overall report that is kept somewhere? I've only seen it report individual code smells as comments in the pull requests, rather than the additional information referred to above?

Comment by Julian Ladisch [ 05/Feb/18 ]

If sonar's quality gate rejects a pull request then there will be a "Details" link after "X sonarqube – SonarQube reported 7 issues, with 3 critical" in github's checks box: https://docs.sonarqube.org/display/PLUG/GitHub+Plugin#GitHubPlugin-Description

Comment by John Malconian [ 05/Feb/18 ]

The Sonarqube PR plugin does not write information back to the Sonarqube database. It is possible, however, to produce specific reports for branches (branch reports are maintained separately for each branch). If you'd like Sonarqube to produce a report for a non-master branch use the 'sqBranch' in the Jenkinsfile. Example:

buildMvn {
   sqBranch  = ['folio-1049-test', 'some-other-branch']
   ..
}
Comment by Marc Johnson [ 05/Feb/18 ]

Thanks Julian Ladisch and John Malconian that is useful to know. I may well give that a try (I haven't seen a coverage report for a non-master branch).

Comment by Marc Johnson [ 05/Feb/18 ]

I think we can give it a try. Given we seem to be intending to focus some of our attention (from the Madrid and after conversations about our CI processes) on raising quality and reducing feedback cycles for issues, this may encourage us to pay more attention to the SonarQube information.

From the local builds that I run against SonarQube, I believe quite a few recent changes would likely fail these metrics, so we need to be conscious that, at least in the short term, we may see some changes take more time.

The experiments I did, before Christmas, around logging vulnerabilities and improving coverage, suggested to me that some areas are difficult to cover with our current tests (exceptions in asynchronous callbacks and some kinds of failure scenarios), so changes to these areas may need significant investment to reach these levels (this does generally increase the overall coverage substantially in some cases).

Comment by shale99 [ 06/Feb/18 ]

personally, i think this slows down development dramatically, there is a fine line between writing good , tested good and having an "A" in everything sonar qube reports.
Marc Johnson - there is 4 days of work (estimated by sonar) to get rid of the major issues in mod-inventory-storage - which is quite readable, well tested and stable, does it make any sense to blindly burn 4 days on those major issues ? of course not, unless we really have nothing else to do.

this is the reality of software design , spend days doing something perfectly and not release anything , or do things to a point where they are stable and tested and keep moving forward. this is the classic 'lets not release' until everything is perfect bug that kills project progress.

and this is before trying to understand sonarqubes stats - the test-loader tests all rules / loading etc... yet has a 0.1% coverage rate? should i stop working and start investigating?

there is no perfect world here, you need a balance and there may not be a one size fits all here.

i say , yes to some sort of threshold
i say , no to what was described here

Comment by Julian Ladisch [ 06/Feb/18 ]

When test-data-loader unit test hangs and therefore the code coverage report is at 0 % or 0.1 % then you should stop working and start investigating. I've opened an issue for that: https://folio-org.atlassian.net/browse/FOLIO-1055

Comment by Julian Ladisch [ 06/Feb/18 ]

The major issues of mod-inventory-storage are a good example: https://sonarcloud.io/project/issues?facetMode=effort&id=org.folio%3Amod-inventory-storage&resolved=false&severities=MAJOR
If you click on "File" in the left menu of above page then you see that it is only 2 or 1 hour of work per file. Usually you only change one file at a time so you can invest work that is as small as 1 or 2 hours to fix the existing issues. This is affordable.

Look at the overview page of mod-inventory-storage: https://sonarcloud.io/dashboard?id=org.folio%3Amod-inventory-storage
The light brown column with the leak period (last 30 days) shows that an additional effort of only 18 minutes were required to fix the issues of the last 30 days. This is affordable.

Most of the issues (more than 80 %) that sonar reports about mod-inventory-storage are within generated files. Fixing or suppressing them takes less time than the estimated time because this can be done once in the code generator.

Comment by Heikki Levanto [ 06/Feb/18 ]

I am strongly against this. Requiring 80% test coverage means it will be impossible to work on the small modules like mod-codex-mock, mod-notify, and mod-notes. Mod-codex-mock has about 42%, the rest somewhere in their 60's. And that is the most I can do! Most of the code is generated by RMB, and I have no way of testing that. Of the source files I have written myself, the coverage is a bit higher, 60-70%, and can not made much higher without quite much work. The main problem is that the RMB routines declare they can throw exceptions, which I have to handle, but I have no way to provoke those, so I can not test that code.

So, unless and until we change something in SQ config to ignore RMB's autogenerated code, and redesign the error handling in RMB's database routines, demanding 80% test coverage will mean an immediate and permanent stop on all development of my small modules.

Comment by Julian Ladisch [ 06/Feb/18 ]

RMB has recently been changed to allow for mocking so that you now can provoke exceptions: https://github.com/folio-org/raml-module-builder/pull/115

Comment by Adam Dickmeiss [ 06/Feb/18 ]

I think we can go pretty far if we can make SQ ignore coverage, unused args, etc.. for generated code.

Example: For mod-codex-mux I get 67% now . My own code has 84% coverage.

Comment by Marc Johnson [ 06/Feb/18 ]

Adam Dickmeiss I've just tried this out locally, on mod-circulation-storage, and applying an exclusion for coverage, duplications and analysis of src/main/java/org/folio/rest/jaxrs/*/ and this seems to exclude (most of) the generated code.

Coverage did go up (from 60 to 67%) and issues went down. We might find there are also some surprising consequences to this, for example, duplication percentage went up (due to less overall code).

I do wonder if we may miss gaps in behaviour that we are leveraging by excluding the generated code?

Comment by Julian Ladisch [ 06/Feb/18 ]

The module of the code generator must thoroughly unit test the generated code. Then users of the generator do not need to re-test it. See RMB-130 Closed .

Comment by Alfons Seelen [ 06/Feb/18 ]

I think, the question should not be whether we want to implement this quality gate, but how forgiving we set the gate. Perhaps it might be too tight at some points, as Heikki Levanto and Adam Dickmeiss pointed out. But using SonarLint/Qube with the option to developers to just ignore the warnings until you don't see them anymore because there are too many, makes no sense.

Another perspective: I think this decision is not (only) up to developers. They can advise, but essentially the stakeholders should decide whether they want their product quick and dirty (unmaintainable, many bugs, but fast releases (at least at the beginning*)) or slow and clean (very maintainable, fewer bugs, but slow releases (at the beginning*)).

*) I really think that the speed you gain at the beginning of the project by doing it quick and dirty slows down the longer the project lives, until minor changes can take a lot of time because they keep breaking other code.

Comment by shale99 [ 06/Feb/18 ]

Alfons Seelen - i dont necessarily think it needs to be black or white - there is alot of gray here. you can see quite alot of unit tests in the system today. yes, some modules have more then others, each module serving a different purpose of course. but the project as is , from a coverage standpoint is not that bad. unless i am wrong, i also dont feel the back end module's stability is that bad either - so maybe the current process isnt that bad?
The question is more of , looking at the current modules states - if we spend days stopping commits and focusing on sonarqube , and delaying a sprint, how much stability have we added to the system at this cost? from a stakeholders point of view, that is what i think really matters. i dont think that sonar qube is magic that will say, ok, right now your system is buggy and unstable, spend a week on sonar qube and now its not.. right?

having said that, i will re-iterate what i mentioned before - there is room for this - deciding to block pull requests until a module fulfills these thresholds is time consuming and i would like to think there are more gradual methods

Comment by Alfons Seelen [ 06/Feb/18 ]

shale99: The grey thinking was exactly the purpose of shifting the discussion to how we set the thresholds for SonarQube. Let the question not be 'yes' or 'no', but more 'how far', from 0% (as it is now and which makes absolutely no sense) to 100%. If you don't want your code to be inspected by an automated review, I think it should be reviewed by another developer before you can merge it. Until now, we do none of this and personally I think the general code quality should be higher to be able to maintain it on the long run, that we should meet a certain standard. What exactly that standard will be should be our focus in the discussion, in which I would certainly invite stakeholders or at least product owners.
BTW, we don't need to spend days stopping commits. We just start writing higher quality code and no commits will be stopped. At least, my IDE supports SonarLint and checks before I commit anything. And theoretically, sprints can't be delayed. You probably complete less tickets with a certain time investment, but on the long run you'll get less tickets of the 'bug' type. And I don't say, our system is buggy and unstable, but I believe you really underestimate the importance of maintainability. Code gets a mess after a certain time, you can't properly work with it and that will really slow you down, aside from this mess making your job extremely annoying.

Comment by shale99 [ 06/Feb/18 ]

Alfons Seelen - not sure i underestimate that - how would this not stop pull requests for existing modules that currently dont meet that threshold? are you suggesting this only runs on new code?

Comment by Alfons Seelen [ 06/Feb/18 ]

Perhaps I am warned too much by a horror scenario in the past, in my mission to keep projects clean, so I might overestimate myself . But no, I would like it to run on existing code as well. This takes time, of course, but shouldn't be too much. If certain parts or modules have become too dirty already I would suggest to rewrite them, at least not maintain them. That might be a pity, but the consequence of pushing forward important things as quality checks. The longer we wait, the worse it will get.

Comment by Julian Ladisch [ 07/Feb/18 ]

Heikki Levanto: Thank you for mentioning that RMB's PostgresClient should not throw Exceptions on asynchronous functions. That should be fixed. Bug RMB-131 Closed

Generated at Thu Feb 08 23:10:27 UTC 2024 using Jira 1001.0.0-SNAPSHOT#100246-sha1:7a5c50119eb0633d306e14180817ddef5e80c75d.