-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development
: Refactor atlas tests
#9352
Conversation
…ore/restructure-tests
…ory and prevent unambiguous bean exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve refactored code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored code lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.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
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
Learnt from: julian-christl PR: ls1intum/Artemis#8052 File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310 Timestamp: 2024-02-23T00:03:06.365Z Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
🔇 Additional comments (1)
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java (1)
53-53
: Verify the impact of extending a different base class.Extending
AbstractAtlasIntegrationTest
instead ofAbstractSpringIntegrationIndependentTest
may introduce changes in the test setup, lifecycle, and available utilities. Carefully review the differences between these base classes and ensure that the existing test methods are still valid and compatible with the new base class.
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/atlas/learningpath/LearningPathIntegrationTest.java
Show resolved
Hide resolved
28b9ce4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.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 (1)
src/test/java/de/tum/cit/aet/artemis/atlas/AbstractAtlasIntegrationTest.java (1)
1-159
: Overall, the code changes look goodThe introduction of
AbstractAtlasIntegrationTest
consolidates the@Autowired
dependencies into a common abstract class for the Atlas integration tests. This promotes code reuse and simplifies the test classes that will extend this abstract class.
import de.tum.cit.aet.artemis.shared.base.AbstractSpringIntegrationIndependentTest; | ||
import de.tum.cit.aet.artemis.text.util.TextExerciseUtilService; | ||
|
||
public abstract class AbstractAtlasIntegrationTest extends AbstractSpringIntegrationIndependentTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Add JavaDoc comment to the class definition
Consider adding a JavaDoc comment to the AbstractAtlasIntegrationTest
class to describe its purpose and usage. This will enhance code readability and help other developers understand the intent of this abstract test class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve refactoring
Checklist
General
Server
Motivation and Context
Description
Introduces
AbstractAtlasIntegrationTest
and moves all@Autowire
into the abstract test class.Review Progress
Code Review
Test Coverage
unchanged
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor