Development: Execute server tests only for changed modules in GitHub action for non-default branches
#10036
Conversation
This reverts commit 786d172.
There was a problem hiding this comment.
I've looked through the workflow and since we still run all tests for MySql and Postgres / the default branch, I understand that the downsides are limited. I also really like the idea of coverage thresholds per module.
Thanks for the clarification and the improvement. Keep it up so we can see the benefit of module specific reports manifest :)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
gradle/jacoco.gradle (1)
190-191:⚠️ Potential issuePrevent path traversal vulnerability
The module path construction should validate the module name to prevent path traversal.
+ if (!moduleName.matches("^[a-zA-Z0-9-_]+$")) { + throw new GradleException("Invalid module name: $moduleName") + } def modulePath = "$BasePath/$moduleName/**/*.class" task.sourceDirectories.setFrom(project.files("src/main/java/$modulePath"))
🧹 Nitpick comments (4)
gradle/jacoco.gradle (4)
98-110: Add documentation for the test report configurationWhile the configuration is correct, it would benefit from comments explaining:
- The purpose of the XML and HTML reports
- How the dynamic module list works with
reportedModules- The relationship between the aggregated and per-module reports
112-115: Clarify the verification enabling logicThe condition
enabled = reportedModules.size() == 0might not be immediately clear to other developers. Consider adding a comment explaining why verification is only enabled when no specific modules are set.
170-171: Fix typo in task descriptionThere's a typo in the task description: "vioalations" should be "violations"
- task.description = "Validates JaCoCo coverage for vioalations for $moduleName" + task.description = "Validates JaCoCo coverage violations for $moduleName"
133-149: Add parameter validation in helper methodsThe helper methods should validate their input parameters to prevent NullPointerException.
Add null checks for the parameters:
private JacocoReport registerJacocoReportTask(String moduleName, JacocoReport rootTask) { + Objects.requireNonNull(moduleName, "moduleName must not be null") + Objects.requireNonNull(rootTask, "rootTask must not be null") def taskName = "jacocoCoverageReport-$moduleName" // ... rest of the method } private JacocoCoverageVerification registerJacocoTestCoverageVerification(String moduleName, JacocoCoverageVerification rootTask) { + Objects.requireNonNull(moduleName, "moduleName must not be null") + Objects.requireNonNull(rootTask, "rootTask must not be null") def taskName = "jacocoTestCoverageVerification-$moduleName" // ... rest of the method }Also applies to: 158-176
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradle/jacoco.gradle(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
gradle/jacoco.gradle (1)
20-23: Address low test coverage in buildagent moduleThe buildagent module has a significantly low instruction coverage threshold of 31.3%, which is considerably lower than other modules (most are above 75%). This indicates potential testing gaps that should be addressed.
Consider:
- Investigating why the coverage is so low
- Creating a plan to improve test coverage
- Setting a timeline for gradually increasing the threshold
ole-ve
dismissed stale reviews from coderabbitai[bot], DominikRemo, BBesrour, and SimonEntholzer
via
cf9bebb
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
check.sh (1)
6-12: Improve array handling and IFS usage.The current implementation has potential issues with array handling and IFS modification.
if [ -n "${CHANGED_MODULES}" ]; then # Always execute ArchitectureTests CHANGED_MODULES+=("ArchitectureTest") - IFS=, - TEST_MODULE_TAGS=$(echo "-DtestTags=${CHANGED_MODULES[*]}") + # Use local IFS to avoid affecting the rest of the script + local IFS=, + TEST_MODULE_TAGS="-DtestTags=${CHANGED_MODULES[*]}" figradle/test.gradle (2)
48-57: Document the purpose of ext properties and improve null handling.The ext block needs better documentation and null handling.
// Allow using in jacoco.gradle ext { + // Property to specify test tags to include (comma-separated) includedTestTags = System.getProperty("includeTags") - includedTags = !includedTestTags ? new String[]{} : includedTestTags.split(",") as String[] + includedTags = includedTestTags?.split(",") as String[] ?: new String[]{} + // Property to specify modules to include (comma-separated) includedModulesTag = System.getProperty("includeModules") - includedModules = !includedModulesTag ? new String[]{} : includedModulesTag.split(",") as String[] + includedModules = includedModulesTag?.split(",") as String[] ?: new String[]{} + // Flag to determine if all tests should be run runAllTests = includedTags.size() == 0 && includedModules.size() == 0 + // Base package path for module-specific test filtering BasePath = "de/tum/cit/aet/artemis" }
93-94: Document the purpose of jacoco.gradle integration.Add documentation explaining the purpose of the jacoco.gradle integration and its relationship with module-specific coverage.
-// Dynamic generation of jacoco test report generation-/coverage verification-tasks (per-module) +/** + * Integration with jacoco.gradle for test coverage reporting and verification + * - Generates per-module coverage reports + * - Configures coverage verification thresholds + * - Supports both aggregated and module-specific coverage analysis + */ apply from: "gradle/jacoco.gradle"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docker/test-server-multi-node-postgresql-localci.ymlis excluded by!**/*.yml
📒 Files selected for processing (6)
build.gradle(1 hunks)check.sh(1 hunks)gradle/test.gradle(2 hunks)per-module-coverage-recorder.sh(1 hunks)read.ms(1 hunks)result.md(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- result.md
- read.ms
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
🧰 Additional context used
🪛 Shellcheck (0.10.0)
per-module-coverage-recorder.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
check.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: Mend Security Check
🔇 Additional comments (1)
gradle/test.gradle (1)
74-80: Verify module path construction and improve error handling.The module path construction could be more robust.
✅ Verification successful
Module path construction is valid and robust
The verification confirms that all module paths follow the correct pattern
de.tum.cit.aet.artemis.<module>and exist in the codebase, including the "shared" module. The current implementation is appropriate and doesn't require additional error handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all module paths exist for module in assessment athena atlas buildagent communication core exam exercise fileupload iris lecture lti modeling plagiarism programming quiz text tutorialgroup shared; do echo "Checking path: de/tum/cit/aet/artemis/$module" if ! find . -type d -path "*/de/tum/cit/aet/artemis/$module" | grep -q .; then echo "Warning: Module path not found: $module" fi doneLength of output: 6594
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
gradle/test.gradle (6)
48-57: Consider moving BasePath to a constants file.The
BasePathconstant represents a core package structure that might be used across different Gradle files. Consider moving it to a dedicated constants file to improve maintainability and reusability.
62-62: Update comment to reflect new module-based execution.The comment should be updated to include examples of both tag-based and module-based test execution for better documentation.
-// Execute the test cases: ./gradlew test -// Execute only architecture tests: ./gradlew test -DincludeTags="ArchitectureTest" +// Examples: +// Execute all test cases: ./gradlew test +// Execute only architecture tests: ./gradlew test -DincludeTags="ArchitectureTest" +// Execute tests for specific modules: ./gradlew test -DincludeModules="atlas,shared"
74-80: Consider making the shared module inclusion configurable.The "shared" module is always included when running module-specific tests. Consider making this configurable in case there are scenarios where we don't want to run shared module tests.
- // Always execute "shared"-folder when executing module-specifc tests - includedModules += "shared" + // Include shared module tests by default, unless explicitly disabled + if (!project.hasProperty('skipSharedTests')) { + includedModules += "shared" + }
77-79: Make the package structure more flexible.The test filter assumes a fixed package structure. Consider making this more flexible to accommodate future package restructuring.
- testFilter.includeTestsMatching("de.tum.cit.aet.artemis.$val.*") + testFilter.includeTestsMatching("${project.ext.BasePath}.$val.*")
Line range hint
84-91: Consider dynamic heap size configuration based on module size.The heap size settings are fixed for all test executions. For module-specific tests, these values might be excessive. Consider making them configurable based on the modules being tested.
- minHeapSize = "2g" // initial heap size - maxHeapSize = "6g" // maximum heap size + // Configure heap sizes based on test scope + minHeapSize = runAllTests ? "2g" : "1g" + maxHeapSize = runAllTests ? "6g" : "3g"
93-94: Add validation for Jacoco configuration file.Add a check to ensure the Jacoco configuration file exists before applying it to prevent build failures.
-apply from: "gradle/jacoco.gradle" +def jacocoConfig = file("gradle/jacoco.gradle") +if (!jacocoConfig.exists()) { + throw new GradleException("Jacoco configuration file not found at: ${jacocoConfig.absolutePath}") +} +apply from: jacocoConfig
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradle/test.gradle(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-tests-selected
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Analyse
- GitHub Check: Build .war artifact
🔇 Additional comments (1)
gradle/test.gradle (1)
64-80: Implementation aligns well with PR objectives.The test configuration successfully implements module-specific test execution while maintaining support for tag-based filtering. This aligns perfectly with the PR's goal of improving test efficiency by running tests only for changed modules.
krusche
changed the title
Development: Execute only tests for changed modules in GitHub test job for non-default branchesDevelopment: Execute server tests only for changed modules in GitHub action for non-default branches
Checklist
General
Motivation and Context
Most often developers don't change functionality related to all of Artemis, but only to some modules. Thus, running the whole test-suite for each commit is ineffective. Instead, for pull requests etc. it might be fine to run the reduced test suite for the in-memory DB test pipeline on non-default branches. To ensure everything still works, all tests should be run for every default-branch push.
Description
Matching tests and source code
By determining the module-folders with changed files, the tests for the respective module are executed. For example changing
src/main/java/de/tum/cit/aet/artemis/text/service/ABCService.java, all tests within the foldersrc/main/java/de/tum/cit/aet/artemis/textare executed.This is a first step and in the future, it might be an idea to extend the reduced test set to modules on which the changed module is dependent. This however, is not that trivial because of transitivity.
Per-Module Coverage Report & Verification
When running a reduced set of tests for specific modules, we might be interested in having per-module reports and validate thresholds per-module. Validating on a module basis is not only more insightful but also required as running a reduced set of tests might lead to reduced coverage metrics causing verification to fail.
Steps for Testing
These testing steps are a bit different and don't involve Artemis.
git checkout -b feat/test-changed-modules-1.github/workflows/test.ymlScreenshots
The job summary generated on a branch where a single file in the
fileupload-module has been changed.Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores