We faced the issue when SonarQube for PRs found no new bugs in code and was merged to master, but after this merge the nightly job on master found a new bug in code and failed by the Quality Gate.
That bug was related to code that was changed in 11/10/2020, but was raised as New Code issue in the master branch.
This is a real issue (we use the wrong way to access the static class member). But I don’t see any reason why sonar should complain about it now, this is old tech debt. We changed SikluEthtool::GetPortParameters to be static since the unit tests code was not included in the diff (it was not changed) the Sonar did not find the issue on a PR build. When the master build starts, the full source code was scanned by Sonar and the issue in the unit tests was detected.
The expected behaviour was to fail the PR job before it was merged to master or not to fail the master with new bug in code.
What is a proper solution for avoiding such issues in future?
The issue has occurred again with the updated SonarQube to Version 9.7 (build 61563). The scenario was the same, merged PR with no new bugs in code and then failed nightly build job on master.
The “New Bug” in master branch appeared in lines that were not changed, only lines number differs (See screenshots above)
We need a working solution how to avoid such behaviour for our production environment. Is it possible to perform full code scan on PR jobs? Maybe, there is a workaround how we could stay within PR scans, and continue to benefit from its features, but to perform full scan before merging to master?
The workaround would be to analyze the PR’s underlying branch. You don’t get the benefit of PR decoration, but you would get a “whole code” analysis.
as I understood from your answer, the only available solution to perform a full code review is fall back to branch analysis? Will Sonar team prepare a feature update in future versions to fix such unwanted behaviour?
We’ve talked in the past about raising more issues in PR analysis, but it hasn’t bubbled to the top of the list. I’ve pinged the Product Managers on the topic, though.
We consider addressing this need in the future and have created a portal card here where other users can share their needs. You can find the link to it here.
If we move forward with this we will update the card and if you left your email you will receive a notification.
This is definitely a bug in SonarQube. Why you are considering it as a new feature?
This bug causes instability in our CI/CD system. As a temporary solution, can you propose us a workaround or something like that?
As explained by @ganncamp, this is currently the normal behavior for PRs: PR analysis currently raises issues on changed code only.
The main workaround I can suggest is to analyze your feature branch if you want to be sure to catch these issues before the merge.
We made big improvements recently to speed up the PR analysis, analyzing only the code that has changed. Out of curiosity, would you be ready to sacrifice feedback time on your PR for stricter checks?
As explained by ganncamp, this is currently the normal behavior for PRs: PR analysis currently raises issues on changed code only.
If, for example, in PR someone changes the type of some variable that is accessible in many places in the code, it may lead to additional issues in the code that were not changed by PR.
The main workaround I can suggest is to analyze your feature branch if you want to be sure to catch these issues before the merge.
Yes, this is what we do, but it leads to additional problems. For example, if an issue is detected by Sonar during scanning the feature branch and if the SW engineer will mark this issue as a false positive, after the PR merge the same false positive issue will not be marked in the main branch and next Sonar scan of the main branch will fail.
We made big improvements recently to speed up the PR analysis, analyzing only the code that has changed. Out of curiosity, would you be ready to sacrifice feedback time on your PR for stricter checks?
For us, the time required for PR analysis is not critical at all. Our CI/CD system runs tests at the same time as Sonar analysis PR.
If you can add a flag that will force Sonar to do a full scan of the entire codebase during PR analysis (instead of analyzing only changed code), it will be a great solution for us.
We will much appreciate it if you will be able to add such a flag.
Chris, we are still waiting for the solution. When we can get it?
We raised the issue in October and the issue is still not resolved.
I replied to the thread a month ago. Till now, I did not get even reply.
We are paying for the license and we expect that Sonar will fix critical bugs in the product.
As Chris shared here, we’re still assessing the interest in the feature:
That link he shared is your opportunity to vote for it. If we decide to move forward with the functionality, we’ll update that portal card & folks who voted will be notified.
In the meantime, I’m not aware of any concrete plans for this feature.
Can you please explain why you are considering this as a new feature and not as a bug?
We run Sonar twice: the first time using PR flow and the second time using regular scan and we got different results. If the product has no bugs, I expect to get the same result. Am I missing something?
The feature is currently working as designed: report issues raised on changed lines.
I understand that you view it as a bug. We do not. It is an unfortunate corner case that we’re nonetheless considering fixing. But it’s not simple.
I understand that you are working hard in order to make the solution fast and robust.
May I ask you to do a simple solution - on PR just to let us configure Sonar to scan the entire code instead of scanning only the diff? (I understand that this will slow down the scan).
If you don’t have a configuration for that, maybe we can do some hack inside the Sonar container to achieve that?
When Sonar analyzes PR, how does Sonar knows which lines should be analyzed and which lines not? Is Sonar analyzing only changed lines or changed lines + a few lines before the change and a few lines after the change?
