Development: Execute tests only for changed modules on non-default branchs
#10036
Conversation
This reverts commit 786d172.
WalkthroughThis pull request introduces several modifications across different scripts and configuration files, primarily focusing on enhancing code coverage reporting and testing infrastructure. The changes include updating the pull request template, modifying the Gradle build configuration to improve test execution flexibility, introducing a new Jacoco coverage configuration, adding a Python script for parsing module coverage reports, and creating a shell script to identify changed modules. The modifications aim to improve test flexibility, coverage reporting, and CI/CD processes. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
gradle/jacoco.gradle (1)
162-162: Fix typo in task descriptionThere's a typo in the task description: "Validates JaCoCo coverage for vioalations for $moduleName". Please correct "vioalations" to "violations".
Apply this change:
- task.description = "Validates JaCoCo coverage for vioalations for $moduleName" + task.description = "Validates JaCoCo coverage for violations for $moduleName"supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py (2)
11-11: Remove unnecessary f-string prefixThe
fprefix is unnecessary since there are no placeholders in the string. Removing it avoids confusion and aligns with Python conventions.Apply this change:
- report_file = os.path.join(module_path, f"jacocoCoverageReport.xml") + report_file = os.path.join(module_path, "jacocoCoverageReport.xml")🧰 Tools
🪛 Ruff (0.8.2)
11-11: f-string without any placeholders
Remove extraneous
fprefix(F541)
35-35: Use 'is None' for comparisons to NoneWhen comparing to
None, it's recommended to useis Noneinstead of== Nonefor correctness and readability.Apply this change:
- if instruction_counter == None or class_counter == None: + if instruction_counter is None or class_counter is None:🧰 Tools
🪛 Ruff (0.8.2)
35-35: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
35-35: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
docker/monitoring/grafana/provisioning/dashboards/artemis/endpoint_monitoring.json (2)
458-458: Remove "Copy" from dashboard title.The dashboard title "Endpoint Monitoring Copy" suggests this is a duplicate. Consider removing "Copy" from the title for production use.
- "title": "Endpoint Monitoring Copy", + "title": "Endpoint Monitoring",
31-78: Consider adding thresholds for request duration.The panel configuration could benefit from warning/critical thresholds to highlight performance issues.
Add threshold steps to the configuration:
"thresholds": { "mode": "absolute", "steps": [ { "color": "green", "value": null - } + }, + { + "color": "yellow", + "value": 1000 + }, + { + "color": "red", + "value": 2000 + } ] }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/dependabot.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)build.gradle(2 hunks)docker/monitoring/grafana/provisioning/dashboards/artemis/endpoint_monitoring.json(1 hunks)gradle/jacoco.gradle(1 hunks)supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py(1 hunks)supporting_scripts/extract_number_of_server_starts.sh(1 hunks)supporting_scripts/get_changed_modules.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/PULL_REQUEST_TEMPLATE.md
🧰 Additional context used
🪛 Ruff (0.8.2)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
11-11: f-string without any placeholders
Remove extraneous f prefix
(F541)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
51-51: Undefined name module_folder
(F821)
🔇 Additional comments (8)
gradle/jacoco.gradle (2)
81-84: Verify that 'includedModules' is defined
The variable includedModules is used to determine reportedModules, but it is not defined within this file. Ensure that includedModules is defined and accessible in this context to prevent potential runtime errors.
88-90: Verify that '$BasePath' variable is defined
The variable $BasePath is used in ignoredDirectories and modulePath, but it is not defined within this file. Please verify that $BasePath is properly defined and accessible to avoid issues with path resolutions.
Also applies to: 182-183
supporting_scripts/extract_number_of_server_starts.sh (1)
12-14: Update to server start count validation is appropriate
The modification to check if the number of server starts is greater than 4 and the updated error message accurately reflect the intended validation logic.
supporting_scripts/get_changed_modules.sh (1)
1-27: Script correctly identifies changed modules
The script effectively determines which modules have changed compared to a specified branch and outputs a comma-separated list. The implementation follows best practices and is correct.
docker/monitoring/grafana/provisioning/dashboards/artemis/endpoint_monitoring.json (1)
105-115: LGTM: Well-structured Prometheus query for request duration.
The query correctly:
- Uses rate() for calculating average duration
- Includes proper time window with $__rate_interval
- Converts to milliseconds for better readability
build.gradle (3)
121-130: LGTM: Well-structured test configuration properties.
The ext block provides a clean way to manage test execution configuration with:
- Clear property names
- Proper null handling
- Good separation of tags and modules
134-152: LGTM: Improved test execution logic.
The test task configuration properly handles:
- Default case (all tests except integration tests)
- Tag-based test selection
- Module-specific test selection with automatic inclusion of shared module
173-174: Verify jacoco.gradle configuration.
The Jacoco configuration has been moved to an external file. Let's verify its contents.
✅ Verification successful
Jacoco configuration appears well-structured with appropriate coverage thresholds
The jacoco.gradle file contains a comprehensive configuration with:
- Aggregated coverage thresholds (90% instruction coverage, max 10 uncovered classes)
- Per-module specific thresholds defined for all modules
- Proper task configuration for both reporting and verification
- Appropriate exclusion patterns for generated files
- Latest Jacoco tool version (0.8.12)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check jacoco.gradle content and coverage thresholds
# Check if jacoco.gradle exists and show its content
cat gradle/jacoco.gradle
Length of output: 6744
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
Outdated
Show resolved
Hide resolved
WalkthroughThis pull request introduces several changes across different files, focusing on improving testing, coverage reporting, and script functionality. The modifications include updating the pull request template, enhancing build configurations, adding a new Grafana dashboard for endpoint monitoring, introducing a comprehensive Jacoco coverage configuration, and creating utility scripts for module and coverage analysis. The changes aim to provide more flexibility in test execution, improve code coverage reporting, and streamline development workflows. Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer
participant PR as Pull Request Template
participant Build as Build System
participant Coverage as Coverage Script
participant Report as Coverage Report
Dev->>PR: Submits Pull Request
PR->>Build: Triggers Build
Build->>Coverage: Run Coverage Generation
Coverage->>Report: Generate Detailed Report
Report->>Dev: Provide Coverage Insights
Possibly related PRs
Suggested Labels
Suggested Reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
gradle/jacoco.gradle (1)
162-162: Fix typographical error in task descriptionThe word "vioalations" in the task description should be "violations".
Apply this diff to correct the typo:
-task.description = "Validates JaCoCo coverage for vioalations for $moduleName" +task.description = "Validates JaCoCo coverage for violations for $moduleName"build.gradle (2)
146-146: Fix typographical error in commentIn the comment, "module-specifc" should be "module-specific".
Apply this diff to correct the typo:
- // Always execute "shared"-folder when executing module-specifc tests + // Always execute "shared"-folder when executing module-specific tests
173-173: Improve comment clarityConsider rephrasing the comment for better readability.
Apply this diff to rephrase the comment:
-// Dynamic generation of jacoco test report generation-/coverage verification-tasks (per-module) +// Dynamically generate Jacoco test report and coverage verification tasks per modulesupporting_scripts/extract_number_of_server_starts.sh (1)
14-14: Improve clarity and grammar in error messageThe error message can be improved for clarity and correct grammar.
Apply this diff to correct the message:
- echo "The number of Server Starts should be lower than/equals 4! Please adapt this check if the change is intended or try to fix the underlying issue causing a different number of server starts!" + echo "The number of Server Starts should be less than or equal to 4! Please adapt this check if the change is intended or try to fix the underlying issue causing a different number of server starts!"supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py (4)
11-11: Remove unnecessary f-string prefixThe string doesn't contain any interpolation, so the
fprefix is unnecessary.- report_file = os.path.join(module_path, f"jacocoCoverageReport.xml") + report_file = os.path.join(module_path, "jacocoCoverageReport.xml")🧰 Tools
🪛 Ruff (0.8.2)
11-11: f-string without any placeholders
Remove extraneous
fprefix(F541)
35-35: Useis Nonefor None comparisonReplace
== Nonewithis Nonefor proper Python idioms.- if instruction_counter == None or class_counter == None: + if instruction_counter is None or class_counter is None:🧰 Tools
🪛 Ruff (0.8.2)
35-35: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
35-35: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
24-54: Consider adding type hintsAdding type hints would improve code maintainability and IDE support.
-def extract_coverage(reports): +def extract_coverage(reports: list[dict[str, str]]) -> list[dict[str, float | str | int]]:🧰 Tools
🪛 Ruff (0.8.2)
35-35: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
35-35: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
51-51: Undefined name
module_folder(F821)
57-64: Add error handling for file operationsConsider adding try-except block to handle potential file operation errors.
def write_summary_to_file(coverage_by_module, output_file): - with open(output_file, "w") as f: - f.write("## Coverage Results\n\n") - f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n") - f.write("|-------------|---------------------------|----------------|\n") - for result in coverage_by_module: - f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n") + try: + with open(output_file, "w") as f: + f.write("## Coverage Results\n\n") + f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n") + f.write("|-------------|---------------------------|----------------|\n") + for result in coverage_by_module: + f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n") + except IOError as e: + print(f"Error writing to file {output_file}: {e}") + raisedocker/monitoring/grafana/provisioning/dashboards/artemis/endpoint_monitoring.json (3)
458-458: Remove 'Copy' from dashboard titleThe dashboard title suggests this is a copy. Update it to reflect its actual purpose.
- "title": "Endpoint Monitoring Copy", + "title": "Endpoint Monitoring",
459-459: Consider removing hard-coded dashboard UIDThe hard-coded UID could cause conflicts when importing the dashboard in different environments. Consider removing it to let Grafana generate a new one during import.
- "uid": "fc1fba24-8e72-439a-9759-b35315c1e2ee", + "uid": null,
107-107: Consider adding rate limiting to Prometheus queriesThe topk queries without proper rate limiting could impact Prometheus performance. Consider adding a max() time range.
- "expr": "topk($topNumberEndpoints, ...", + "expr": "max_over_time(topk($topNumberEndpoints, ...)[$__interval])",Also applies to: 204-204, 307-307, 407-407
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
.github/dependabot.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
.github/PULL_REQUEST_TEMPLATE.md(1 hunks)build.gradle(2 hunks)docker/monitoring/grafana/provisioning/dashboards/artemis/endpoint_monitoring.json(1 hunks)gradle/jacoco.gradle(1 hunks)supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py(1 hunks)supporting_scripts/extract_number_of_server_starts.sh(1 hunks)supporting_scripts/get_changed_modules.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/PULL_REQUEST_TEMPLATE.md
🧰 Additional context used
🪛 Ruff (0.8.2)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
11-11: f-string without any placeholders
Remove extraneous f prefix
(F541)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
51-51: Undefined name module_folder
(F821)
🔇 Additional comments (4)
gradle/jacoco.gradle (1)
1-209: Overall code structure and logic is sound
The implementation of coverage thresholds and dynamic generation of Jacoco tasks is well-structured and follows best practices.
build.gradle (1)
135-153: Conditional test execution logic is well implemented
The updates to the test task correctly handle conditional test execution based on includeModules and includeTags, enhancing flexibility in the testing process.
supporting_scripts/extract_number_of_server_starts.sh (1)
12-12: Logical condition update is appropriate
Updating the condition to check if numberOfStarts is greater than 4 aligns with the intended behavior to allow up to 4 server starts.
supporting_scripts/get_changed_modules.sh (1)
1-27: Script correctly identifies changed modules
The script efficiently identifies and lists the changed modules compared to a specified branch, enhancing the testing process.
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
Outdated
Show resolved
Hide resolved
docker/monitoring/grafana/provisioning/dashboards/artemis/endpoint_monitoring.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py (3)
5-22: Consider these improvements for better robustness and maintainability.While the function works correctly, here are some suggested improvements:
-def get_report_by_module(input_directory): +def get_report_by_module(input_directory: str) -> list[dict[str, str]]: + if not os.path.isdir(input_directory): + raise ValueError(f"Input directory does not exist: {input_directory}") results = [] for module_folder in os.listdir(input_directory): module_path = os.path.join(input_directory, module_folder) if os.path.isdir(module_path): - report_file = os.path.join(module_path, f"jacocoCoverageReport.xml") + report_file = os.path.join(module_path, "jacocoCoverageReport.xml")🧰 Tools
🪛 Ruff (0.8.2)
11-11: f-string without any placeholders
Remove extraneous
fprefix(F541)
57-64: Add error handling and type safety.The function would benefit from better error handling and type hints:
-def write_summary_to_file(coverage_by_module, output_file): +def write_summary_to_file(coverage_by_module: list[dict[str, Union[str, float, int]]], + output_file: str) -> None: + if not coverage_by_module: + print("Warning: No coverage data to write") + return + + os.makedirs(os.path.dirname(output_file), exist_ok=True) try: with open(output_file, "w") as f: f.write("## Coverage Results\n\n") f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n") f.write("|-------------|---------------------------|----------------|\n") for result in coverage_by_module: f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n") + except IOError as e: + raise IOError(f"Failed to write coverage report to {output_file}: {e}")
66-75: Add comprehensive error handling to the main workflow.The CLI interface would benefit from proper error handling and exit codes:
if __name__ == "__main__": + import sys + parser = argparse.ArgumentParser(description="Process JaCoCo coverage reports.") parser.add_argument("input_directory", type=str, help="Root directory containing JaCoCo coverage reports") parser.add_argument("output_file", type=str, help="Output file to save the coverage results") args = parser.parse_args() - - reports = get_report_by_module(args.input_directory) - coverage_by_module = extract_coverage(reports) - write_summary_to_file(coverage_by_module, args.output_file) + try: + reports = get_report_by_module(args.input_directory) + if not reports: + print("No coverage reports found", file=sys.stderr) + sys.exit(1) + + coverage_by_module = extract_coverage(reports) + write_summary_to_file(coverage_by_module, args.output_file) + sys.exit(0) + except Exception as e: + print(f"Error: {e}", file=sys.stderr) + sys.exit(1)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/test.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
11-11: f-string without any placeholders
Remove extraneous f prefix
(F541)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (1)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py (1)
1-4: LGTM! Clean and appropriate imports.
The imports are minimal, well-organized, and use standard library modules appropriate for the task.
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
gradle/jacoco.gradle (5)
20-23: Consider addressing low coverage in buildagent moduleThe buildagent module has a significantly low coverage threshold of 30.4%, which is considerably lower than other modules. This might indicate technical debt that should be addressed.
Consider creating a separate ticket to improve test coverage for the buildagent module. This will help maintain code quality and reduce the risk of regressions.
170-170: 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"
113-114: Document verification behaviorThe verification is disabled when specific modules are selected. While this aligns with the PR's objective of module-specific testing, it should be documented to prevent confusion.
Consider adding a comment explaining why verification is disabled in this case and what developers should expect.
190-191: Use project.file() for robust path constructionCurrent path construction using string concatenation could be made more robust using Gradle's path handling utilities.
- def modulePath = "$BasePath/$moduleName/**/*.class" - task.sourceDirectories.setFrom(project.files("src/main/java/$modulePath")) + def modulePath = project.file("$BasePath/$moduleName").path + "/**/*.class" + task.sourceDirectories.setFrom(project.files(project.file("src/main/java").path + "/$modulePath"))
1-92: Consider documenting module selection strategyWhile the implementation effectively supports selective module testing, it would be beneficial to document:
- How modules are identified from changed files
- The relationship between module paths and coverage thresholds
- The impact on the overall build process
This documentation would help future maintainers understand the design decisions and trade-offs.
Consider adding this documentation in a README file or as detailed comments in this configuration file.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
gradle/jacoco.gradle(1 hunks)
🔇 Additional comments (2)
gradle/jacoco.gradle (2)
82-84: LGTM: Dynamic module selection aligns with PR objectives
The dynamic module selection based on includedModules effectively supports the PR's goal of executing tests only for changed modules on non-default branches.
94-96: Verify Jacoco version compatibility
Let's verify if 0.8.12 is the latest stable version compatible with the project's Gradle version.
✅ Verification successful
JaCoCo version is up to date
The project is using JaCoCo version 0.8.12, which is confirmed to be the latest stable version available from Maven Central.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest Jacoco version from Maven Central
curl -s https://search.maven.org/solrsearch/select\?q\=g:%22org.jacoco%22+AND+a:%22org.jacoco.core%22\&rows\=1\&core\=gav | jq -r '.response.docs[0].v'
Length of output: 156
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
supporting_scripts/get_changed_modules.sh (1)
30-31: Handle special characters in module namesThe current IFS-based output might break with special characters in module names.
-IFS=, -echo "${CHANGED_MODULES[*]}" +# Join array elements with proper escaping +( IFS=,; echo "${CHANGED_MODULES[*]/#/\"}" | sed 's/,/\",/g; s/$/\"/' )gradle/jacoco.gradle (3)
170-170: 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 for violations for $moduleName"
178-184: Improve method documentationThe method documentation is incomplete and missing parameter descriptions.
/** * Prepares a Jacoco report task (report & verification) to match a specific + * module's requirements by setting up source and class directories. + * * @param task that is modified * @param moduleName of the module. * @param rootTask the JacocoReportBase root task + * @return void */
186-188: Use Gradle's build directory property consistentlyReplace hardcoded build directory path with Gradle's build directory property.
- task.executionData = project.fileTree("${project.layout.buildDirectory.get()}/jacoco") { + task.executionData = project.fileTree(project.layout.buildDirectory.file("jacoco").get()) { include "test.exec" }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
gradle/jacoco.gradle(1 hunks)supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py(1 hunks)supporting_scripts/get_changed_modules.sh(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py
11-11: f-string without any placeholders
Remove extraneous f prefix
(F541)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (1)
supporting_scripts/code-coverage/per_module_cov_report/parse_module_coverage.py (1)
32-35: 🛠️ Refactor suggestion
Fix None comparison and add constants
Use proper None comparison and define constants for counter types.
+INSTRUCTION_COUNTER = "INSTRUCTION"
+CLASS_COUNTER = "CLASS"
+
- instruction_counter = root.find("./counter[@type='INSTRUCTION']")
- class_counter = root.find("./counter[@type='CLASS']")
+ instruction_counter = root.find(f"./counter[@type='{INSTRUCTION_COUNTER}']")
+ class_counter = root.find(f"./counter[@type='{CLASS_COUNTER}']")
- if instruction_counter == None or class_counter == None:
+ if instruction_counter is None or class_counter is None:Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
35-35: Comparison to None should be cond is None
Replace with cond is None
(E711)
| MODULE_BASE_PATH="src/main/java/de/tum/cit/aet/artemis" | ||
| MODULE_BASE_TEST_PATH="src/test/java/de/tum/cit/aet/artemis" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate path existence before processing
The script assumes the existence of the module paths but should validate them first.
MODULE_BASE_PATH="src/main/java/de/tum/cit/aet/artemis"
MODULE_BASE_TEST_PATH="src/test/java/de/tum/cit/aet/artemis"
+
+# Validate paths exist
+if [ ! -d "$MODULE_BASE_PATH" ] || [ ! -d "$MODULE_BASE_TEST_PATH" ]; then
+ echo "Error: One or more required paths do not exist"
+ exit 1
+fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| MODULE_BASE_PATH="src/main/java/de/tum/cit/aet/artemis" | |
| MODULE_BASE_TEST_PATH="src/test/java/de/tum/cit/aet/artemis" | |
| MODULE_BASE_PATH="src/main/java/de/tum/cit/aet/artemis" | |
| MODULE_BASE_TEST_PATH="src/test/java/de/tum/cit/aet/artemis" | |
| # Validate paths exist | |
| if [ ! -d "$MODULE_BASE_PATH" ] || [ ! -d "$MODULE_BASE_TEST_PATH" ]; then | |
| echo "Error: One or more required paths do not exist" | |
| exit 1 | |
| fi |
| for MODULE in $MODULES; do | ||
| if git diff "$BRANCH_TO_COMPARE" --name-only | grep -q "^$MODULE_BASE_PATH/$MODULE/"; then | ||
| CHANGED_MODULES+=("$MODULE") | ||
| elif git diff "$BRANCH_TO_COMPARE" --name-only | grep -q "^$MODULE_BASE_TEST_PATH/$MODULE/"; then | ||
| CHANGED_MODULES+=("$MODULE") | ||
| fi | ||
| done |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for git operations
The git diff commands should include error handling to catch potential issues.
for MODULE in $MODULES; do
- if git diff "$BRANCH_TO_COMPARE" --name-only | grep -q "^$MODULE_BASE_PATH/$MODULE/"; then
+ if ! changes=$(git diff "$BRANCH_TO_COMPARE" --name-only 2>/dev/null); then
+ echo "Error: Failed to get changes from git"
+ exit 1
+ fi
+
+ if echo "$changes" | grep -q "^$MODULE_BASE_PATH/$MODULE/"; then
CHANGED_MODULES+=("$MODULE")
- elif git diff "$BRANCH_TO_COMPARE" --name-only | grep -q "^$MODULE_BASE_TEST_PATH/$MODULE/"; then
+ elif echo "$changes" | grep -q "^$MODULE_BASE_TEST_PATH/$MODULE/"; then
CHANGED_MODULES+=("$MODULE")
fi
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for MODULE in $MODULES; do | |
| if git diff "$BRANCH_TO_COMPARE" --name-only | grep -q "^$MODULE_BASE_PATH/$MODULE/"; then | |
| CHANGED_MODULES+=("$MODULE") | |
| elif git diff "$BRANCH_TO_COMPARE" --name-only | grep -q "^$MODULE_BASE_TEST_PATH/$MODULE/"; then | |
| CHANGED_MODULES+=("$MODULE") | |
| fi | |
| done | |
| for MODULE in $MODULES; do | |
| if ! changes=$(git diff "$BRANCH_TO_COMPARE" --name-only 2>/dev/null); then | |
| echo "Error: Failed to get changes from git" | |
| exit 1 | |
| fi | |
| if echo "$changes" | grep -q "^$MODULE_BASE_PATH/$MODULE/"; then | |
| CHANGED_MODULES+=("$MODULE") | |
| elif echo "$changes" | grep -q "^$MODULE_BASE_TEST_PATH/$MODULE/"; then | |
| CHANGED_MODULES+=("$MODULE") | |
| fi | |
| done |
| if [ $# -eq 0 ]; then | ||
| echo "Usage: $0 <branch_to_compare>" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance input validation and error handling
While the script checks for the presence of an argument, it should also validate that the specified branch exists.
if [ $# -eq 0 ]; then
echo "Usage: $0 <branch_to_compare>"
exit 1
fi
+
+# Validate that the branch exists
+if ! git rev-parse --verify "$BRANCH_TO_COMPARE" >/dev/null 2>&1; then
+ echo "Error: Branch '$BRANCH_TO_COMPARE' does not exist"
+ exit 1
+fiCommittable suggestion skipped: line range outside the PR's diff.
| def write_summary_to_file(coverage_by_module, output_file): | ||
| with open(output_file, "w") as f: | ||
| f.write("## Coverage Results\n\n") | ||
| f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n") | ||
| f.write("|-------------|---------------------------|----------------|\n") | ||
| for result in coverage_by_module: | ||
| f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations
The function should handle file operation errors gracefully.
-def write_summary_to_file(coverage_by_module, output_file):
+def write_summary_to_file(coverage_by_module: list[dict], output_file: str) -> None:
+ try:
with open(output_file, "w") as f:
f.write("## Coverage Results\n\n")
f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n")
f.write("|-------------|---------------------------|----------------|\n")
for result in coverage_by_module:
f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n")
+ except IOError as e:
+ raise RuntimeError(f"Failed to write coverage summary to {output_file}: {e}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def write_summary_to_file(coverage_by_module, output_file): | |
| with open(output_file, "w") as f: | |
| f.write("## Coverage Results\n\n") | |
| f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n") | |
| f.write("|-------------|---------------------------|----------------|\n") | |
| for result in coverage_by_module: | |
| f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n") | |
| def write_summary_to_file(coverage_by_module: list[dict], output_file: str) -> None: | |
| try: | |
| with open(output_file, "w") as f: | |
| f.write("## Coverage Results\n\n") | |
| f.write("| Module Name | Instruction Coverage (%) | Missed Classes |\n") | |
| f.write("|-------------|---------------------------|----------------|\n") | |
| for result in coverage_by_module: | |
| f.write(f"| {result['module']} | {result['instruction_coverage']:.2f} | {result['missed_classes']} |\n") | |
| except IOError as e: | |
| raise RuntimeError(f"Failed to write coverage summary to {output_file}: {e}") |
| def get_report_by_module(input_directory): | ||
| results = [] | ||
| for module_folder in os.listdir(input_directory): | ||
| module_path = os.path.join(input_directory, module_folder) | ||
|
|
||
| if os.path.isdir(module_path): | ||
| report_file = os.path.join(module_path, f"jacocoTestReport.xml") | ||
|
|
||
| if os.path.exists(report_file): | ||
| results.append({ | ||
| "module": module_folder, | ||
| "report_file": report_file | ||
| }) | ||
| else: | ||
| print(f"No XML report file found for module: {module_folder}. Skipping...") | ||
|
|
||
| return results | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type hints and input validation
The function should include type hints and validate the input directory.
-def get_report_by_module(input_directory):
+def get_report_by_module(input_directory: str) -> list[dict[str, str]]:
+ if not os.path.isdir(input_directory):
+ raise ValueError(f"Input directory does not exist: {input_directory}")
+
results = []
for module_folder in os.listdir(input_directory):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def get_report_by_module(input_directory): | |
| results = [] | |
| for module_folder in os.listdir(input_directory): | |
| module_path = os.path.join(input_directory, module_folder) | |
| if os.path.isdir(module_path): | |
| report_file = os.path.join(module_path, f"jacocoTestReport.xml") | |
| if os.path.exists(report_file): | |
| results.append({ | |
| "module": module_folder, | |
| "report_file": report_file | |
| }) | |
| else: | |
| print(f"No XML report file found for module: {module_folder}. Skipping...") | |
| return results | |
| def get_report_by_module(input_directory: str) -> list[dict[str, str]]: | |
| if not os.path.isdir(input_directory): | |
| raise ValueError(f"Input directory does not exist: {input_directory}") | |
| results = [] | |
| for module_folder in os.listdir(input_directory): | |
| module_path = os.path.join(input_directory, module_folder) | |
| if os.path.isdir(module_path): | |
| report_file = os.path.join(module_path, f"jacocoTestReport.xml") | |
| if os.path.exists(report_file): | |
| results.append({ | |
| "module": module_folder, | |
| "report_file": report_file | |
| }) | |
| else: | |
| print(f"No XML report file found for module: {module_folder}. Skipping...") | |
| return results |
🧰 Tools
🪛 Ruff (0.8.2)
11-11: f-string without any placeholders
Remove extraneous f prefix
(F541)
| def modulePath = "$BasePath/$moduleName/**/*.class" | ||
| task.sourceDirectories.setFrom(project.files("src/main/java/$modulePath")) |
There was a problem hiding this comment.
Prevent 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"))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def modulePath = "$BasePath/$moduleName/**/*.class" | |
| task.sourceDirectories.setFrom(project.files("src/main/java/$modulePath")) | |
| 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")) |
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.ymlReview Progress
Code Review
Manual Tests
Summary by CodeRabbit
New Features
Bug Fixes
Chores