SONARCH-708, SONARJAVA-5497: Extend SonarJava CheckRegistrar API for registering custom file scanner hooks

[kaufco]

SONARCH-708

[dorian-burihabwa-sonarsource]

I think the change looks simple enough to integrated as-is, but there are a couple of questions I have left for you to look at.

In addition, I thin that he ticket attached to this PR should be a SONARJAVA ticket because this change implies a modification of the public API and should be properly documented as part of the release

[dorian-burihabwa-sonarsource]

What are we testing here? Isn't the default implementation of registerCustomFileScanner empty?

[kaufco]

Yes, but QG failed due to low coverage. Even if the method is empty, it must still be tested. So just making sure the method is there and can be called without issues is the best thing we can do here I think.

[kaufco]

Summary from our discussion:

• If we introduced a return value just for testing, then what we really would be testing is only that the method does not terminate abnormally. We wouldn't be testing any behavior. I.e., if the method returns true, this does not proof anything what the method does or does not do apart from returning true.
• Therefore, a return value only for the matter of testing an empty method is redundant. Normal termination is equivalent to the method not throwing an exception, so we can test that instead. assertTrue(context.registerCustomFileScanner(null, null)) would look nicer in the test, but it does not justify modifying the API
• If we add a return value, it must have a true semantical meaning, not be introduced only for the testing. What we could do: a return value false indicates that the registration failed. The registrar could then print a log message or something.

[dorian-burihabwa-sonarsource]

To help understanding the intent of the test, it would help if the test a name in the form of an assertion (that should not be broken)

[kaufco]

The purpose of the method is just to get coverage for an empty default method (see the other comment). Naming the method registerCustomFileScanner_does_not_throw_exception would satisfy your request, but still I think it doesn't capture well the purpose of the test. I renamed it to cover_empty_default_implementations. Wdyt?

[dorian-burihabwa-sonarsource]

Nitpicking: Have we considered returning some value here? To test that something was added, we need to look into the internals of the Checkregistrar implementation. It works here but does not promote good testing practices in the custom plugins downstream.

[kaufco]

I think we should stick to the pattern used in CheckRegistrar and not be doing it different for a single method: all the register* methods have a void return type.

If we want to change that, I think it should be done in a separate ticket, because changing it for all methods would clearly go beyond the scope of this PR (and also break backwards compatibility on the bytecode level, because even though source code level compatibility does remain, the method signatures would change in the class files).

[dorian-burihabwa-sonarsource]

Can we arrange these tests in a less clever but more explicit way?

Suggested change

	if (ruleScope != RuleScope.TEST) {
	mainChecks.add(scanner);
	}
	if (ruleScope != RuleScope.MAIN) {
	testChecks.add(scanner);
	}
	if (ruleScope == RuleScope.MAIN || ruleScope == RuleScope.ALL) {
	mainChecks.add(scanner);
	}
	if (ruleScope == RuleScope.TEST || ruleScope == RuleScope.ALL) {
	testChecks.add(scanner);
	}

[kaufco]

Fixed.

[dorian-burihabwa-sonarsource]

If we change the implemetation in SonarComponents, we might as well change it here

[kaufco]

Fixed.

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[dorian-burihabwa-sonarsource]

I think what bothers me most is the need to test an empty method and the lack of guidance for what a good implementation of registerCustomFileScanner should look like.

After discussing with @kaufco, it looks like we have 2 options to improve the current proposal.

1. Improve the documentation of the new method to describe what a good implementation of registerCustomFileScanner should do and keep the test of the of the empty method (with a name that indicates that there is no expected behavior of the default implementation)
2. Enforce some sort of contract by setting a return type and/or an exception type to registerCustomFileScanner

My preference goes for option 2 because it enables us to write a test where we can implement a child class and test what the default behavior is.

@Test
void registerCustomFileScanner_default_implementation_returns_expected_value() {
  class Child extends CheckRegistrar.RegistrarContext {
    // Nothing overridden here
  }
  var context = new Child();
  assertThat(context.registerCustomFileScanner(null, null)).isEqualTo(EXPECTED_VALUE);
}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
97.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube