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.

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.

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.

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.

Looks like all extensions are getting a 0.0% coverage rating. Since there are other jobs for checking coverage, could that report simply be suppressed, at least for extensions? (Fixing the underlying issue is also fine but feels like more work).

@André Costa (WMSE) you have to sort from the other direction, see here. The extensions with 0 % coverage either 1) were part of an initial import and have not been added to the codehealth pipeline in CI (we had capacity issues so we didn't add all extensions) or 2) they don't have unit tests, only integration tests, and we only use unit tests for calculating the coverage.

@KHarlan (WMF) Thanks, I missed that there were some extensions among the services.

I actually came to this issue from a patch which got a weird coverage report, but I guess the reason might then be that the master branch hasn't been re-analysed yet (since the initial import) meaning coverage comparisons fail in a weird way.

@KHarlan (WMF) So I've given codehealth a few days to get the chance to re-analyse the master branch of the extension. And it still seems like it isn't picking up any of the coverage. Per doc.wikimedia.org/cover-extensions it should be ~74%. Could it be that the default settings makes it look in the wrong place?

hi @André Costa (WMSE), looks like it is scenario 2 that I referenced earlier -- the tests in that extension are not extending MediaWikiUnitTestCase. (Those tests also need to be in `tests/phpunit/unit`.) See also task T230423

Ah thanks for the clarification, true we only extend MediaWikiTestCase and the tests are directly under test/phpunit (the /unit subdirectory does not seem to be documented on Manual:PHP_unit_testing/Writing_unit_tests_for_extensions and the UnitTestsList defaults to test/phpunit.

Yes, the documentation is in need of updating for sure. I added a small entry there but please feel free to expand or modify based on your experience. Thanks!

The `globalwatchlist` extension appears to be missing entirely? It has unit tests (https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/GlobalWatchlist/+/refs/heads/master/tests/phpunit/unit/) but I can't find it in the search

@DannyS712 the extension needs to be added to the codehealth pipeline; I've submitted a patch.

Ah, I didn't realize that was missing - thanks

See SonarQube's comment on Patch set 1 of https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/606518/