Merge pull request #324 from uhafner/build-total-of-subscores

Fix total computation of scores

src/main/java/edu/hm/hafner/grading/AnalysisMarkdown.java

@@ -53,12 +53,12 @@ protected void createSpecificDetails(final AggregatedScore aggregation, final Li
 
             if (score.getSubScores().size() > 1) {
                 details.addText(formatBoldColumns("Total",
-                                sum(aggregation, AnalysisScore::getErrorSize),
-                                sum(aggregation, AnalysisScore::getHighSeveritySize),
-                                sum(aggregation, AnalysisScore::getNormalSeveritySize),
-                                sum(aggregation, AnalysisScore::getLowSeveritySize),
-                                sum(aggregation, AnalysisScore::getTotalSize)))
-                        .addTextIf(formatBoldColumns(sum(aggregation, AnalysisScore::getImpact)), score.hasMaxScore())
+                                sum(score, AnalysisScore::getErrorSize),
+                                sum(score, AnalysisScore::getHighSeveritySize),
+                                sum(score, AnalysisScore::getNormalSeveritySize),
+                                sum(score, AnalysisScore::getLowSeveritySize),
+                                sum(score, AnalysisScore::getTotalSize)))
+                        .addTextIf(formatBoldColumns(sum(score, AnalysisScore::getImpact)), score.hasMaxScore())
                         .addNewline();
             }
 
@@ -76,7 +76,7 @@ protected void createSpecificDetails(final AggregatedScore aggregation, final Li
         }
     }
 
-    private int sum(final AggregatedScore score, final Function<AnalysisScore, Integer> property) {
-        return score.getAnalysisScores().stream().map(property).reduce(Integer::sum).orElse(0);
+    private int sum(final AnalysisScore score, final Function<AnalysisScore, Integer> property) {
+        return score.getSubScores().stream().map(property).reduce(Integer::sum).orElse(0);
     }
 }

src/main/java/edu/hm/hafner/grading/CoverageScore.java

@@ -5,7 +5,6 @@
 import java.util.List;
 import java.util.Objects;
 
-import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
@@ -87,7 +86,7 @@ public String getMetricTagName() {
 
     @JsonIgnore
     public Node getReport() {
-        return ObjectUtils.defaultIfNull(report, new ModuleNode("empty"));
+        return report;
     }
 
     @Override
@@ -256,7 +255,9 @@ public CoverageScore build() {
                     "You must either specify a coverage report or provide a list of sub-scores.");
 
             if (scores.isEmpty() && report != null && metric != null) {
-                return new CoverageScore(getId(), getName(), getConfiguration(), report, metric);
+                return new CoverageScore(getId(), getName(), getConfiguration(),
+                        Objects.requireNonNull(report),
+                        Objects.requireNonNull(metric));
             }
             else {
                 return new CoverageScore(getId(), getName(), getConfiguration(), scores);

src/main/java/edu/hm/hafner/grading/JacksonFacade.java

@@ -9,6 +9,8 @@
 import com.fasterxml.jackson.databind.json.JsonMapper;
 import com.fasterxml.jackson.databind.node.ObjectNode;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * Facade for Jackson that does wrap an exception into a {@link RuntimeException}.
  *
@@ -26,6 +28,7 @@ static JacksonFacade get() {
     /**
      * Creates a new instance of {@link JacksonFacade}.
      */
+    @SuppressFBWarnings(value = "BC_UNCONFIRMED_CAST_OF_RETURN_VALUE", justification = "false positive")
     JacksonFacade() {
         mapper = JsonMapper.builder().configure(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS, true).build()
                 .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)

src/main/java/edu/hm/hafner/grading/ScoreMarkdown.java

@@ -30,6 +30,7 @@ abstract class ScoreMarkdown<S extends Score<S, C>, C extends Configuration> {
     static final int MESSAGE_INITIAL_CAPACITY = 1024;
     private static final int MAX_SIZE = 10_000; // limit the size of the output to this number of characters
     private static final String TRUNCATION_TEXT = "\n\nToo many test failures. Grading output truncated.";
+    private static final int HUNDRED_PERCENT = 100;
 
     private final String type;
     private final String icon;
@@ -50,7 +51,7 @@ abstract class ScoreMarkdown<S extends Score<S, C>, C extends Configuration> {
      * @return Markdown text
      */
     public static String getPercentageImage(final String title, final int percentage) {
-        if (percentage < 0 || percentage > 100) {
+        if (percentage < 0 || percentage > HUNDRED_PERCENT) {
             throw new IllegalArgumentException("Percentage must be between 0 and 100: " + percentage);
         }
         return String.format("""
@@ -150,8 +151,8 @@ static String createScoreTitleSuffix(final int maxScore, final int value, final
         if (maxScore == 0) {
             return StringUtils.EMPTY;
         }
-        if (maxScore == 100) {
-            return String.format(" - %d of %d", value, maxScore);
+        if (maxScore == HUNDRED_PERCENT) {
+            return String.format(" - %d of %d", value, maxScore); // no need to show percentage for a score of 100
         }
         return String.format(" - %d of %d (%d%%)", value, maxScore, percentage);
     }

src/main/java/edu/hm/hafner/grading/TestMarkdown.java

@@ -56,11 +56,11 @@ protected void createSpecificDetails(final AggregatedScore aggregation,
 
             if (score.getSubScores().size() > 1) {
                 details.addText(formatBoldColumns("Total",
-                                sum(aggregation, TestScore::getPassedSize),
-                                sum(aggregation, TestScore::getSkippedSize),
-                                sum(aggregation, TestScore::getFailedSize),
-                                sum(aggregation, TestScore::getTotalSize)))
-                        .addTextIf(formatBoldColumns(sum(aggregation, TestScore::getImpact)), score.hasMaxScore())
+                                sum(score, TestScore::getPassedSize),
+                                sum(score, TestScore::getSkippedSize),
+                                sum(score, TestScore::getFailedSize),
+                                sum(score, TestScore::getTotalSize)))
+                        .addTextIf(formatBoldColumns(sum(score, TestScore::getImpact)), score.hasMaxScore())
                         .addNewline();
             }
 
@@ -123,7 +123,7 @@ private String getMessage(final TestCase issue) {
         return issue.getMessage() + LINE_BREAK;
     }
 
-    private int sum(final AggregatedScore score, final Function<TestScore, Integer> property) {
-        return score.getTestScores().stream().map(property).reduce(Integer::sum).orElse(0);
+    private int sum(final TestScore score, final Function<TestScore, Integer> property) {
+        return score.getSubScores().stream().map(property).reduce(Integer::sum).orElse(0);
     }
 }

src/main/java/edu/hm/hafner/grading/TestScore.java

@@ -7,7 +7,6 @@
 import java.util.function.Function;
 import java.util.stream.Collectors;
 
-import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
@@ -36,8 +35,7 @@ public final class TestScore extends Score<TestScore, TestConfiguration> {
     private final int passedSize;
     private final int failedSize;
     private final int skippedSize;
-    @CheckForNull
-    private final transient Node report;
+    private transient Node report; // do not persist the tree of nodes
 
     private TestScore(final String id, final String name, final TestConfiguration configuration,
             final List<TestScore> scores) {
@@ -61,6 +59,18 @@ private TestScore(final String id, final String name, final TestConfiguration co
         skippedSize = sum(report, TestResult.SKIPPED);
     }
 
+    /**
+     * Restore an empty report after de-serialization.
+     *
+     * @return this
+     */
+    @Serial @CanIgnoreReturnValue
+    private Object readResolve() {
+        report = new ModuleNode("empty");
+
+        return this;
+    }
+
     private int aggregate(final List<TestScore> scores, final Function<TestScore, Integer> property) {
         return scores.stream().reduce(0, (sum, score) -> sum + property.apply(score), Integer::sum);
     }
@@ -75,7 +85,7 @@ private int sum(final Node testReport, final TestResult testResult) {
 
     @JsonIgnore
     public Node getReport() {
-        return ObjectUtils.defaultIfNull(report, new ModuleNode("empty"));
+        return report;
     }
 
     @Override

src/test/java/edu/hm/hafner/grading/AnalysisMarkdownTest.java

@@ -202,15 +202,19 @@ void shouldShowScoreWithTwoResults() {
         var analysisMarkdown = new AnalysisMarkdown();
 
         assertThat(analysisMarkdown.createDetails(score))
-                .contains("Style - 30 of 100",
-                        "|CheckStyle|1|2|3|4|10|30",
+                .contains("Style - 60 of 100",
+                        "|CheckStyle 1|1|2|3|4|10|30",
+                        "|CheckStyle 2|1|2|3|4|10|30",
+                        "|**Total**|**2**|**4**|**6**|**8**|**20**|**60**",
                         "Bugs - 0 of 100",
-                        "|SpotBugs|4|3|2|1|10|-120",
+                        "|SpotBugs 1|4|3|2|1|10|-120",
+                        "|SpotBugs 2|4|3|2|1|10|-120",
+                        "|**Total**|**8**|**6**|**4**|**2**|**20**|**-240**",
                         ":moneybag:|*1*|*2*|*3*|*4*|:heavy_minus_sign:|:heavy_minus_sign:",
                         ":moneybag:|*-11*|*-12*|*-13*|*-14*|:heavy_minus_sign:|:heavy_minus_sign:");
         assertThat(analysisMarkdown.createSummary(score))
-                .contains("- :warning: Style - 30 of 100: 10 warnings found (1 error, 2 high, 3 normal, 4 low)",
-                        "- :warning: Bugs - 0 of 100: 10 warnings found (4 errors, 3 high, 2 normal, 1 low)")
+                .contains("- :warning: Style - 60 of 100: 20 warnings found (2 errors, 4 high, 6 normal, 8 low)",
+                        "- :warning: Bugs - 0 of 100: 20 warnings found (8 errors, 6 high, 4 normal, 2 low)")
                 .doesNotContain("Total");
     }
 
@@ -223,7 +227,12 @@ static AggregatedScore createScoreForTwoResults() {
                       "tools": [
                         {
                           "id": "checkstyle",
-                          "name": "CheckStyle",
+                          "name": "CheckStyle 1",
+                          "pattern": "target/checkstyle.xml"
+                        },
+                        {
+                          "id": "checkstyle",
+                          "name": "CheckStyle 2",
                           "pattern": "target/checkstyle.xml"
                         }
                       ],
@@ -238,7 +247,12 @@ static AggregatedScore createScoreForTwoResults() {
                       "tools": [
                         {
                           "id": "spotbugs",
-                          "name": "SpotBugs",
+                          "name": "SpotBugs 1",
+                          "pattern": "target/spotbugsXml.xml"
+                        },
+                        {
+                          "id": "spotbugs",
+                          "name": "SpotBugs 2",
                           "pattern": "target/spotbugsXml.xml"
                         }
                       ],

src/test/java/edu/hm/hafner/grading/GradingReportTest.java

@@ -153,15 +153,17 @@ void shouldCreateAnalysisResults() {
 
         var score = AnalysisMarkdownTest.createScoreForTwoResults();
         assertThat(results.getTextSummary(score)).isEqualTo(
-                "Autograding score - 30 of 200 (15%)");
+                "Autograding score - 60 of 200 (30%)");
         assertThat(results.getMarkdownDetails(score)).contains(
-                "Autograding score - 30 of 200 (15%)",
+                "Autograding score - 60 of 200 (30%)",
                 "Unit Tests Score: not enabled",
                 "Code Coverage Score: not enabled",
                 "Mutation Coverage Score: not enabled",
-                "|CheckStyle|1|2|3|4|10|30",
-                "Style - 30 of 100",
-                "|SpotBugs|4|3|2|1|10|-120",
+                "|CheckStyle 1|1|2|3|4|10|30",
+                "|CheckStyle 2|1|2|3|4|10|30",
+                "Style - 60 of 100",
+                "|SpotBugs 1|4|3|2|1|10|-120",
+                "|SpotBugs 2|4|3|2|1|10|-120",
                 "Bugs - 0 of 100");
     }
 

src/test/java/edu/hm/hafner/grading/TestMarkdownTest.java

@@ -181,9 +181,15 @@ void shouldShowNoImpactsWithTwoSubResults() {
 
     static Node createTwoReports(final ToolConfiguration tool) {
         if (tool.getId().equals("itest")) {
+            if (tool.getName().contains("2")) {
+                return TestScoreTest.createTestReport(5, 3, 4, "2nd-");
+            }
             return TestScoreTest.createTestReport(5, 3, 4);
         }
         else if (tool.getId().equals("mtest")) {
+            if (tool.getName().contains("2")) {
+                return TestScoreTest.createTestReport(0, 0, 10, "2nd-");
+            }
             return TestScoreTest.createTestReport(0, 0, 10);
         }
         throw new IllegalArgumentException("Unexpected tool ID: " + tool.getId());
@@ -199,7 +205,12 @@ void shouldShowScoreWithTwoResults() {
                     "tools": [
                       {
                         "id": "itest",
-                        "name": "Integrationstests",
+                        "name": "Integrationstests 1",
+                        "pattern": "target/i-junit.xml"
+                      },
+                      {
+                        "id": "itest",
+                        "name": "Integrationstests 2",
                         "pattern": "target/i-junit.xml"
                       }
                     ],
@@ -213,7 +224,12 @@ void shouldShowScoreWithTwoResults() {
                     "tools": [
                       {
                         "id": "mtest",
-                        "name": "Modultests",
+                        "name": "Modultests 1",
+                        "pattern": "target/m-junit.xml"
+                      },
+                      {
+                        "id": "mtest",
+                        "name": "Modultests 2",
                         "pattern": "target/m-junit.xml"
                       }
                     ],
@@ -231,10 +247,14 @@ void shouldShowScoreWithTwoResults() {
 
         assertThat(testMarkdown.createDetails(score))
                 .containsIgnoringWhitespaces(
-                        "One - 23 of 100",
-                        "|Integrationstests|5|3|4|12|23",
-                        "Two - 70 of 100",
-                        "|Modultests|0|0|10|10|-30",
+                        "One - 46 of 100",
+                        "|Integrationstests 1|5|3|4|12|23",
+                        "|Integrationstests 2|5|3|4|12|23",
+                        "|**Total**|**10**|**6**|**8**|**24**|**46**",
+                        "Two - 40 of 100",
+                        "|Modultests 1|0|0|10|10|-30",
+                        "|Modultests 2|0|0|10|10|-30",
+                        "|**Total**|**0**|**0**|**20**|**20**|**-60**",
                         ":moneybag:|*1*|*2*|*3*|:heavy_minus_sign:|:heavy_minus_sign:",
                         ":moneybag:|*-1*|*-2*|*-3*|:heavy_minus_sign:|:heavy_minus_sign:",
                         "__test-class-failed-0:test-failed-0__",
@@ -249,10 +269,10 @@ void shouldShowScoreWithTwoResults() {
                         "```text StackTrace-2```");
         assertThat(testMarkdown.createSummary(score))
                 .containsIgnoringWhitespaces(
-                        "One - 23 of 100",
-                        "4 tests failed, 5 passed, 3 skipped",
-                        "Two - 70 of 100",
-                        "10 tests failed, 0 passed");
+                        "One - 46 of 100",
+                        "8 tests failed, 10 passed, 6 skipped",
+                        "Two - 40 of 100",
+                        "20 tests failed, 0 passed");
     }
 
     @Test

src/test/java/edu/hm/hafner/grading/TestScoreTest.java

@@ -2,11 +2,11 @@
 
 import java.util.List;
 
+import org.apache.commons.lang3.StringUtils;
 import org.junit.jupiter.api.Test;
 
 import edu.hm.hafner.coverage.ClassNode;
 import edu.hm.hafner.coverage.ModuleNode;
-import edu.hm.hafner.coverage.Node;
 import edu.hm.hafner.coverage.TestCase.TestCaseBuilder;
 import edu.hm.hafner.coverage.TestCase.TestResult;
 import edu.hm.hafner.grading.TestScore.TestScoreBuilder;
@@ -182,28 +182,32 @@ void shouldHandleOverflowWithPositiveImpact() {
                 .hasValue(50);
     }
 
-    static Node createTestReport(final int passed, final int skipped, final int failed) {
-        var root = new ModuleNode(String.format("Tests (%d/%d/%d)", failed, skipped, passed));
+    static ModuleNode createTestReport(final int passed, final int skipped, final int failed) {
+        return createTestReport(passed, skipped, failed, StringUtils.EMPTY);
+    }
+
+    static ModuleNode createTestReport(final int passed, final int skipped, final int failed, final String prefix) {
+        var root = new ModuleNode(String.format("%sTests (%d/%d/%d)", prefix, failed, skipped, passed));
         var tests = new ClassNode("Tests");
         root.addChild(tests);
 
         for (int i = 0; i < failed; i++) {
             tests.addTestCase(new TestCaseBuilder()
-                    .withTestName("test-failed-" + i)
-                    .withClassName("test-class-failed-" + i)
-                    .withMessage("failed-message-" + i)
-                    .withDescription("StackTrace-" + i)
+                    .withTestName(prefix + "test-failed-" + i)
+                    .withClassName(prefix + "test-class-failed-" + i)
+                    .withMessage(prefix + "failed-message-" + i)
+                    .withDescription(prefix + "StackTrace-" + i)
                     .withStatus(TestResult.FAILED).build());
         }
         for (int i = 0; i < skipped; i++) {
             tests.addTestCase(new TestCaseBuilder()
-                    .withTestName("test-skipped-" + i)
-                    .withClassName("test-class-skipped-" + i)
+                    .withTestName(prefix + "test-skipped-" + i)
+                    .withClassName(prefix + "test-class-skipped-" + i)
                     .withStatus(TestResult.SKIPPED).build());
         }
         for (int i = 0; i < passed; i++) {
             tests.addTestCase(new TestCaseBuilder()
-                    .withTestName("test-passed-" + i)
+                    .withTestName(prefix + "test-passed-" + i)
                     .withStatus(TestResult.PASSED).build());
         }