Integrated code lifecycle: Add blackbox tests to LocalCI

[chrisknedl]

Checklist

General

• I tested all changes and their related features with all corresponding user types on a test server.
• This is a small issue that I tested locally and was confirmed by another developer on a test server.
• Language: I followed the guidelines for inclusive, diversity-sensitive, and appreciative language.
• I chose a title conforming to the naming conventions for pull requests.

Server

• Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
• I strictly followed the principle of data economy for all database calls.
• I strictly followed the server coding and design guidelines.
• I added multiple integration tests (Spring) related to the features (with a high test coverage).
• I documented the Java code using JavaDoc style.

Changes affecting Programming Exercises

• High priority: I tested all changes and their related features with all corresponding user types on a test server configured with the integrated lifecycle setup (LocalVC and LocalCI).

Motivation and Context

For Java exercises, the DejaGnu project type was previously only available for Jenkins Setups. LocalCI should also support this.

Description

The Aeolus templates for blackbox tests are updated. Aside from that, the Custom Feedback Parser that is part of the Jenkins server notification plugin (https://github.com/ls1intum/jenkins-server-notification-plugin/tree/main/src/main/java/de/tum/in/www1/jenkins/notifications) is included in the Artemis backend.
Since there are now two Parsers, XML and Custom, the records that were previously contained in the XML Parser are now put in a new package, so that both parsers can access them. A message attribute is added to the TestCase Record, because we need to see the results of the DejaGnu test runs.
Specification of the JSON feedback format: https://docs.artemis.cit.tum.de/user/exercises/programming.html#jenkins

Steps for Testing

Prerequisites:

• 1 Instructor

1. Log in to Artemis
2. Create a Java programming exercise with project type 'DejaGnu', online editor and static code analysis enabled.
3. View the exercise in the editor. Make sure that the template repo build contains one test case with description: 'Could not find a main method!'
4. The solution repo build should contain six test cases and look like the screenshot below.
   The '(See more)' items should all be expandable and show the whole feedback test.
5. To test SCA, add a magic number in one of the files of the solution repo, e.g. int blabla = 23;
6. Rerun the solution build, it should show the SCA feedback as well now.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.

Review Progress

Code Review

• Code Review 1
• Code Review 2

Manual Tests

• Test 1
• Test 2

Test Coverage

Class/File	Line Coverage	Confirmation (assert/expect)
CustomFeedbackParser.java	86%	✅

Screenshots

Summary by CodeRabbit

Release Notes

• New Features

  • Added support for parsing custom JSON and XML test result files.
  • Introduced new project type MAVEN_BLACKBOX for Java programming language.
  • Added new records: CustomFeedback, TestCase, TestSuite, and TestSuites for enhanced test result representation.
  • Introduced a new class Failure for handling error messages in test results.

• Improvements

  • Enhanced test result parsing with more robust error handling.
  • Improved build job execution service to handle multiple file formats.
  • Increased expected values for test templates related to MAVEN_BLACKBOX.

• Technical Changes

  • Refactored test result parsing classes and methods.
  • Updated shell script templates for build processes.
  • Improved naming and structure of utility classes and methods.
  • Added unit tests for CustomFeedbackParser to ensure comprehensive coverage.

[HawKhiem]

Tesed on TS3. Works as described

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/main/java/de/tum/cit/aet/artemis/buildagent/service/parser/CustomFeedbackParser.java (1)

60-67: ⚠️ Potential issue

Use appropriate exception type and consistent message formatting

The use of InvalidPropertiesFormatException is not appropriate for this context.

-    private static void validateCustomFeedback(final String fileName, final CustomFeedback feedback) throws InvalidPropertiesFormatException {
+    private static void validateCustomFeedback(final String fileName, final CustomFeedback feedback) {
         if (feedback.name() == null || feedback.name().trim().isEmpty()) {
-            throw new InvalidPropertiesFormatException(String.format("Custom feedback from file %s needs to have a name attribute.", fileName));
+            throw new IllegalArgumentException(String.format("Custom feedback from file %s needs to have a name attribute.", fileName));
         }
         if (!feedback.successful() && feedback.message() == null) {
-            throw new InvalidPropertiesFormatException(String.format("Custom non-success feedback from file %s needs to have a message", fileName));
+            throw new IllegalArgumentException(String.format("Custom non-success feedback from file %s needs to have a message.", fileName));
         }
     }

🧹 Nitpick comments (6)

src/main/java/de/tum/cit/aet/artemis/buildagent/service/parser/CustomFeedbackParser.java (6)

21-21: Consider making ObjectMapper thread-safe

The static ObjectMapper instance should be configured for thread safety since it might be accessed concurrently. Consider using JsonMapper.builder().build() or enabling thread-safe features.

-    private static final ObjectMapper mapper = new ObjectMapper();
+    private static final ObjectMapper mapper = JsonMapper.builder()
+            .enable(MapperFeature.BLOCK_UNSAFE_POLYMORPHIC_BASE_TYPES)
+            .build();

---

41-44: Enhance error handling specificity

The catch block catches all IOException instances but only logs them. Consider:

1. Catching more specific exceptions (JsonParseException, JsonMappingException)
2. Propagating the error to the caller for better error handling

-        catch (IOException e) {
+        catch (JsonParseException | JsonMappingException e) {
+            log.error("Invalid JSON format in custom feedback file {}. {}", fileName, e.getMessage(), e);
+            throw new IllegalArgumentException("Failed to parse custom feedback file: " + fileName, e);
+        }
+        catch (IOException e) {
             log.error("Error during custom Feedback creation. {}", e.getMessage(), e);
-            return;
+            throw new UncheckedIOException("Failed to read custom feedback file: " + fileName, e);
         }

---

45-45: Remove redundant comment

The comment is redundant as the control flow makes it clear that the code below only executes if no exception occurs.

-        // Only create TestCase if there was no exception during the custom feedback extraction
         final TestCase testCase = customFeedbackToTestCase(feedback);

---

73-73: Fix incorrect return type documentation

The @return documentation mentions returning a "Testsuite" but the method returns a TestCase.

-     * @return A Testsuite in the same format as used by JUnit reports
+     * @return A TestCase in the same format as used by JUnit reports

---

75-86: Simplify the conversion logic

The conversion logic can be simplified using a ternary operator and method references.

     private static TestCase customFeedbackToTestCase(final CustomFeedback feedback) {
-        final TestCase testCase;
-        if (feedback.successful()) {
-            testCase = new TestCase(feedback.name(), null, null, null, feedback.message());
-        }
-        else {
-            final Failure failure = new Failure();
-            failure.setMessage(feedback.message());
-            testCase = new TestCase(feedback.name(), failure, null, null, null);
-        }
-        return testCase;
+        return feedback.successful()
+            ? new TestCase(feedback.name(), null, null, null, feedback.message())
+            : new TestCase(feedback.name(), createFailure(feedback.message()), null, null, null);
+    }
+
+    private static Failure createFailure(String message) {
+        final Failure failure = new Failure();
+        failure.setMessage(message);
+        return failure;
     }

---

88-99: Enhance null handling and immutability

Consider using Optional for cleaner null handling and make variables final where possible.

     private static void processTestCase(final TestCase testCase, final List<LocalCITestJobDTO> failedTests, final List<LocalCITestJobDTO> successfulTests) {
         if (testCase.isSkipped()) {
             return;
         }
-        Failure failure = testCase.extractFailure();
-        if (failure != null) {
-            failedTests.add(new LocalCITestJobDTO(testCase.name(), List.of(failure.extractMessage())));
-        }
-        else {
-            successfulTests.add(new LocalCITestJobDTO(testCase.name(), List.of(testCase.extractSuccessMessage())));
-        }
+        Optional.ofNullable(testCase.extractFailure())
+            .ifPresentOrElse(
+                failure -> failedTests.add(new LocalCITestJobDTO(testCase.name(), List.of(failure.extractMessage()))),
+                () -> successfulTests.add(new LocalCITestJobDTO(testCase.name(), List.of(testCase.extractSuccessMessage())))
+            );
     }

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6b5379e and 0c71745.

📒 Files selected for processing (1)

• src/main/java/de/tum/cit/aet/artemis/buildagent/service/parser/CustomFeedbackParser.java (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

src/main/java/de/tum/cit/aet/artemis/buildagent/service/parser/CustomFeedbackParser.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

⏰ Context from checks skipped due to timeout of 90000ms (7)

• GitHub Check: client-style
• GitHub Check: server-style
• GitHub Check: client-tests
• GitHub Check: Build and Push Docker Image
• GitHub Check: server-tests
• GitHub Check: Analyse
• GitHub Check: Build .war artifact

[b-fein]

Code 👍

[flbrgit]

Tested on TS4. Works as described.