 | This page is an archive. Do not edit the contents of this page. Please direct any additional comments to the current talk page. |
The "duplicated lines density" warning triggers on this patch: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/658312
The issue seems to be that we changed a message signature, which means making the same one line change in many places. Many of these lines will look the same (parameter lists of subclasses, or invocations).
It seems like the warning should not trigger of the repeated chunks are very small (one line), or if the total number of lines is low.
See here: the method is redeclared to widen visibility from private to public.
The description on sonarcloud is, as usually, very interesting: > "Overriding a method just to call the same method from the super class without performing any other actions is useless and misleading. The only time this is justified is in final overriding methods, where the effect is to lock in the parent class behavior."
Welp, sounds like they missed another compliant case.
There are also cases in which a subclass wants to provide additional guarantees for the behavior of a method, e.g. a getter may return null in the base class, but is guaranteed to not return null in a subclass. We need to override the method in order to declare the contract in a doc block.
I would just disable this rule, it seems "useless and misleading".
In task T256942, there is a request to deactivate the "Functions should not contain too many return statements" rule. I've deactivated it (for PHP analysis) while we discuss whether it's useful to re-enable.
Personally, I sometimes find it helpful as a reminder that a method is doing too many things; other times I ignore it as it doesn't make sense. I don't have an opinion on whether we should keep it enabled or disabled for the MediaWiki PHP profile.
Its a personal coding style choice and often more returns can make something more readable, not less; eg for any possible error that is found, return early, rather than wrapping the rest of the code in a giant `if` block to ensure there isn't an error found. Fully support deactivating the inline comments from the bot, though if it wants to take the returns into account and note them in its general comment for the patch set that would be not as annoying imo
> Fully support deactivating the inline comments from the bot, though if it wants to take the returns into account and note them in its general comment for the patch set that would be not as annoying imo
Yes, that's an option. When we were on Gerrit 2.15, I started prototyping that because robot comments were barely supported in Gerrit 2.15. Because you can't use HTML in your comments, you'd end up with the current general soanrqube bot comment followed by something like:
I guess this isn't so bad; the overall goal is for a human to at least scan the comment to see if it's something worth addressing which the previous implementation didn't do (I imagine a lot of people just ignored the "SonarQube build failed" message). Do others have feedback on this?
See here. Simplified example:
foreach ( $foo as $bar ) {
if ( foo1() ) {
doSomething1();
} elseif ( foo2() ) {
continue; // <-- faulty line
} else {
doSomething2();
}
}
The continue line is reported by SonarQube as being non-compliant with message "Remove this redundant jump.". On sonarcloud.io there's an interesting description for this issue: "jump statements that direct the control flow to the original direction are just a waste of keystrokes."
I have a very different idea of "waste of keystrokes". Jump statements make code like that more resilient and reliable. Without the continue, some day I might add some instructions after the conditional (but inside the loop) without any tool telling me of my mistake. So no, this is not a waste of keystrokes.
As far as I can tell, the rule in question is inactive for PHP but active for JavaScript and TypeScript. If you happen to have a recent patch, it would be helpful in optimizing the scan feedback in SonarCloud, since the provided patch sample was pruned from SonarCloud long ago. I have gone ahead and disabled the rule for JavaScript and TypeScript. In case it's still an issue, please feel free to reopen the topic.
MGrosse-WMF has been added to the developers group with the +2 role in SonarCloud. With this access, they should be able to mark the raised issue as intentional.
In https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/1075600/1 I get a code smell alert telling me to 'Use a standard "import" statement instead of "require".'
However, Resource Loader cannot deal with "import" and so we have to use require. Probably, this rule should just be disabled unless we can find a way to tell code handled by ResourceLoader directly and compiled or node-js or testing code apart.
(There is also that issue that this is about code touched in the previous, until now still unmerged, change. But Sonar is probably built with a branch-based instead of commit-based workflow in mind, so I'm not sure if this can be fixed.)
Made some configuration changes on "new code" definition to prevent alerts on changes unrelated to the patch.
In the comment by the bot on PatchSet 1 of this change: 1070555: Community updates: add missing instrumentation link id, it shows the quality gate as failed but all the items in the list below show still "0". When one clicks through to the analysis, this seems to be about problems in the linked file that existed prior to this change and on lines not touched by this change.
I'm not sure what the desired behavior here would be, but the status quo is not ideal. --~~~~
@MGrosse-WMF Thank you for pointing this out. I have realized the new rules flagged issues unrelated to the change. I'm investigating to ensure the analysis focuses on new code, not legacy issues. I'll adjust the configuration to better align with our feedback process and prevent this undesired behavior.
The current configuration treats any code changed in the last 30 days as new. I've updated this to consider only code changed since the previous version as new, so going forward, only code related to the specific patch will be analyzed.
The bot is recommending a for...of loop: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/753803/comment/48afe633_9133e4a3/
Yeah I don't think SonarQube can grok projects with both ES5 and ES6. @JBranaa (WMF) I think we should disable this particular rule.
I've never noticed this before, but now it seems to be suggesting var -> let / const, too. All those comments are very annoying because they take up a lot of space, see e.g. here. Could we at least make it validate the code against ES5 by default?
Ping @JBranaa (WMF) as QTE are maintainers of this tool. Maybe a phab task is helpful for visibility and triaging?
@Daimona Eaytoy @ESanders (WMF) I extended the default Sonar JavaScript profile, made that the default, and have disabled those two rules https://sonarcloud.io/organizations/wmftest/rules?qprofile=AYFmyGoW0S9tUy1aTrH7&activation=true
@KHarlan (WMF): For some reason, it got worst... Now it seems to be complaining about pretty much everything, including wild things like "25 more comment lines need to be written to reach the minimum threshold of 25.0% comment density", unwanted stuff ("Replace all tab characters in this file by sequences of white-spaces.") and puzzling suggestions ("Unexpected string concatenation."). See here for an example.
@Daimona Eaytoy, ugh, sorry. I am not sure how it happened, but "Extending" the default profile somehow... added a bunch of rules? It went from 209 to 275 rules. I reverted back to the "Sonar way" profile, which means the "var let" business is back. :( I (or QTE who maintain this tool) will have to try another time.
Sorry for the delay on this. Please submit a phab task for for this and tag it quality-and-test-engineering and we'll take a look at it.
I have encountered this as well and filed https://phabricator.wikimedia.org/T311530.
I have confirmed the "Unexpected var, use let or const instead." rule has been disabled for both Javascript and Typescript. Incase anyone encounters more ES6 syntax issues kindly let me know.
On https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/929398 it reports coverage is 0 but Popups has both JavaScript and PHPUnit coverage. Is this misconfigured. Perhaps it's not aware of the mechanism for which coverage is collected?
This message links to https://sonarcloud.io/summary/new_code?id=mediawiki-extensions-Popups&branch=929398-2 which says 0% coverage on 9 new lines to cover. Following that link, I get to https://sonarcloud.io/component_measures?id=mediawiki-extensions-Popups&branch=929398-2&metric=new_coverage&view=list, which shows lines in files that don't seem to be covered by tests in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/929398
SonarCloud does seem to have knowledge of code coverage for Popups (https://sonarcloud.io/component_measures?metric=coverage&view=list&id=mediawiki-extensions-Popups), although seemingly only for PHP (https://sonarcloud.io/component_measures?id=mediawiki-extensions-Popups&metric=coverage). For JS coverage, make sure that your `npm run coverage` command is generating a file in `coverage/lcov.info`.
Hopefully this does it! https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/930660
There was no coverage/lcov.info file.
If the bot sees a TODO or FIXME comment, it will leave a comment saying "Take the required action to fix the issue indicated by this comment". I'm asking that this be disabled. Developers tend to know that TODOs and FIXMEs should be fixed eventually, but it's not always possible or convenient to do that immediately, and a noisy bot won't change that. Whenever I see some space for improvement, I like to mark it with a TODO or FIXME, even if it's not actionable, because it's easier to find (via grep, and IDEs can even highlight such comments), and consequently, it's perhaps more likely that someone will work on it at some point. Discouraging TODOs is not good, because if you don't want to annoy the bot you may either remove the comment, or not format it as a TODO, both of which are bad options.
@Daimona Eaytoy I have made some changes to avoid alerts on "FIXME" occurrences. I'm keeping an eye out; let me know if you receive any additional alerts.
Yeah, I ignore all these for TODOs; generally FIXMEs are for issues that should be fixed before the patch is landed, though that varies by team. In our team we encourage making each inline action comment into a Phabricator task and mentioning it (so TODO: Do x becomes TODO (T123456): Do x), but that's rather specialist and maybe something we could in the longer run add to MW codesniffer, not here.
A file was excluded from analysis.
In https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1003568/1#message-7b4d72a20d278f68aba32d7a855647cfb726bdf4, SonarQube Bot complains about duplicated lines in machine-generated code (includes/config-schema.php).
I think the only way around that is using a sonar-project.properties file https://docs.sonarsource.com/sonarcloud/advanced-setup/ci-based-analysis/sonarscanner-cli/#use, to tell Sonar to ignore particular files.
Sonar recently made it possible to exclude source files through project settings in the portal. I have excluded the specific file from analysis; hopefully that is enough to solve the issue.