Development: Define DTO architecture test per module with module-specific thresholds
#9959
Conversation
…of the module package
github-actions
bot
added
tests
assessment
|
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/athena/architecture/AthenaCodeStyleArchitectureTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/test/java/de/tum/cit/aet/artemis/atlas/architecture/AtlasCodeStyleArchitectureTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/test/java/de/tum/cit/aet/artemis/exam/architecture/ExamCodeStyleArchitectureTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
WalkthroughThis pull request introduces multiple new test classes for various modules within the Artemis application. Each new test class extends 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}
ole-ve
dismissed stale reviews from coderabbitai[bot] and coolchock
via
750a60e
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleCodeStyleTest.java (1)
18-32: 🛠️ Refactor suggestionConsider providing default implementations instead of abstract methods.
The current implementation requires each module to override these methods, leading to code duplication. Since the goal is to eventually reduce all thresholds to 0, consider:
- Making these concrete methods with default implementation of 0
- Allowing modules to override only when necessary during the transition period
- protected int dtoAsAnnotatedRecordThreshold() { - return 0; - } + protected int dtoAsAnnotatedRecordThreshold() { + return 0; // Override in specific modules if needed during transition + } - protected int dtoNameEndingThreshold() { - return 0; - } + protected int dtoNameEndingThreshold() { + return 0; // Override in specific modules if needed during transition + }
🧹 Nitpick comments (5)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleCodeStyleTest.java (3)
34-35: Add JavaDoc to document test method behavior.Add documentation to explain the test's purpose, rules being checked, and expected outcomes.
+ /** + * Tests the implementation of DTOs in the module for compliance with: + * 1. All DTOs should be records and annotated with @JsonInclude + * 2. All classes in DTO package should have names ending with "DTO" + */ @Test void testDTOImplementations() {
36-48: Improve test maintainability and readability.
- Consider using
lessThanOrEqualToinstead ofhasSizeto allow for improvements without requiring immediate threshold updates.- Move the inline comment to JavaDoc as suggested in previous reviews.
- Consider extracting the rules into separate methods for better readability.
- assertThat(result.getFailureReport().getDetails()).hasSize(dtoAsAnnotatedRecordThreshold()); + assertThat(result.getFailureReport().getDetails()).hasSizeLessThanOrEqualTo(dtoAsAnnotatedRecordThreshold()); - assertThat(result.getFailureReport().getDetails()).hasSize(dtoNameEndingThreshold()); + assertThat(result.getFailureReport().getDetails()).hasSizeLessThanOrEqualTo(dtoNameEndingThreshold());
50-52: Add JavaDoc to document helper method.Add documentation to explain the purpose and format of the returned package path.
+ /** + * Constructs the package path for DTO classes in the module. + * Uses '..' to match any number of subpackages. + * @return The package path pattern for finding DTOs + */ private String getModuleDtoSubpackage() {src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadCodeStyleArchitectureTest.java (1)
5-5: Consider making the test class publicWhile package-private visibility works, making the test class public would:
- Follow common JUnit test visibility patterns
- Allow potential reuse in other test scenarios
- Improve discoverability
-class FileUploadCodeStyleArchitectureTest extends AbstractModuleCodeStyleTest { +public class FileUploadCodeStyleArchitectureTest extends AbstractModuleCodeStyleTest {src/test/java/de/tum/cit/aet/artemis/quiz/architecture/QuizCodeStyleArchitectureTest.java (1)
5-5: Add JavaDoc documentation to describe the test's purpose.Since this is a test class enforcing architectural rules, it should be documented with JavaDoc explaining its specific role in enforcing DTO implementations for the quiz module.
+/** + * Architectural test class that enforces DTO implementation rules specific to the quiz module. + * This test ensures proper separation of DTOs and maintains consistent architectural patterns. + */ class QuizCodeStyleArchitectureTest extends AbstractModuleCodeStyleTest {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/test/java/de/tum/cit/aet/artemis/athena/architecture/AthenaCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/atlas/architecture/AtlasCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exam/architecture/ExamCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/architecture/LectureCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/lti/architecture/LtiCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/modeling/architecture/ModelingCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/plagiarism/architecture/PlagiarismCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/architecture/ProgrammingCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/quiz/architecture/QuizCodeStyleArchitectureTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleCodeStyleTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/tutorialgroup/architecture/TutorialGroupCodeStyleArchitectureTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/java/de/tum/cit/aet/artemis/lecture/architecture/LectureCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/atlas/architecture/AtlasCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/athena/architecture/AthenaCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/tutorialgroup/architecture/TutorialGroupCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/plagiarism/architecture/PlagiarismCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/programming/architecture/ProgrammingCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/lti/architecture/LtiCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/exam/architecture/ExamCodeStyleArchitectureTest.java
- src/test/java/de/tum/cit/aet/artemis/modeling/architecture/ModelingCodeStyleArchitectureTest.java
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/de/tum/cit/aet/artemis/quiz/architecture/QuizCodeStyleArchitectureTest.java (1)
Pattern 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/fileupload/architecture/FileUploadCodeStyleArchitectureTest.java (1)
Pattern 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/shared/architecture/module/AbstractModuleCodeStyleTest.java (1)
Pattern 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
🔇 Additional comments (7)
src/test/java/de/tum/cit/aet/artemis/shared/architecture/module/AbstractModuleCodeStyleTest.java (2)
1-14: LGTM: Class structure and imports are well-organized.
The class hierarchy and package structure are appropriate for the module-specific architecture testing purpose.
16-16: LGTM: Logger declaration follows best practices.
The logger is properly configured with appropriate access modifiers and naming.
src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadCodeStyleArchitectureTest.java (2)
1-4: LGTM: Package structure and imports are well-organized
The package name follows the module structure, and imports are minimal.
7-10: LGTM: Method implementation is correct
The getModulePackage implementation correctly constructs the module-specific package name.
Let's verify the ARTEMIS_PACKAGE constant usage across other module tests:
✅ Verification successful
Verified: Implementation follows consistent pattern across all module tests
The implementation in FileUploadCodeStyleArchitectureTest follows the same pattern used across all other module tests. Each module test consistently:
- Extends AbstractModuleCodeStyleTest
- Overrides getModulePackage()
- Returns ARTEMIS_PACKAGE concatenated with its specific module name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of ARTEMIS_PACKAGE constant in other module tests
# Expected: All module tests should use ARTEMIS_PACKAGE constant similarly
ast-grep --pattern 'class $_ extends AbstractModuleCodeStyleTest {
$$$
@Override
public String getModulePackage() {
return ARTEMIS_PACKAGE + $_
}
$$$
}'
Length of output: 292
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find all getModulePackage implementations
rg -U "class.*extends\s+AbstractModuleCodeStyleTest[\s\S]*?getModulePackage\(\)[\s\S]*?return.*ARTEMIS_PACKAGE.*;" -A 1
Length of output: 13125
src/test/java/de/tum/cit/aet/artemis/quiz/architecture/QuizCodeStyleArchitectureTest.java (3)
1-4: LGTM! Clean package structure and imports.
The package name correctly reflects the module structure, and imports are minimal.
7-10: Consider implementing this in the superclass.
As noted in a previous review, this implementation might not be needed if a default implementation was provided in the superclass. The package name could be derived from the class name or package automatically.
5-11: Consider adding module-specific threshold configuration.
The PR objectives mention allowing "module-specific violation thresholds", but this implementation doesn't specify any thresholds. Consider overriding additional methods to configure thresholds specific to the quiz module if needed.
Let's check if other modules implement custom thresholds:
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
# Conflicts: # src/test/java/de/tum/cit/aet/artemis/shared/architecture/AbstractModuleTestArchitectureTest.java
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
Checklist
General
Server
Motivation and Context
As part of restructuring the server code into modules, we temporarily disabled
ArchitectureTest#testDtoImplementationsbecause of too many violations. This PR enables it again.Note: The changed line count does not reflect the complexity of the changes - most of the code is simple boilerplate Java inheritance code.
Description
Moved the test to a common abstract architecture superclass
AbstractModuleCodeStyleTestand implement it in each module test directory. These specify the module-respective DTO-package names and violation thresholds.Review Progress
Code Review
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores