Development: Refactor file upload tests
#9415
Conversation
…ore/restructure-tests
…ory and prevent unambiguous bean exception
github-actions
bot
added
tests
fileupload
…or-file-upload-tests
…or-file-upload-tests
…or-file-upload-tests
WalkthroughThe changes introduce a new base integration test class, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 4
🧹 Outside diff range comments (6)
src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadSubmissionIntegrationTest.java (1)
Line range hint
713-717: Use consistent indentation for code readabilityThe indentation in the
saveExercise_beforeDueDatetest method is inconsistent. Ensure to use 4 spaces for indentation to maintain code readability and adhere to the project's coding style guidelines.Apply this diff to correct the indentation:
@Test @WithMockUser(username = TEST_PREFIX + "student3", roles = "USER") void saveExercise_beforeDueDate() throws Exception { - FileUploadSubmission storedSubmission = request.postWithMultipartFile("/api/exercises/" + releasedFileUploadExercise.getId() + "/file-upload-submissions", + FileUploadSubmission storedSubmission = request.postWithMultipartFile("/api/exercises/" + releasedFileUploadExercise.getId() + "/file-upload-submissions", notSubmittedFileUploadSubmission, "submission", validFile, FileUploadSubmission.class, HttpStatus.OK); assertThat(storedSubmission.isSubmitted()).isTrue(); }src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadExerciseIntegrationTest.java (5)
Line range hint
67-73: Add assertions to validate the expected failureIn the method
createFileUploadExerciseFails(), there are no assertions to confirm that the API call fails as intended. Consider adding assertions to verify the response body or specific error messages to ensure the test accurately checks for the expectedHttpStatus.BAD_REQUEST.Apply this change to include assertions:
@Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void createFileUploadExerciseFails() throws Exception { String filePattern = "Example file pattern"; fileUploadExercise.setFilePattern(filePattern); - request.postWithResponseBody("/api/file-upload-exercises", fileUploadExercise, FileUploadExercise.class, HttpStatus.BAD_REQUEST); + var response = request.postWithResponseBody("/api/file-upload-exercises", fileUploadExercise, String.class, HttpStatus.BAD_REQUEST); + assertThat(response).contains("Specific error message or validation details"); }
Line range hint
74-80: Include assertions to confirm the expected error conditionIn
createFileUploadExerciseFailsIfAlreadyCreated(), the test lacks assertions to verify that the exercise creation fails because it already exists. Adding assertions to check the response content will help ensure the test accurately reflects the expected behavior.Consider applying this diff to add necessary assertions:
@Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void createFileUploadExerciseFailsIfAlreadyCreated() throws Exception { String filePattern = "Example file pattern"; fileUploadExercise.setFilePattern(filePattern); fileUploadExercise = fileUploadExerciseRepository.save(fileUploadExercise); - request.postWithResponseBody("/api/file-upload-exercises", fileUploadExercise, FileUploadExercise.class, HttpStatus.BAD_REQUEST); + var response = request.postWithResponseBody("/api/file-upload-exercises", fileUploadExercise, String.class, HttpStatus.BAD_REQUEST); + assertThat(response).contains("Exercise already exists error message"); }
Line range hint
133-137: Avoid direct database access in testsThe method
createFileUploadExerciseForExam()directly saves grading criteria to the repository:gradingCriteria = exerciseUtilService.addGradingInstructionsToExercise(fileUploadExercise); gradingCriterionRepository.saveAll(gradingCriteria);According to the coding guidelines, tests should avoid direct database access. Consider using mock objects or test utilities to manage test data without directly interacting with the database.
Refactor the code to use test utilities:
-gradingCriteria = exerciseUtilService.addGradingInstructionsToExercise(fileUploadExercise); -gradingCriterionRepository.saveAll(gradingCriteria); +gradingCriteria = gradingCriterionUtilService.addGradingInstructionsToExercise(fileUploadExercise);
Line range hint
217-221: Use static mocks for dependency injectionThe test utilizes Mockito for verifying interactions:
verify(examLiveEventsService, times(1)).createAndSendProblemStatementUpdateEvent(any(), any(), any()); verify(groupNotificationScheduleService, never()).checkAndCreateAppropriateNotificationsWhenUpdatingExercise(any(), any(), any());As per the coding guidelines, the mock strategy should use static mocks. Ensure that all mocks are properly declared as static and that the mocking strategy aligns with the project's conventions.
Line range hint
356-361: Minimize context restarts in parameterized testsThe parameterized test
testGetAllExercisesOnPageAsEditorSuccess()may cause unnecessary context restarts, impacting test performance. Consider restructuring the test to minimize context initialization by combining similar test cases or using different parameterization strategies.Refactor the test method to reduce context restarts:
-@Test -@WithMockUser(username = TEST_PREFIX + "editor1", roles = "EDITOR") +@ParameterizedTest +@WithMockUser(username = TEST_PREFIX + "editor1", roles = "EDITOR") void testGetAllExercisesOnPageAsEditorSuccess() throws Exception { // Test implementation }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadAssessmentIntegrationTest.java (2 hunks)
- src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadExerciseIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadSubmissionIntegrationTest.java (1 hunks)
- src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadTestArchitectureTest.java (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.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: truesrc/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadAssessmentIntegrationTest.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: truesrc/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadExerciseIntegrationTest.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: truesrc/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadSubmissionIntegrationTest.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: truesrc/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadTestArchitectureTest.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 (13)
src/test/java/de/tum/cit/aet/artemis/fileupload/architecture/FileUploadTestArchitectureTest.java (5)
1-5: LGTM: Package declaration and imports are correct.The package declaration matches the file path, and the imports are appropriate for the class implementation.
6-6: LGTM: Class declaration and structure are appropriate.The class name
FileUploadTestArchitectureTestis descriptive and follows the naming convention for test classes. It correctly extendsAbstractModuleTestArchitectureTestwith the appropriate type parameter.
8-11: LGTM:getModulePackage()method is correctly implemented.The method properly overrides the superclass method and returns the expected package name for the file upload module. It adheres to the small and specific test method guideline.
13-16: LGTM:getAbstractModuleIntegrationTestClass()method is correctly implemented.The method properly overrides the superclass method and returns the correct class object for
AbstractFileUploadIntegrationTest. It adheres to the small and specific test method guideline.
1-17: Overall, theFileUploadTestArchitectureTestclass is well-implemented and follows best practices.The class adheres to the provided coding guidelines:
- The test class name is descriptive.
- Methods are small and specific.
- It extends the appropriate abstract class for module testing.
- No database access or mocking is required, which is suitable for an architectural test.
The class structure and implementation provide a solid foundation for testing the architecture of the file upload module.
src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.java (2)
24-24: LGTM: Class structure and naming.The class name
AbstractFileUploadIntegrationTestis descriptive and follows the naming convention for abstract classes. ExtendingAbstractSpringIntegrationIndependentTestis a good practice for integration tests, providing a common base for file upload-related tests.
1-81: Overall assessment: Good foundation with room for improvement.The
AbstractFileUploadIntegrationTestclass provides a solid foundation for file upload integration tests. It follows proper naming conventions, extends an appropriate base class, and adheres to the basic structure expected for test classes.However, there are opportunities for improvement:
- Consider reducing the number of autowired dependencies to better adhere to the Single Responsibility Principle.
- Evaluate the possibility of using constructor injection instead of field injection.
- Add comments or documentation to guide developers on how to properly extend this class and adhere to all test-related coding guidelines.
These improvements will enhance the maintainability, readability, and overall quality of the test suite. Great job on establishing this base class for file upload tests!
src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadSubmissionIntegrationTest.java (4)
45-45: ExtendingAbstractFileUploadIntegrationTestfor improved test structureChanging the base class from
AbstractSpringIntegrationIndependentTesttoAbstractFileUploadIntegrationTestis appropriate. It helps in encapsulating common setup and utilities specific to file upload integration tests, promoting code reuse and maintainability.
Line range hint
400-405: Verify injection ofparticipationRepositoryafter removing@AutowiredIn the method
submitExercise_afterDueDate_forbidden(),participationRepositoryis used, but its@Autowiredfield declaration has been removed. Confirm thatparticipationRepositoryis correctly injected via the base class or another method to avoid potential runtime exceptions.Run the following script to check for the declaration of
participationRepository:#!/bin/bash # Description: Verify that `participationRepository` is declared in parent classes. # Search for the declaration of `participationRepository` in parent classes. rg --type java 'private ParticipationTestRepository participationRepository' src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.javaThis script will help you verify if
participationRepositoryis declared inAbstractFileUploadIntegrationTest.java, ensuring that the dependency is appropriately managed.
Line range hint
62-81: Ensure utility services are accessible after removing@AutowiredfieldsMethods like
initTestCase()use services such asuserUtilService,fileUploadExerciseUtilService, andparticipationUtilService, which were previously injected via@Autowired. Verify that these services are now available through the base class or other means to prevent anyNullPointerException.Use the following script to check the declarations:
#!/bin/bash # Description: Verify that utility services are declared or inherited. # Check for declarations in the base class. rg --type java 'protected UserUtilService userUtilService' src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.java rg --type java 'protected FileUploadExerciseUtilService fileUploadExerciseUtilService' src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.java rg --type java 'protected ParticipationUtilService participationUtilService' src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.javaThis will confirm if the utility services are declared in the base class and accessible in the subclass.
Line range hint
84-93: Verify injection offileUploadSubmissionRepositoryafter removing@AutowiredIn the
testRepositoryMethods()method,fileUploadSubmissionRepositoryis used, but its@Autowiredfield declaration has been removed. Ensure thatfileUploadSubmissionRepositoryis still properly injected, possibly through the new base class or another mechanism, to preventNullPointerExceptionat runtime.You can run the following script to check if
fileUploadSubmissionRepositoryis declared in the base classes:This script searches for the declaration of
fileUploadSubmissionRepositoryinAbstractFileUploadIntegrationTest.java. If the repository is declared there, it confirms that the dependency is properly injected through the base class.src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadExerciseIntegrationTest.java (2)
52-52: Ensure the new base class provides necessary setup and utilitiesThe test class now extends
AbstractFileUploadIntegrationTestinstead ofAbstractSpringIntegrationIndependentTest. Please verify that the new base class initializes all required dependencies and provides the necessary setup and utility methods that were previously available. This ensures that all tests function correctly with the updated inheritance.
Line range hint
400-406: Validate assertions for accurate test verificationIn the method
testGetFileUploadExercise_exampleSolutionVisibility(), ensure that the assertions accurately reflect the expected visibility of the example solution based on the user's role and theexampleSolutionPublicationDate.Review the assertions and confirm they align with the expected outcomes in different scenarios.
src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/fileupload/AbstractFileUploadIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadAssessmentIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/fileupload/FileUploadAssessmentIntegrationTest.java
Show resolved
Hide resolved
Checklist
General
Server
Motivation and Context
Description
Introduces
AbstractFileUploadIntegrationTest.Review Progress
Code Review
Test Coverage
unchanged
Summary by CodeRabbit
New Features
Refactor
Tests