Skip to content
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: Reactivate auxiliary repository integration tests for LocalVC #9763

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ public abstract class AbstractProgrammingIntegrationLocalCILocalVCTest extends A
// External Repositories

// Services
@Autowired
protected ProgrammingExerciseIntegrationTestService programmingExerciseIntegrationTestService;

@Autowired
protected AeolusRequestMockProvider aeolusRequestMockProvider;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,18 @@ void testExportSubmissionsByParticipationIds_includePracticeSubmissions() throws
programmingExerciseIntegrationTestService.testExportSubmissionsByParticipationIds_includePracticeSubmissions();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testExportSubmissionsByParticipationIds_addParticipantIdentifierToProjectNameError() throws Exception {
programmingExerciseIntegrationTestService.testExportSubmissionsByParticipationIds_addParticipantIdentifierToProjectNameError();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testExportSubmissionsByParticipationIds_addParticipantIdentifierToProjectName() throws Exception {
programmingExerciseIntegrationTestService.testExportSubmissionsByParticipationIds_addParticipantIdentifierToProjectName();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testExportSubmissionsByParticipationIds() throws Exception {
Expand Down Expand Up @@ -562,6 +574,12 @@ void createProgrammingExercise_projectTypeNotExpected_badRequest() throws Except
programmingExerciseIntegrationTestService.createProgrammingExercise_projectTypeNotExpected_badRequest();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void createProgrammingExercise_onlineCodeEditorNotExpected_badRequest() throws Exception {
programmingExerciseIntegrationTestService.createProgrammingExercise_onlineCodeEditorNotExpected_badRequest();
}

private static Set<ProgrammingLanguage> generateSupportedLanguagesWithoutHaskell() {
Set<ProgrammingLanguage> supportedLanguages = ArgumentSources.generateJenkinsSupportedLanguages();
supportedLanguages.remove(ProgrammingLanguage.HASKELL);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.eq;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.timeout;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -292,10 +293,6 @@ void setup(String userPrefix, MockDelegate mockDelegate, VersionControlService v
GitService.commit(localGit).setMessage("empty").setAllowEmpty(true).setSign(false).setAuthor("test", "[email protected]").call();
localGit.push().call();

// we use the temp repository as remote origin for all repositories that are created during the
// TODO: distinguish between template, test and solution
doReturn(new GitUtilService.MockFileRepositoryUri(remoteRepoFile)).when(versionControlService).getCloneRepositoryUri(anyString(), anyString());

this.plagiarismChecksTestReposDir = Files.createTempDirectory("jplag-repos").toFile();
}

Expand Down Expand Up @@ -1433,6 +1430,8 @@ void importProgrammingExercise_vcsProjectWithSameTitleAlreadyExists_badRequest()
}

void importProgrammingExercise_updatesTestCaseIds() throws Exception {
doReturn(new GitUtilService.MockFileRepositoryUri(remoteRepoFile)).when(versionControlService).getCloneRepositoryUri(anyString(), anyString());

programmingExercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationAndAuxiliaryRepositoriesElseThrow(programmingExercise.getId());
var tests = programmingExerciseUtilService.addTestCasesToProgrammingExercise(programmingExercise);
var test1 = tests.getFirst();
Expand Down Expand Up @@ -1464,6 +1463,8 @@ void importProgrammingExercise_updatesTestCaseIds() throws Exception {
var savedProgrammingExercise = programmingExerciseRepository.findByIdElseThrow(response.getId());

assertThat(savedProgrammingExercise.getProblemStatement()).isEqualTo(newProblemStatement);

reset(versionControlService);
}

void exportSubmissionsByStudentLogins_notInstructorForExercise_forbidden() throws Exception {
Expand Down Expand Up @@ -1920,11 +1921,11 @@ void testGetPlagiarismResultWithoutExercise() throws Exception {

void testValidateValidAuxiliaryRepository() throws Exception {
AuxiliaryRepositoryBuilder auxRepoBuilder = AuxiliaryRepositoryBuilder.defaults();
testAuxRepo(auxRepoBuilder, HttpStatus.CREATED);
testAuxRepo(auxRepoBuilder, HttpStatus.OK);
}

void testValidateAuxiliaryRepositoryIdSetOnRequest() throws Exception {
testAuxRepo(AuxiliaryRepositoryBuilder.defaults().withId(0L), HttpStatus.BAD_REQUEST);
testAuxRepo(AuxiliaryRepositoryBuilder.defaults().withId(0L), HttpStatus.INTERNAL_SERVER_ERROR);

}

Expand Down Expand Up @@ -1953,12 +1954,12 @@ void testValidateAuxiliaryRepositoryWithInvalidCheckoutDirectory() throws Except

void testValidateAuxiliaryRepositoryWithoutCheckoutDirectory() throws Exception {
AuxiliaryRepositoryBuilder auxRepoBuilder = AuxiliaryRepositoryBuilder.defaults().withoutCheckoutDirectory();
testAuxRepo(auxRepoBuilder, HttpStatus.CREATED);
testAuxRepo(auxRepoBuilder, HttpStatus.OK);
}

void testValidateAuxiliaryRepositoryWithBlankCheckoutDirectory() throws Exception {
AuxiliaryRepositoryBuilder auxRepoBuilder = AuxiliaryRepositoryBuilder.defaults().withCheckoutDirectory(" ");
testAuxRepo(auxRepoBuilder, HttpStatus.CREATED);
testAuxRepo(auxRepoBuilder, HttpStatus.OK);
}

void testValidateAuxiliaryRepositoryWithTooLongCheckoutDirectory() throws Exception {
Expand All @@ -1981,7 +1982,7 @@ void testValidateAuxiliaryRepositoryWithTooLongDescription() throws Exception {

void testValidateAuxiliaryRepositoryWithoutDescription() throws Exception {
AuxiliaryRepositoryBuilder auxRepoBuilder = AuxiliaryRepositoryBuilder.defaults().withoutDescription();
testAuxRepo(auxRepoBuilder, HttpStatus.CREATED);
testAuxRepo(auxRepoBuilder, HttpStatus.OK);
}

void testGetAuxiliaryRepositoriesMissingExercise() throws Exception {
Expand Down Expand Up @@ -2158,7 +2159,7 @@ public void addAuxiliaryRepositoryToExercise(ProgrammingExercise exercise) {
}

private String defaultAuxiliaryRepositoryEndpoint() {
return "/api/programming-exercises/setup";
return "/api/programming-exercises";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

}

private String defaultResetEndpoint() {
Expand Down Expand Up @@ -2191,17 +2192,7 @@ private void testAuxRepo(AuxiliaryRepositoryBuilder body, HttpStatus expectedSta

private void testAuxRepo(List<AuxiliaryRepository> body, HttpStatus expectedStatus) throws Exception {
programmingExercise.setAuxiliaryRepositories(body);
programmingExercise.setId(null);
programmingExercise.setSolutionParticipation(null);
programmingExercise.setTemplateParticipation(null);
programmingExercise.setChannelName("pe-test");
programmingExercise.setShortName("ExerciseTitle");
programmingExercise.setTitle("Title");
if (expectedStatus == HttpStatus.CREATED) {
mockDelegate.mockConnectorRequestsForSetup(programmingExercise, false, false, false);
mockDelegate.mockGetProjectKeyFromAnyUrl(programmingExercise.getProjectKey());
}
request.postWithResponseBody(defaultAuxiliaryRepositoryEndpoint(), programmingExercise, ProgrammingExercise.class, expectedStatus);
request.putWithResponseBody(defaultAuxiliaryRepositoryEndpoint(), programmingExercise, ProgrammingExercise.class, expectedStatus);
}

private static class AuxiliaryRepositoryBuilder {
Expand Down Expand Up @@ -2275,8 +2266,8 @@ AuxiliaryRepository get() {
}
}

void testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden() throws Exception {
userUtilService.addInstructor("other-instructors", userPrefix + "instructoralt");
void testReEvaluateAndUpdateProgrammingExercise_instructorNotInCourse_forbidden(String testPrefix) throws Exception {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

userUtilService.addInstructor("other-instructors", testPrefix + "instructoralt");
programmingExerciseUtilService.addCourseWithOneProgrammingExercise();
ProgrammingExercise programmingExercise = programmingExerciseTestRepository.findAllWithEagerTemplateAndSolutionParticipations().getFirst();
request.put("/api/programming-exercises/" + programmingExercise.getId() + "/re-evaluate", programmingExercise, HttpStatus.FORBIDDEN);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
package de.tum.cit.aet.artemis.programming;

import java.io.IOException;

import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.security.test.context.support.WithMockUser;

class ProgrammingExerciseLocalVCIntegrationTest extends AbstractProgrammingIntegrationLocalCILocalVCTest {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add class-level JavaDoc and integration test annotation.

The class is missing required documentation and integration test setup:

  1. Add comprehensive JavaDoc describing the test class purpose and scope
  2. Add @SpringBootTest annotation 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.

Suggested change
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 {


private static final String TEST_PREFIX = "programmingexerciselocalvc";

@BeforeEach
void initTestCase() throws Exception {
programmingExerciseIntegrationTestService.setup(TEST_PREFIX, this, versionControlService, continuousIntegrationService);
}

@AfterEach
void tearDown() throws IOException {
programmingExerciseIntegrationTestService.tearDown();
}

protected String getTestPrefix() {
return TEST_PREFIX;
}

@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();
}

Comment on lines +28 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testGetAuxiliaryRepositoriesMissingExercise() throws Exception {
programmingExerciseIntegrationTestService.testGetAuxiliaryRepositoriesMissingExercise();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testGetAuxiliaryRepositoriesOk() throws Exception {
programmingExerciseIntegrationTestService.testGetAuxiliaryRepositoriesOk();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testGetAuxiliaryRepositoriesEmptyOk() throws Exception {
programmingExerciseIntegrationTestService.testGetAuxiliaryRepositoriesEmptyOk();
}

@Test
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
void testGetAuxiliaryRepositoriesForbidden() throws Exception {
programmingExerciseIntegrationTestService.testGetAuxiliaryRepositoriesForbidden();
}

@Test
@WithMockUser(username = TEST_PREFIX + "student1", roles = "USER")
void testExportAuxiliaryRepositoryForbidden() throws Exception {
programmingExerciseIntegrationTestService.testExportAuxiliaryRepositoryForbidden();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testExportAuxiliaryRepositoryBadRequest() throws Exception {
programmingExerciseIntegrationTestService.testExportAuxiliaryRepositoryBadRequest();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testExportAuxiliaryRepositoryExerciseNotFound() throws Exception {
programmingExerciseIntegrationTestService.testExportAuxiliaryRepositoryExerciseNotFound();
}

@Test
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testExportAuxiliaryRepositoryRepositoryNotFound() throws Exception {
programmingExerciseIntegrationTestService.testExportAuxiliaryRepositoryRepositoryNotFound();
}

@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();
}
//
Comment on lines +160 to +177
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve test method naming and add missing success scenario.

  1. Rename test methods to follow camelCase convention instead of using underscores
  2. 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.

// @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
}
Comment on lines +178 to +203
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Clean up commented code and clarify TODO.

  1. Either implement the commented-out tests or remove them
  2. 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?

Loading