Development: Reactivate auxiliary repository integration tests for LocalVC
#9763
Conversation
…ks auxiliary repo tests. Added mock to the _1_ test which actually needs it
github-actions
bot
added
tests
programming
WalkthroughThe changes in this pull request introduce enhancements to integration testing for programming exercises. A new field is added to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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. 🔧 pmdsrc/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.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/programming/ProgrammingExerciseIntegrationJenkinsGitlabTest.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/programming/ProgrammingExerciseIntegrationTestService.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.
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: 6
🧹 Outside diff range and nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java (1)
112-159: Improve test organization with JUnit 5 nested classes.Consider grouping related tests using
@Nestedclasses for better organization and readability.Example refactoring:
+ @Nested + @DisplayName("Auxiliary Repository Retrieval Tests") + class AuxiliaryRepositoryRetrievalTests { @Test @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") void testGetAuxiliaryRepositoriesMissingExercise() throws Exception { programmingExerciseIntegrationTestService.testGetAuxiliaryRepositoriesMissingExercise(); } // ... other retrieval tests + } + @Nested + @DisplayName("Auxiliary Repository Export Tests") + class AuxiliaryRepositoryExportTests { // ... export related tests + }src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (1)
1928-1928: Consider returningHttpStatus.BAD_REQUESTinstead ofHttpStatus.INTERNAL_SERVER_ERROR.When a client provides an invalid request, such as setting an ID on a new auxiliary repository, it's more appropriate to return
HttpStatus.BAD_REQUEST(400) rather thanHttpStatus.INTERNAL_SERVER_ERROR(500). This change will accurately reflect that the error was due to a bad request from the client side.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationJenkinsGitlabTest.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java(9 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCIntegrationTest.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.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/programming/ProgrammingExerciseIntegrationJenkinsGitlabTest.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/programming/ProgrammingExerciseIntegrationTestService.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/programming/ProgrammingExerciseLocalVCIntegrationTest.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/programming/ProgrammingExerciseIntegrationTestService.java (2)
Learnt from: Hialus
PR: ls1intum/Artemis#7451
File: src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java:9-9
Timestamp: 2024-11-12T12:51:51.200Z
Learning: The use of wildcard imports is acceptable in this project as specified in the `.editorconfig` file.
Learnt from: Hialus
PR: ls1intum/Artemis#7451
File: src/test/java/de/tum/in/www1/artemis/exercise/programmingexercise/ProgrammingExerciseTestService.java:9-9
Timestamp: 2024-11-12T12:51:58.050Z
Learning: The use of wildcard imports is acceptable in this project as specified in the `.editorconfig` file.
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java (1)
55-57: LGTM! Verify service usage in test implementations.
The addition of programmingExerciseIntegrationTestService follows the established patterns and is properly placed in the services section.
Let's verify that this service is actually used in the test implementations:
✅ Verification successful
Service usage correctly implemented in all relevant test classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of programmingExerciseIntegrationTestService in test classes
# Expected: Find references in concrete test classes that extend this abstract class
# Search for classes extending AbstractProgrammingIntegrationLocalCILocalVCTest
ast-grep --pattern 'class $_ extends AbstractProgrammingIntegrationLocalCILocalVCTest'
# Search for usage of programmingExerciseIntegrationTestService in test files
rg "programmingExerciseIntegrationTestService\." "src/test/java"
Length of output: 37684
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationJenkinsGitlabTest.java (3)
77-81: LGTM! Well-structured test method.
The test follows best practices with:
- Clear, descriptive name indicating the test scenario and expected outcome
- Proper security configuration with @WithMockUser
- Focused scope testing error handling
83-87: LGTM! Well-structured test method.
The test follows best practices with:
- Clear, descriptive name indicating the test scenario
- Proper security configuration with @WithMockUser
- Complements the error case test with success case coverage
577-581: LGTM! Well-structured test method.
The test follows best practices with:
- Clear, descriptive name indicating the test scenario and expected outcome
- Proper security configuration with @WithMockUser
- Focused scope testing invalid input handling
| // @Test | ||
| // @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| // void test_redirectGetSolutionRepositoryFilesWithoutContent() throws Exception { | ||
| // programmingExerciseIntegrationTestService.test_redirectGetSolutionRepositoryFilesWithoutContent(); | ||
| // } | ||
| // | ||
| // @Test | ||
| // @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| // void test_redirectGetTemplateRepositoryFilesWithContent() throws Exception { | ||
| // programmingExerciseIntegrationTestService.test_redirectGetTemplateRepositoryFilesWithContent(); | ||
| // } | ||
| // | ||
| // @Test | ||
| // @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| // void testRedirectGetParticipationRepositoryFilesWithContentAtCommit() throws Exception { | ||
| // programmingExerciseIntegrationTestService.testRedirectGetParticipationRepositoryFilesWithContentAtCommit(); | ||
| // } | ||
| // | ||
| // @Test | ||
| // @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| // void testRedirectGetParticipationRepositoryFilesWithContentAtCommitForbidden() throws Exception { | ||
| // programmingExerciseIntegrationTestService.testRedirectGetParticipationRepositoryFilesWithContentAtCommitForbidden(); | ||
| // } | ||
|
|
||
| // TODO add all other tests | ||
| } |
There was a problem hiding this comment.
Clean up commented code and clarify TODO.
- Either implement the commented-out tests or remove them
- Replace the vague TODO with specific test cases that need to be added
Would you like help implementing the commented-out tests or creating a detailed list of remaining test cases to be added?
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructoralt1", roles = "INSTRUCTOR") | ||
| void testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden() throws Exception { | ||
| programmingExerciseIntegrationTestService.testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden(TEST_PREFIX); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testReEvaluateAndUpdateProgrammingExercise_notFound() throws Exception { | ||
| programmingExerciseIntegrationTestService.testReEvaluateAndUpdateProgrammingExercise_notFound(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testReEvaluateAndUpdateProgrammingExercise_isNotSameGivenExerciseIdInRequestBody_conflict() throws Exception { | ||
| programmingExerciseIntegrationTestService.testReEvaluateAndUpdateProgrammingExercise_isNotSameGivenExerciseIdInRequestBody_conflict(); | ||
| } | ||
| // |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test method naming and add missing success scenario.
- Rename test methods to follow camelCase convention instead of using underscores
- Add test for successful re-evaluation scenario
Example refactoring:
- void testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden() throws Exception {
+ void testReEvaluateAndUpdateProgrammingExerciseWhenInstructorNotInCourseThenForbidden() throws Exception {
programmingExerciseIntegrationTestService.testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden(TEST_PREFIX);
}
+ @Test
+ @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
+ void testReEvaluateAndUpdateProgrammingExerciseSuccessful() throws Exception {
+ programmingExerciseIntegrationTestService.testReEvaluateAndUpdateProgrammingExerciseSuccessful();
+ }Committable suggestion skipped: line range outside the PR's diff.
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.security.test.context.support.WithMockUser; | ||
|
|
||
| class ProgrammingExerciseLocalVCIntegrationTest extends AbstractProgrammingIntegrationLocalCILocalVCTest { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add class-level JavaDoc and integration test annotation.
The class is missing required documentation and integration test setup:
- Add comprehensive JavaDoc describing the test class purpose and scope
- Add
@SpringBootTestannotation for proper Spring context initialization
Apply this diff:
+/**
+ * Integration tests for validating the LocalVC functionality with auxiliary repositories.
+ * These tests verify the behavior of repository operations, validations, and access controls
+ * in the context of programming exercises.
+ */
+@SpringBootTest
class ProgrammingExerciseLocalVCIntegrationTest extends AbstractProgrammingIntegrationLocalCILocalVCTest {📝 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.
| class ProgrammingExerciseLocalVCIntegrationTest extends AbstractProgrammingIntegrationLocalCILocalVCTest { | |
| /** | |
| * Integration tests for validating the LocalVC functionality with auxiliary repositories. | |
| * These tests verify the behavior of repository operations, validations, and access controls | |
| * in the context of programming exercises. | |
| */ | |
| @SpringBootTest | |
| class ProgrammingExerciseLocalVCIntegrationTest extends AbstractProgrammingIntegrationLocalCILocalVCTest { |
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateValidAuxiliaryRepository() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateValidAuxiliaryRepository(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryIdSetOnRequest() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryIdSetOnRequest(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithoutName() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithoutName(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithTooLongName() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithTooLongName(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithDuplicatedName() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithDuplicatedName(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithRestrictedName() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithRestrictedName(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithInvalidCheckoutDirectory() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithInvalidCheckoutDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithoutCheckoutDirectory() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithoutCheckoutDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithBlankCheckoutDirectory() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithBlankCheckoutDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithTooLongCheckoutDirectory() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithTooLongCheckoutDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithDuplicatedCheckoutDirectory() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithDuplicatedCheckoutDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithNullCheckoutDirectory() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithNullCheckoutDirectory(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithTooLongDescription() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithTooLongDescription(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") | ||
| void testValidateAuxiliaryRepositoryWithoutDescription() throws Exception { | ||
| programmingExerciseIntegrationTestService.testValidateAuxiliaryRepositoryWithoutDescription(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add database query count tracking for performance monitoring.
As per coding guidelines, integration tests should track database query performance. Add @Transactional and @SqlMeasure annotations to monitor database interactions.
Example for one test method:
@Test
+ @Transactional
+ @SqlMeasure
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testValidateValidAuxiliaryRepository() throws Exception {
programmingExerciseIntegrationTestService.testValidateValidAuxiliaryRepository();
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -2158,7 +2159,7 @@ public void addAuxiliaryRepositoryToExercise(ProgrammingExercise exercise) { | |||
| } | |||
|
|
|||
| private String defaultAuxiliaryRepositoryEndpoint() { | |||
| return "/api/programming-exercises/setup"; | |||
| return "/api/programming-exercises"; | |||
There was a problem hiding this comment.
Correct the endpoint returned by defaultAuxiliaryRepositoryEndpoint().
The method defaultAuxiliaryRepositoryEndpoint() currently returns "/api/programming-exercises", which does not point to the auxiliary repository endpoint. It should return the correct path, such as "/api/programming-exercises/" + exerciseId + "/auxiliary-repository", to accurately reflect its purpose.
| @@ -2275,8 +2266,8 @@ | |||
| } | |||
| } | |||
|
|
|||
| void testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden() throws Exception { | |||
| userUtilService.addInstructor("other-instructors", userPrefix + "instructoralt"); | |||
| void testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden(String testPrefix) throws Exception { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove or utilize the unused parameter testPrefix.
The parameter String testPrefix in testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden is not effectively used except for constructing a username. If it's unnecessary, consider removing it to simplify the method signature. If needed, ensure it's properly provided when the method is called.
krusche
changed the title
Develop: Re-add auxiliary repository integration tests for LocalVCDevelopment: Reactivate auxiliary repository integration tests for LocalVC
krusche
deleted the
chore/refactor/programming-exercise-integration-tests
branch
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
Description
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Documentation