Talk:Continuous integration/Codehealth Pipeline

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.

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.

Hi,

I make many patches that add support for new languages. Today I saw a "duplicated lines" notification for the first time at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/749150

Messages*.php files are repetitive by their nature. They should probably be changed to something more structured, that uses less raw repetitive code, but until then, can they be excluded from this check?

Not practically. Just ignore them. The CodeHealth pipeline is advisory for a reason.

I get this in TimedMediaHandler's WebVideoTranscodes.php which has an enormous array of data definitions. :D To be honest it wouln't hurt to redefine those in a more sensible way, it's just mildly annoying to make it backwards-compatible with static class constants :D

(For the time being I'm ignoring it but plan to refactor these when time permits.)

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.

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.

@PWangai-WMF ^

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.

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:

• SomeFile.php, line 53: Unused variable. https://gerrit.wikimedia.org/r/c/mediawiki/core/+/608993/4/includes/libs/http/SetCookieCompat.php#53

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.

Yes, indeed.

The patch on https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/710123 claims that 0% code coverage of new lines is added, but this doesn't seem aware of the unit testing framework that Popups uses. It even suggests writing JavaScript unit tests for JavaScript unit tests!

Is it possible to add some kind of local sonar configuration to wire this up correctly or to disable it?

This caused some unnecessary and avoidable confusion during code review.

@Jdlrobson sorry for the confusion. The best solution would be to update https://gerrit.wikimedia.org/r/plugins/gitiles/integration/config/+/refs/heads/master/jjb/mediawiki.yaml#794 with a npm test command that we can agree will be used in all repositories using node-qunit (not many, AFAIK): WikibaseMediaInfo, QuickSurveys (?), Popups, and MobileFrontend. `npm run coverage` sounds fine to me. And that command needs to output an `lcov.info` file in `coverage/lcov.info` for SonarQube to pick it up during analysis. How does that sound?

Jest provides coverage out of the box if configured correctly.

https://codesearch.wmcloud.org/deployed/?q=(coverage%7Cjest)&i=nope&files=package.json%24&excludeFiles=node_modules%2F.*&repos=

`coverage` does seem to be the defacto standard. Could we perhaps test that first and fall back to test:unit to avoid disruption?

I don't know much about yaml and --if--present argument.

Looking at above:

test-unit-coverage 1

test:unit 5

coverage 19

test:coverage 1

Note, our code coverage is generated by "npm run coverage", and "npm test" not "npm run test:unit" as coverage reports slow down the execution of unit tests.

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".

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.