Development: Exclude gradle source files from jacoco test report verification
#10240
Conversation
github-actions
bot
added
tests
lti
WalkthroughThis pull request introduces modifications to the Jacoco code coverage configuration in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java[ERROR] Error at ruleset.xml:58:5 59| 67| 72| 76| 79| 80| 82| 83| 84| 85| 86| 87| 88| 91| 92| 107| 120| 125| 135| 138| 185| ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ole-ve
dismissed stale reviews from HawKhiem and coderabbitai[bot]
via
fd0564b
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
gradle/jacoco.gradle (1)
Line range hint
137-145: Typographical Correction in Task Description
In theregisterJacocoTestCoverageVerificationmethod, the task description contains a typo: "vioalations" should be corrected to "violations". Fixing this will improve clarity in logging and reports.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradle/jacoco.gradle(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (6)
gradle/jacoco.gradle (6)
3-4: Threshold Adjustment: Aggregated Coverage Thresholds Updated
The new AggregatedCoverageThresholds now require an INSTRUCTION ratio of 0.8 (down from 0.89) and a CLASS limit of 66 (up from 56). Please verify that these revised thresholds align with the project's quality goals and do not unintentionally lower the code coverage standards.
45-46: Module-Specific Threshold: Iris Module Update
The iris module now has an INSTRUCTION threshold of 0.760 and a CLASS threshold of 25. Confirm that this change reflects the intended adjustment and that the iris module reports meet these stricter requirements.
87-89: Improved Ignored Directories Matching for Gradle Files
TheignoredDirectorieslist now specifies"org/gradle/**"and"**/gradle-wrapper.jar/**"without the leading double asterisks for some entries. This approach improves specificity in directory exclusion. Please ensure these patterns correctly exclude all Gradle source files as intended in the PR objectives.
107-116: AfterEvaluate Block in JacocoTestReport: Correct Exclusion Setup
The added afterEvaluate block adjusts theclassDirectoriesby creating a file tree that includes only files matching"$BasePath/**/*.class"and then excludes the directories inignoredDirectories. This is crucial for ensuring Gradle source files are not reported. Verify that the file tree resolves as expected in all build environments.
124-133: Coverage Verification: Condition and Directory Exclusion Logic
The jacocoTestCoverageVerification block now enables full coverage verification only when either no specific module is set or all modules are included, and it sets the classDirectories excluding the ignored directories. Double-check that this conditional correctly triggers the verification process in all bundle scenarios without inadvertently skipping any necessary checks.
Line range hint
146-155: Source and Class Directories Setup: Verify Variable Usage
In theprepareJacocoReportTaskmethod, the source directories are set with the expressiontask.sourceDirectories.setFrom(project.files("src/main/java/$BasePath/$moduleName/**/*.class"))and the class directories are filtered using
ignoredDirectories. Ensure that the variable$BasePathis correctly defined in the build script context so that the paths resolve properly; otherwise, some files may be overlooked or miscategorized.✅ Verification successful
Dependency Resolution Check for BasePath Definition
The search shows that
BasePathis defined ingradle/test.gradle(line 56) as:BasePath = "de/tum/cit/aet/artemis"and it is used in
gradle/jacoco.gradle(as seen at lines 87, 111, and 209). This indicates that the variable is correctly defined in the build script context and its usage in constructing source and class directory paths appears to be consistent. No misconfiguration was detected from the grep results.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for any definitions or usages of "BasePath" in the repository. rg "BasePath" -n .Length of output: 22939
Adjusted the thresholds. Got baited in thinking that the value is |
github-actions
bot
added
documentation
athena
github-actions
bot
removed
documentation
athena
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java (1)
382-439: LGTM! Excellent architecture test to enforce proper async handling.The changes enforce proper handling of asynchronous method calls in integration tests by requiring the use of wait methods (timeout, after, await). This helps prevent flaky tests and improves test reliability.
Consider extracting the magic number
2000inPyrisEventSystemIntegrationTest.javato a constant since it's used consistently across multiple test methods. This would make it easier to maintain and update the timeout value.+ private static final int ASYNC_TIMEOUT_MS = 2000;Then update the test methods to use this constant:
- verify(irisExerciseChatSessionService, timeout(2000).times(1)).onNewResult(any(Result.class)); - verify(pyrisPipelineService, after(2000).never()).executeExerciseChatPipeline(any(), any(), any(), any(), any()); + verify(irisExerciseChatSessionService, timeout(ASYNC_TIMEOUT_MS).times(1)).onNewResult(any(Result.class)); + verify(pyrisPipelineService, after(ASYNC_TIMEOUT_MS).never()).executeExerciseChatPipeline(any(), any(), any(), any(), any());src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java (4)
100-102: Consider using a constant for the logger name.Extract the logger name to a constant to improve maintainability and avoid potential typos.
+ private static final String ASYNC_EXCEPTION_LOGGER = SimpleAsyncUncaughtExceptionHandler.class.getName(); private ListAppender<ILoggingEvent> asyncExceptionLogAppender; private Logger asyncExceptionLog; - asyncExceptionLog = (Logger) LoggerFactory.getLogger(SimpleAsyncUncaughtExceptionHandler.class); + asyncExceptionLog = (Logger) LoggerFactory.getLogger(ASYNC_EXCEPTION_LOGGER);Also applies to: 149-152
106-153: Consider extracting test data setup into helper methods.The
setUpmethod is quite long and handles multiple concerns. Consider extracting the setup logic into smaller, focused helper methods for better readability and maintainability.+ private void setupTextExercise() { + textExercise = textExerciseUtilService.createSampleTextExercise(null); + textExercise.setFeedbackSuggestionModule(ATHENA_MODULE_TEXT_TEST); + textExercise = textExerciseRepository.save(textExercise); + + var textParticipation = participationUtilService.createAndSaveParticipationForExercise(textExercise, TEST_PREFIX + "student1"); + textSubmission = createAndSaveTextSubmission(textParticipation); + textBlock = createAndSaveTextBlock(textSubmission); + textFeedback = createAndSaveTextFeedback(participationUtilService.addResultToParticipation(textParticipation, textSubmission)); + } + + private TextSubmission createAndSaveTextSubmission(Participation textParticipation) { + var submission = new TextSubmission().text("Test - This is what the feedback references - Submission"); + submission.setParticipation(textParticipation); + return textSubmissionRepository.save(submission); + }
170-170: Consider using a constant for the timeout duration.Extract the timeout duration to a constant to improve maintainability and consistency across test methods.
+ private static final Duration DEFAULT_TIMEOUT = Duration.ofSeconds(10); + private static final Duration EMPTY_FEEDBACK_WAIT = Duration.ofSeconds(2); - await().atMost(Duration.ofSeconds(10)).untilAsserted(() -> athenaRequestMockProvider.verify()); + await().atMost(DEFAULT_TIMEOUT).untilAsserted(() -> athenaRequestMockProvider.verify());Also applies to: 214-214, 227-227, 234-234, 242-243
254-261: Consider using Optional for getExceptionName.The method returns null when no exception is present. Using Optional would make this more explicit and safer.
- @Nullable - private static String getExceptionName(ILoggingEvent event) { - IThrowableProxy throwableProxy = event.getThrowableProxy(); - if (throwableProxy == null) { - return null; - } - return throwableProxy.getClassName(); + private static Optional<String> getExceptionName(ILoggingEvent event) { + return Optional.ofNullable(event.getThrowableProxy()) + .map(IThrowableProxy::getClassName); }gradle/jacoco.gradle (3)
2-3: Consider using symbolic constants for target thresholds.Extract the target thresholds mentioned in the TODO comment into constants to make them more maintainable.
+ static final double TARGET_INSTRUCTION_COVERAGE = 0.90 + static final int TARGET_CLASS_COVERAGE = 0 // TODO: this should become 90% for INSTRUCTION and 0 for CLASS - AggregatedCoverageThresholds = ["INSTRUCTION": 0.888, "CLASS": 69] + AggregatedCoverageThresholds = ["INSTRUCTION": TARGET_INSTRUCTION_COVERAGE, "CLASS": TARGET_CLASS_COVERAGE]
84-84: Consider using a sorted list for reportedModules.The TODO comment suggests the need for alphabetical ordering. Consider sorting the list when it's created.
- // TODO: somehow the report order is in reversed alphabetical order, it would be great if it could be from A to Z finalizedBy reportedModules + .sort() .collect { module -> registerJacocoTestCoverageVerification(module as String, jacocoTestCoverageVerification) }
190-254: Consider extracting XML parsing logic into a separate method.The XML parsing logic in the
doLastblock is quite complex. Consider extracting it into a separate method for better maintainability.+ private static void parseAndValidateCoverageReport(File reportsFile, String moduleName, double minInstructionCoveredRatio, int maxNumberUncoveredClasses) { + if (!reportsFile.exists()) { + println "⚠️ Jacoco report not found for $moduleName" + return + } + // ... rest of the XML parsing logic ... + } task.doLast { - // ... current XML parsing logic ... + def moduleName = task.name.replace('jacocoTestCoverageVerification-', '') + if (moduleName == "aggregated") { + moduleName = "Aggregated Code Coverage" + } + def reportsFile = project.file("${task.project.layout.buildDirectory.get().asFile}/reports/jacoco/$moduleName/jacocoTestReport.xml") + parseAndValidateCoverageReport(reportsFile, moduleName, minInstructionCoveredRatio, maxNumberUncoveredClasses) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
docs/user/exercises/programming/feedback-analysis-affected-students.pngis excluded by!**/*.png,!**/*.pngdocs/user/exercises/programming/feedback-analysis-channel.pngis excluded by!**/*.png,!**/*.pngdocs/user/exercises/programming/feedback-analysis-detail.pngis excluded by!**/*.png,!**/*.pngdocs/user/exercises/programming/feedback-analysis-filters.pngis excluded by!**/*.png,!**/*.pngdocs/user/exercises/programming/feedback-analysis-overview.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (5)
docs/user/exercises/programming-exercise-setup.inc(1 hunks)gradle/jacoco.gradle(6 hunks)src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.java(5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/test/java/**/*.java`: test_naming: descriptive; test_si...
src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/shared/architecture/ArchitectureTest.javasrc/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-style
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: client-tests-selected
- GitHub Check: Analyse
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/iris/PyrisEventSystemIntegrationTest.java (2)
304-305: LGTM! Improved test reliability by handling asynchronous calls.The changes correctly handle asynchronous method calls by:
- Using
timeout(2000)to wait foronNewResultto be called.- Using
after(2000)to verify thatexecuteExerciseChatPipelineis never called.
319-320: LGTM! Improved test reliability by handling asynchronous calls.The changes correctly handle asynchronous method calls by:
- Using
timeout(2000)to wait foronNewResultto be called.- Using
after(2000)to verify thatexecuteExerciseChatPipelineis never called.src/test/java/de/tum/cit/aet/artemis/athena/service/connectors/AthenaFeedbackSendingServiceTest.java (1)
5-6: LGTM! Good use of static imports for assertions and Awaitility.The static imports improve readability by reducing verbosity in test methods.
docs/user/exercises/programming-exercise-setup.inc (1)
725-802: LGTM! Well-structured and comprehensive documentation.The new section provides clear instructions and visual aids for using the Feedback Analysis feature. The documentation is well-organized with:
- Clear navigation instructions
- Detailed feature descriptions
- Supporting screenshots
- Video tutorial
Checklist
General
Motivation and Context
ignoredDirectoriesby removing suffix wildcard.jacocoReportTaskandjacocoTestCoverageVerification(Artemis-bundle scope task instead per-module)Also updates the iris and Artemis-bundle coverage thresholds.
Steps for Testing
:jacocoTestReport(1) Log of succeeding pipeline commit before changes - Commit
(2) Log of succeeding pipeline commit after changes - Commit
Summary by CodeRabbit