INFO: Undefined report path value for key 'sonar.cxx.other.xslt.1.inputs'

[guwirth]

• remove INFO
• re-factor transformFiles
• move tests to right class
• fix INFO: Undefined report path value for key 'sonar.cxx.other.xslt.1.inputs' #1623

---

This change is 

[ivangalkin]

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @guwirth and @ivangalkin)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java, line 122 at r1 (raw file):

Quoted 8 lines of code…

      String stylesheetKey = Optional.ofNullable(
        getLanguage().getPluginProperty(OTHER_XSLT_KEY + i + STYLESHEET_KEY))
        .orElse("");
      String inputKey = Optional.ofNullable(
        getLanguage().getPluginProperty(OTHER_XSLT_KEY + i + INPUT_KEY))
        .orElse("");
      String outputKey = Optional.ofNullable(
        getLanguage().getPluginProperty(OTHER_XSLT_KEY + i + OUTPUT_KEY))

the null-checks are absolutely not necessary; CxxLanguage::getPluginProperty() never returns null
it's sufficient to write...

String stylesheetKey = getLanguage().getPluginProperty(OTHER_XSLT_KEY + i + STYLESHEET_KEY);
String inputKey = getLanguage().getPluginProperty(OTHER_XSLT_KEY + i + INPUT_KEY);
String outputKey = getLanguage().getPluginProperty(OTHER_XSLT_KEY + i + OUTPUT_KEY);

... just as it was before

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java, line 131 at r1 (raw file):

if (stylesheetKey.isEmpty()) {

here and below: all checks a la if ( xxxKey.isEmpty() ) can be removed; they are always false

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java, line 140 at r1 (raw file):

paramError = true;

Since we could throw away a half of your checks, I believe the best way is to remove paramError declaration and replace all paramError = true just with breaks

So we'll have something like

final String stylesheetValue = context.config().get(stylesheetKey).orElse(null); // it is always present, we checked it above
final String stylesheet = Optional.ofNullable(resolveFilename(baseDir.getAbsolutePath(), stylesheetValue)).orElse("");
if (stylesheetPath.isEmpty()) {
   LOG.error();
   break;
}

final List<File> inputs = getReports(context.config(), baseDir, inputKey);
if (inputs.isEmpty()) {
    LOG.error("XLST: " + inputKey + " value is not defined.");
    break;
}

final List<File> outputs = Arrays.asList(context.config().getStringArray(outputKey));
if (outputs.isEmpty()) {
    LOG.error(...);
    break;
}

if (inputs.size() != outputs.size()) {
    LOG.error(...);
    break;
}

[guwirth]

@ivangalkin thx. Looking to the original code there were also this null-checks. But maybe this is only because of wrong unit tests? Will have a look... Original idea was only a small patch and not a whole refactoring...

[guwirth]

replace all paramError = true just with breaks

Has the drawback that you see only the first error

[ivangalkin]

Reviewed 1 of 3 files at r2.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @guwirth)

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java, line 131 at r1 (raw file):

Previously, ivangalkin wrote…

if (stylesheetKey.isEmpty()) {

here and below: all checks a la if ( xxxKey.isEmpty() ) can be removed; they are always false

this emptiness check (and similar checks below) are still pointless. The language specific keys are always non-null and non-empty.

See CxxLanguage.java

  public String getPluginProperty(String key) {
    return "sonar." + getPropertiesKey() + "." + key;
  }

---

cxx-sensors/src/main/java/org/sonar/cxx/sensors/other/CxxOtherSensor.java, line 140 at r1 (raw file):

replace all paramError = true just with breaks

Has the drawback that you see only the first error

I agree with that argument.

Looking to the original code there were also this null-checks. But maybe this is only because of wrong unit tests?

Yes, it's possible. We often use something like...

CxxLanguage language = Mockito.mock(CxxLanguage.class);

... but forget to stub some functions. That might be the reason why our productive code contains wrong expectations about the return values == null. This is error-prone, because it makes the code more complex and hides possible test configuration issues. I strongly believe we should get rid of them.

You've replaced the null-checks with the isEmpty()checks. But the latter ones are pointless too. Please remove them (see CxxLanguage.java for details).

---

cxx-sensors/src/test/java/org/sonar/cxx/sensors/other/CxxOtherSensorTest.java, line 56 at r2 (raw file):

·····

trailing whitespaces

---

cxx-sensors/src/test/java/org/sonar/cxx/sensors/other/CxxOtherXsltTest.java, line 55 at r2 (raw file):

    fs = TestUtils.mockFileSystem();
    language = TestUtils.mockCxxLanguage();
    when(language.getPluginProperty(CxxOtherSensor.REPORT_PATH_KEY)).thenReturn("sonar.cxx." + CxxOtherSensor.REPORT_PATH_KEY);    

here and below: trailing whitespaces

[guwirth]

getPluginProperty

fixed

stylesheet = Optional.ofNullable(
resolveFilename(baseDir.getAbsolutePath(), context.config().get(stylesheetKey).orElse(null)))
.orElse("");

resolveFilename can return null. Means stylesheet can be empty.

xxxKey.isEmpty()

Needs rework of unit tests.

[guwirth]

@ivangalkin tried to make you happy :-). Please have a look....

[ivangalkin]

Reviewed 1 of 2 files at r3.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @ivangalkin and @guwirth)

---

cxx-sensors/src/test/java/org/sonar/cxx/sensors/other/CxxOtherXsltTest.java, line 46 at r4 (raw file):

 private CxxOtherSensor sensor;

I would prefer not to share any states between the tests, since that might hide/introduce some errors. It is instantiated in each (?) test case so far, but once somebody forget it, he/she will face some old instance. I would eater suggest to move the declaration back to the test cases, or extract the instantiation to the setUp() method.

[ivangalkin]

@guwirth thank you for applying of my comments; I am happy now 👍 I've commented on the test, but this is an optional remark.

[ivangalkin]

thx 👍

[guwirth]

@ivangalkin thx for your valuable input!