[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: |
|
||||||||||||||||||||||||||||||||
| 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:
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 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. 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 |
| 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 Look at the overview page of mod-inventory-storage: https://sonarcloud.io/dashboard?id=org.folio%3Amod-inventory-storage 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
|
| 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? 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. |
| Comment by shale99 [ 06/Feb/18 ] |
|
Alfons Seelen - not sure i underestimate that |
| 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 |
| 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
|