Development: Improve Programming Exercise export tests
#11392
Conversation
…m:ls1intum/Artemis into chore/convert-athena-endpoints-to-filemap
…m:ls1intum/Artemis into chore/convert-athena-endpoints-to-filemap
…ices - Implemented checks in `AthenaFeedbackSuggestionsService` to return an empty list if the feedback suggestion module is not configured for an exercise. - Updated `AthenaModuleService` to throw an `IllegalArgumentException` if the exercise lacks a feedback suggestion module. - Enhanced `AthenaRepositoryExportService` to validate repository types and throw an exception for invalid types. - Modified `AthenaResource` to handle bad requests for invalid repository types. - Added integration tests to ensure proper handling of invalid repository types and feedback suggestion module absence.
…m:ls1intum/Artemis into chore/convert-athena-endpoints-to-filemap
github-actions
bot
added
tests
programming
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
…rogramming-exercises/improve-export-tests
…s/improve-export-tests
github-actions
bot
added
the
server
WalkthroughRemoves one public Athena endpoint and a deprecation annotation; adds null/path guards in core services; introduces RepositoryExportTestUtil and extensive test rewrites to seed and use LocalVC-backed repositories; converts several test spy beans to autowired, disables some tests, and deletes one unit test. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant LocalVC as LocalVCLocalCITestService
participant API as Application/API
participant Git as GitService
rect rgb(232,245,233)
Note left of Test: Test setup — seed & wire LocalVC repos
Test->>LocalVC: seed bare repo & create working copy (projectKey, slug)
LocalVC-->>Test: return repo URIs/paths
Test->>Git: write/commit/push files to working copy
Git-->>LocalVC: update bare repo HEAD
end
rect rgb(255,243,224)
Note left of Test: Runtime — API uses seeded URI for export/list
Test->>API: call export/list endpoint with seeded URI
API->>Git: read bare repo / getLastCommitHash(uri) / list files
Git-->>API: return commit hash and file contents
API-->>Test: return files or zip response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java (1)
266-283: Clarify deprecation status or add missing annotation.The Javadoc states "Deprecated, forwards to internal endpoint" but the method lacks the
@Deprecatedannotation, unlikegetSolutionRepository(line 297) andgetTestRepository(line 319) which both have it. This inconsistency may confuse API consumers.If this endpoint is no longer deprecated, update the Javadoc to remove the deprecation claim. If it is deprecated, add the
@Deprecatedannotation for consistency.Apply this diff if the endpoint should remain deprecated:
/** - * GET public/programming-exercises/:exerciseId/submissions/:submissionId/repository : Deprecated, forwards to internal endpoint + * GET public/programming-exercises/:exerciseId/submissions/:submissionId/repository : Get the submission repository as a zip file download * * @param exerciseId the id of the exercise the submission belongs to * @param submissionId the id of the submission to get the repository for * @param auth the auth header value to check * @param request the HTTP servlet request used for forwarding * @param response the HTTP servlet response used for forwarding + * @throws ServletException if the forward fails + * @throws IOException if the forward fails + * @deprecated Use {@code api/athena/internal/programming-exercises/{exerciseId}/submissions/{submissionId}/repository} instead */ + @Deprecated @GetMapping("public/programming-exercises/{exerciseId}/submissions/{submissionId}/repository")src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseTestCaseServiceTest.java (1)
37-47: Add teardown to clean up LocalRepository resources.The
createAndWireBaseRepositoriescall at line 41 creates filesystem resources (Git repositories and directories) that are never cleaned up. With 4 test methods in this class, each test run leaves 12 repository directories on disk, leading to disk space accumulation and potential test pollution across repeated runs.Add an
@AfterEachmethod to clean up the created repositories. Based on the PR objectives comment, theLocalRepositoryclass provides aresetLocalRepo()method for this purpose:+ @AfterEach + void tearDown() throws Exception { + // Clean up the LocalVC repositories created in setUp + localVCLocalCITestService.deleteLocalRepository(programmingExercise.getProjectKey(), programmingExercise.getProjectKey().toLowerCase() + "-exercise"); + localVCLocalCITestService.deleteLocalRepository(programmingExercise.getProjectKey(), programmingExercise.getProjectKey().toLowerCase() + "-solution"); + localVCLocalCITestService.deleteLocalRepository(programmingExercise.getProjectKey(), programmingExercise.getProjectKey().toLowerCase() + "-tests"); + }Note: Verify the exact cleanup method name in
LocalVCLocalCITestServiceorLocalRepositoryand adjust accordingly. The key is to ensure all created filesystem resources are properly released.src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCIIntegrationTest.java (1)
301-301: Missing cleanup for auxiliary repository.The
auxiliaryRepositoryinstance creates file-system resources (Git instances and directories) that are not cleaned up. Other tests in this file follow the pattern of callingresetLocalRepo()onLocalRepositoryinstances (e.g., lines 637, 729, 775, 959, 1014). Without cleanup, disk space will accumulate and cause test pollution across repeated runs.Add cleanup at the end of the test method:
await().until(() -> { Optional<BuildJob> buildJobOptional = buildJobRepository.findFirstByParticipationIdOrderByBuildStartDateDesc(templateParticipation.getId()); return buildJobOptional.isPresent() && buildJobOptional.get().getRepositoryType().equals(RepositoryType.TEMPLATE); }); + + // Cleanup + auxiliaryRepository.resetLocalRepo(); }Based on learnings
src/test/java/de/tum/cit/aet/artemis/exam/ExamStartTest.java (1)
119-131: Remove the conditional guard aroundprogrammingExerciseTestService.tearDown()to ensure comprehensive cleanup.The current code conditionally calls tearDown only if
exerciseRepo != null. However,programmingExerciseTestService.tearDown()already safely handles null repository fields (lines 371–400 show it checks each repo individually before callingresetLocalRepo()). This pattern is consistent across the codebase (seeRepositoryIntegrationTest.tearDown()andRepositoryExportTestUtil.resetRepos()).When the disabled tests (lines 183, 215) are re-enabled and create programming exercises/participations, repositories may be initialized in different orders or partially. The current guard might skip cleanup if
exerciseRepowas never set, leaving LocalRepository instances with unclosed Git handles and undelet directories.Change:
@AfterEach void tearDown() throws Exception { programmingExerciseTestService.tearDown(); // Always call; handles null fields internally // Cleanup of Bidirectional Relationships for (StudentExam studentExam : createdStudentExams) { exam.removeStudentExam(studentExam); } examRepository.save(exam); }src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java (1)
118-254: Disabled unit tests need explicit issue tracking for re-enablement.All 6 tests in
ParticipationServiceTest.javaare disabled (100% of this test file), representing a significant reduction in unit-level test coverage. While alternative integration tests likeParticipationIntegrationTest.javaprovide some functional coverage through HTTP endpoints (e.g.,participateInProgrammingExercise_successful,practiceProgrammingExercise_successful), unit tests serve a different purpose—directly testing service methods with focused mocks rather than full-stack HTTP testing.The consistent "Temporary: Programming participation creation with LocalVC needs initial repo setup" message indicates this is intentional during migration, but this requires explicit tracking in an issue to ensure re-enablement when LocalVC repository setup infrastructure is ready. Without tracking, these tests risk remaining permanently disabled.
Recommendation: Create an issue to track re-enabling these unit tests once LocalVC initialization is available, and reference that issue in the PR description or a code comment.
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationJenkinsLocalVCTest.java (1)
69-79: Add resource cleanup for real LocalVC and export services.Transitioning from
@MockitoSpyBeanto@Autowiredmeans these services now perform real file system operations, creating Git repositories and directories that persist after tests. Without cleanup, this leads to:
- Disk space accumulation across test runs
- Test pollution where files from one test affect others
- Potential CI/CD build issues
Based on the PR comment,
LocalRepositoryprovides aresetLocalRepo()method that closes Git instances and deletes directories.Add cleanup in the
@AfterEachmethod. For example:@AfterEach @Override protected void resetSpyBeans() { Mockito.reset(continuousIntegrationService); super.resetSpyBeans(); + // Clean up LocalVC file system resources + try { + versionControlService.cleanupRepositories(); + } catch (Exception e) { + // Log cleanup failure + } }Verify the correct cleanup method name for
LocalVCServiceand whetherGitRepositoryExportServicerequires separate cleanup:#!/bin/bash # Description: Find cleanup methods in LocalVCService and GitRepositoryExportService echo "=== LocalVCService cleanup methods ===" ast-grep --pattern $'class LocalVCService { $$$ cleanup$_($$$) { $$$ } $$$ }' echo "=== LocalVCService reset methods ===" ast-grep --pattern $'class LocalVCService { $$$ reset$_($$$) { $$$ } $$$ }' echo "=== GitRepositoryExportService cleanup methods ===" ast-grep --pattern $'class GitRepositoryExportService { $$$ cleanup$_($$$) { $$$ } $$$ }'src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java (1)
400-413: LocalRepositorysourceRepointestRepositoryExportEndpointis never reset (leaks temp repos across runs)In
testRepositoryExportEndpoint,sourceRepois created/configured but never cleaned up:var sourceRepo = new LocalRepository(defaultBranch); var originRepoPath = tempPath.resolve("testOriginRepo"); sourceRepo.configureRepos(localVCBasePath, "athenaSrcLocalRepo", "athenaSrcOriginRepo"); // ... // no resetLocalRepo() or RepositoryExportTestUtil.resetRepos(...) callThis leaves both the working copy and bare repo directories on disk after the test completes. Over many runs (e.g. CI loops, local iterations), this can accumulate unnecessary files and is the same pattern highlighted in the PR description about LocalRepository cleanup.
A minimal fix is to reset the repo at the end of the test, e.g.:
- void testRepositoryExportEndpoint(String urlSuffix) throws Exception { + void testRepositoryExportEndpoint(String urlSuffix) throws Exception { // ... - gitService.copyBareRepositoryWithoutHistory(sourceUri, testsRepoUri, defaultBranch); - - // Get repository contents as map from endpoint - var authHeaders = new HttpHeaders(); - authHeaders.add(HttpHeaders.AUTHORIZATION, athenaSecret); - - String json = request.get("/api/athena/internal/programming-exercises/" + programmingExercise.getId() + "/" + urlSuffix, HttpStatus.OK, String.class, authHeaders); - Map<String, String> repoFiles = request.getObjectMapper().readValue(json, new TypeReference<Map<String, String>>() { - }); - assertThat(repoFiles).as("export returns exactly one file: README.md").isNotNull().hasSize(1).containsOnlyKeys("README.md").containsEntry("README.md", "Initial commit"); + gitService.copyBareRepositoryWithoutHistory(sourceUri, testsRepoUri, defaultBranch); + + try { + var authHeaders = new HttpHeaders(); + authHeaders.add(HttpHeaders.AUTHORIZATION, athenaSecret); + + String json = request.get("/api/athena/internal/programming-exercises/" + programmingExercise.getId() + "/" + urlSuffix, HttpStatus.OK, String.class, authHeaders); + Map<String, String> repoFiles = request.getObjectMapper().readValue(json, new TypeReference<Map<String, String>>() { + }); + assertThat(repoFiles).as("export returns exactly one file: README.md").isNotNull().hasSize(1).containsOnlyKeys("README.md").containsEntry("README.md", "Initial commit"); + } + finally { + sourceRepo.resetLocalRepo(); + } }Alternatively, you could use
RepositoryExportTestUtil.resetRepos(sourceRepo)for consistency with other tests.src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (6)
298-305: Critical resource leak: LocalRepository instances not cleaned up in tearDown.The
tearDown()method does not clean up thestudentRepository1andstudentRepository2LocalRepository instances created insetup()(lines 279-283). Additionally, many test methods now create LocalRepository instances viaRepositoryExportTestUtilhelper methods (e.g., lines 353-354, 390-391, 482-484) that are never cleaned up.LocalRepository instances create file-system resources (Git repositories, working copies) that must be explicitly released via
resetLocalRepo()to avoid disk space accumulation and test pollution.Apply this diff to fix the tearDown:
void tearDown() throws IOException { + RepositoryExportTestUtil.resetRepos(studentRepository1, studentRepository2); if (downloadedFile != null && downloadedFile.exists()) { FileUtils.forceDelete(downloadedFile); } if (plagiarismChecksTestReposDir != null && plagiarismChecksTestReposDir.exists()) { FileUtils.deleteDirectory(plagiarismChecksTestReposDir); } }Note: This only addresses the field-level repositories. Individual test methods that create LocalRepository instances should also capture and clean them up. Consider wrapping LocalRepository creation in try-finally blocks or using
@AfterEachcleanup for test-scoped repositories.Based on learnings and PR objectives comment.
479-499: Resource leak: LocalRepository instances not cleaned up.Lines 482-484 create LocalRepository instances but don't capture or clean them. This pattern appears throughout the file and should be addressed systematically.
Consider a test-scoped cleanup approach for methods that create multiple repositories:
void testExportSubmissionsByParticipationIds() throws Exception { - // Seed LocalVC student repositories and wire URIs - // use shared util to seed and wire repos for both participations - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); + // Seed LocalVC student repositories and wire URIs + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); + + try { + // ... rest of test method + } finally { + RepositoryExportTestUtil.resetRepos(repo1, repo2); + } }Based on PR objectives comment.
501-538: Resource leak: Multiple LocalRepository instances not cleaned up.This method creates several LocalRepository instances:
- Line 503: Base repositories (template, solution, tests) via
createAndWireBaseRepositories- Line 509:
templateRepocaptured but never cleaned- Line 513:
studentRepocaptured but never cleanedAll of these create file-system resources that persist after the test completes.
Consider wrapping repository operations in try-finally:
// Rest call with options (combine + anonymize enabled in getOptions()) final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + participation1.getId(); + try { downloadedFile = request.postWithResponseBodyFile(path, getOptions(), HttpStatus.OK); assertThat(downloadedFile).exists(); List<Path> entries = unzipExportedFile(); - // Checks: file present and single anonymized commit - assertThat(entries).anyMatch(entry -> entry.endsWith("Test.java")); - Optional<Path> extractedRepo1 = entries.stream() - .filter(entry -> entry.toString().endsWith(Path.of("-" + participation1.getId() + "-student-submission.git", ".git").toString())).findFirst(); - assertThat(extractedRepo1).isPresent(); - try (Git downloadedGit = Git.open(extractedRepo1.get().toFile())) { - RevCommit commit = downloadedGit.log().setMaxCount(1).call().iterator().next(); - assertThat(commit.getAuthorIdent().getName()).isEqualTo("student"); - assertThat(commit.getFullMessage()).isEqualTo("All student changes in one commit"); + // Checks: file present and single anonymized commit + assertThat(entries).anyMatch(entry -> entry.endsWith("Test.java")); + Optional<Path> extractedRepo1 = entries.stream() + .filter(entry -> entry.toString().endsWith(Path.of("-" + participation1.getId() + "-student-submission.git", ".git").toString())).findFirst(); + assertThat(extractedRepo1).isPresent(); + try (Git downloadedGit = Git.open(extractedRepo1.get().toFile())) { + RevCommit commit = downloadedGit.log().setMaxCount(1).call().iterator().next(); + assertThat(commit.getAuthorIdent().getName()).isEqualTo("student"); + assertThat(commit.getFullMessage()).isEqualTo("All student changes in one commit"); + } + } finally { + RepositoryExportTestUtil.resetRepos(templateRepo, studentRepo); } }Based on PR objectives comment.
572-580: Resource leak: LocalRepository instances not cleaned up.Lines 574-576 create LocalRepository instances that are never captured or cleaned.
Same cleanup pattern applies:
private File exportSubmissionsByStudentLogins() throws Exception { - // Seed LocalVC student repositories and wire URIs - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); + // Seed LocalVC student repositories and wire URIs + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); + + try { final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participant-identifiers/" + userPrefix + "student1," + userPrefix + "student2"; return request.postWithResponseBodyFile(path, getOptions(), HttpStatus.OK); + } finally { + RepositoryExportTestUtil.resetRepos(repo1, repo2); + } }Based on PR objectives comment.
781-799: Resource leak but good logic: testGenerateStructureOracle refactored well.The LocalVC integration looks correct—the method now:
- Wires base repositories (line 783)
- Creates a tests repository (line 789)
- Seeds the expected directory structure based on build config (lines 791-794)
However,
testsRepo(line 789) is captured but never cleaned up, causing a resource leak.Add cleanup:
RepositoryExportTestUtil.writeFilesAndPush(testsRepo, Map.of(testsPath + "/.placeholder", ""), "Init tests dir"); final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/generate-tests"; - var result = request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.OK); - assertThat(result).startsWith("Successfully generated the structure oracle"); - request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.BAD_REQUEST); + try { + var result = request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.OK); + assertThat(result).startsWith("Successfully generated the structure oracle"); + request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.BAD_REQUEST); + } finally { + RepositoryExportTestUtil.resetRepos(testsRepo); + } }Based on PR objectives comment.
2247-2271: Resource leak in helper: setupExerciseForExport creates uncleaned repositories.Line 2268 calls
createAndWireBaseRepositories, which creates template, solution, and tests LocalRepository instances that are never captured or cleaned.This helper is called by multiple tests (e.g., lines 2219, 2229), so the leak affects multiple test methods. Consider either:
- Returning the created repositories for caller-side cleanup:
-private void setupExerciseForExport() throws IOException, GitAPIException, URISyntaxException, Exception { +private LocalRepository[] setupExerciseForExport() throws IOException, GitAPIException, URISyntaxException, Exception { // ... existing setup code ... // Create and wire base repos via shared util to reduce duplication RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, programmingExercise); programmingExercise = programmingExerciseRepository.save(programmingExercise); + + // Return repo handles for cleanup (caller must retrieve from LocalVC) + // Alternatively, store in test-scoped fields for @AfterEach cleanup }
- Storing repositories in test-scoped fields for centralized cleanup in
@AfterEachortearDown().Note: The extended exception clause (line 2247) is appropriate given the new LocalVC operations.
Based on PR objectives comment.
♻️ Duplicate comments (2)
src/test/java/de/tum/cit/aet/artemis/exam/ExamStartTest.java (1)
215-215: Same tracking verification needed.This parameterized test has the same temporary disable status as the test at line 183. Ensure both are covered by the same tracking mechanism for re-enablement.
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCService.java (1)
158-161: Exists-early return may mask wrong default branch; add safe idempotent HEAD fix + path normalization verification.If the repo dir exists, we skip init and never ensure HEAD → defaultBranch. Old repos (e.g., master) will drift. Also, per CodeQL, harden path derivation against traversal.
Suggested change:
- When
Files.exists(remoteDirPath), open the bare repo and re-link HEAD todefaultBranchif mismatched.- Ensure
LocalVCRepositoryUri.getLocalRepositoryPath(...)enforcesresolved.startsWith(localVCBasePath.normalize())likeGitService.getLocalPathOfRepo(...). If not, add a check here before use.Example patch:
@@ public void createRepository(String projectKey, String repositorySlug) { - if (Files.exists(remoteDirPath)) { - log.debug("Local git repo {} at {} already exists – skipping creation", repositorySlug, remoteDirPath); - return; - } + if (Files.exists(remoteDirPath)) { + try (Git git = Git.open(remoteDirPath.toFile())) { + var repo = git.getRepository(); + var head = repo.getRefDatabase().findRef(Constants.HEAD); + var expected = "refs/heads/" + defaultBranch; + if (head == null || head.isSymbolic() && !expected.equals(head.getTarget().getName())) { + RefUpdate u = repo.getRefDatabase().newUpdate(Constants.HEAD, false); + u.setForceUpdate(true); + u.link(expected); + log.debug("Adjusted HEAD of existing repo {} at {} to {}", repositorySlug, remoteDirPath, expected); + } + } catch (IOException e) { + log.warn("Repo {} exists at {}, but could not verify HEAD: {}", repositorySlug, remoteDirPath, e.getMessage()); + } + log.debug("Local git repo {} at {} already exists – skipping creation", repositorySlug, remoteDirPath); + return; + }To confirm path safety in LocalVCRepositoryUri, please run:
#!/bin/bash # Inspect LocalVCRepositoryUri.getLocalRepositoryPath for normalization and base-path guard rg -nP --type=java -C3 'class\s+LocalVCRepositoryUri\b|getLocalRepositoryPath\s*\(' rg -nP --type=java -C2 'normalize\(\)|startsWith\(' src/main/java | sed -n '1,200p' # Compare with GitService.getLocalPathOfRepo implementation for parity rg -nP -C5 'Path\s+getLocalPathOfRepo\(' src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java
🧹 Nitpick comments (21)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java (1)
504-505: Good refactoring from static mocks to instance mocks.The change correctly replaces static mocking of
Files.createTempDirectorywith mockingzipFileService.createTemporaryZipFile, which is a cleaner approach. The parameter reduction in the helper method call aligns with the updated method signature.Consider renaming the test method to better reflect what's actually being tested:
- void testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir() throws Exception { + void testExportProgrammingExerciseInstructorMaterial_failToCreateZipFile() throws Exception {src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java (1)
101-102: Address or remove the TODO comment.The TODO questioning the necessity of
MockitoAnnotations.openMocks(this)should be resolved, especially since the tests are being migrated from mocking to real LocalVC repositories. If@Mockannotations aren't used on fields in this class, this line (and the associatedcloseablefield and teardown logic) might be unnecessary.Do you want me to verify whether
@Mockannotations are used in this test class or open an issue to track this cleanup?src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingUtilTestService.java (1)
92-96: Prefer OS‑independent path assembly.Replace string concatenation with
Path.resolve(...)to follow file_handling:os_indep_paths.-Path filePath = Path.of(templateRepo.workingCopyGitRepoFile + "/" + fileName); +Path filePath = templateRepo.workingCopyGitRepoFile.toPath().resolve(fileName); @@ -Path filePath = Path.of(solutionRepo.workingCopyGitRepoFile + "/" + fileName); +Path filePath = solutionRepo.workingCopyGitRepoFile.toPath().resolve(fileName); @@ -Path filePath = Path.of(participationRepo.workingCopyGitRepoFile + "/" + fileName); +Path filePath = participationRepo.workingCopyGitRepoFile.toPath().resolve(fileName);As per coding guidelines.
Also applies to: 128-133, 156-160
src/test/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingResourceExportTest.java (1)
78-81: Delete LocalVC project folder in @AfterEach to avoid FS leaks.
createAndWireBaseRepositories(...)creates three bare repos under LocalVC base path. Add teardown to delete<local-vcs-repo-path>/<PROJECT_KEY>.+import java.nio.file.Path; +import org.apache.commons.io.FileUtils; +import org.springframework.beans.factory.annotation.Value; @@ class ExerciseSharingResourceExportTest extends AbstractProgrammingIntegrationLocalCILocalVCTest { @@ + @Value("${artemis.version-control.local-vcs-repo-path}") + private Path localVCBasePath; @@ @AfterEach void tearDown() throws Exception { sharingPlatformMockProvider.reset(); + try { + FileUtils.deleteDirectory(localVCBasePath.resolve(programmingExercise1.getProjectKey()).toFile()); + } catch (Exception ignored) { } }src/test/java/de/tum/cit/aet/artemis/programming/StaticCodeAnalysisIntegrationTest.java (1)
47-52: Add teardown to remove LocalVC repos created for SCA tests.Mirror cleanup to match new base-repo wiring.
+import java.nio.file.Path; +import org.apache.commons.io.FileUtils; +import org.springframework.beans.factory.annotation.Value; @@ class StaticCodeAnalysisIntegrationTest extends AbstractProgrammingIntegrationLocalCILocalVCTest { @@ + @Value("${artemis.version-control.local-vcs-repo-path}") + private Path localVCBasePath; @@ + @org.junit.jupiter.api.AfterEach + void cleanupLocalVC() { + try { + if (programmingExerciseSCAEnabled != null) { + FileUtils.deleteDirectory(localVCBasePath.resolve(programmingExerciseSCAEnabled.getProjectKey()).toFile()); + } + } catch (Exception ignored) { } + }src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java (1)
6-8: LocalVC commit helpers are well-structured; minor reuse opportunityThe new helper stack (
commitToParticipationRepository,commitToRepositoryoverloads,ensureLocalVcRepositoryExists, andwriteFilesAndPush) correctly:
- Guard against
nullrepository URIs with a clearIllegalStateException.- Normalize to
LocalVCRepositoryUriregardless of whether the caller passes aString,VcsRepositoryUri, orLocalVCRepositoryUri.- Use
LocalVCRepositoryUri.getLocalRepositoryPath(localVCBasePath)to locate the bare repo and lazily create it vialocalVCLocalCITestService.createAndConfigureLocalRepository(...).- Clone into a temp working copy under
tempPath, write files, commit viaGitService.commit(git), push, and delete the working copy in afinallyblock usingFileUtils.deleteDirectory, avoiding accumulation of clones.This is a solid abstraction for seeding real commits in LocalVC-backed tests.
Given you already have
RepositoryExportTestUtil.writeFilesAndPush(LocalRepository, ...), consider (optionally) reusing it where you can obtain aLocalRepositoryhandle, to avoid duplicating commit/push logic and centralize cleanup behavior. Not strictly necessary, but it would reduce divergence between test helpers over time.Also applies to: 19-23, 52-53, 59-65, 1110-1161
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCExportsIntegrationTest.java (2)
50-62: Verify cleanup of LocalVC repositories created in setupIn
setup()you callRepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, p1/p2), which createsLocalRepositoryinstances (working copies + bare repos) but you don’t keep references or callresetLocalRepo()/RepositoryExportTestUtil.resetRepos(...)explicitly.If
programmingExerciseIntegrationTestService.tearDown()does not already close and delete those LocalRepositories (including their working copies and bare dirs under the LocalVC base path), repeated test runs could gradually accumulate file-system state.I’d recommend either:
- Storing the returned
LocalRepositoryinstances in fields and invokingRepositoryExportTestUtil.resetRepos(...)in@AfterEach, or- Confirming that
tearDown()covers this cleanup path and adding a short comment to that effect if so.Also applies to: 64-67
69-89: Consider deleting the exported ZIP file after assertionsIn
exportByParticipationIds_happyPathyou:
- Export to a
File.- Extract it to a temporary directory and assert presence of both student repos and
.gitdirs.- Clean up the extracted directory via
RepositoryExportTestUtil.deleteDirectoryIfExists(extracted).The zip file itself is currently left on disk. To avoid leaving behind temp artifacts across repeated runs, you could also delete the export file in the
finallyblock using the existing helper:- finally { - // cleanup extracted folder - RepositoryExportTestUtil.deleteDirectoryIfExists(extracted); - } + finally { + // cleanup extracted folder and exported ZIP + RepositoryExportTestUtil.deleteDirectoryIfExists(extracted); + RepositoryExportTestUtil.deleteFileSilently(file); + }This keeps the test self-contained and avoids unnecessary disk usage in CI.
src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java (1)
1508-1523: Temp-clone commit helper correctly targets LocalVC and cleans up
commitNewFileToParticipationReposensibly derives the bare repository path viaLocalVCRepositoryUriandlocalVCBasePath, clones via file://, writes a unique file, commits with the existingGitService.commithelper, pushes toorigin, and deletes the temp clone directory in afinallyblock. Its usages intestSubmitStudentExam_realisticandaddExamExerciseSubmissionsForUsermatch this contract. If you ever see flakiness from delete failures, you could switch toFileUtils.deleteQuietlyto keep cleanup best-effort, but it’s not strictly necessary.Also applies to: 1929-1936
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (2)
169-184: Setup/teardown now enforce a clean LocalVC project; consider shared-path implicationsCalling
deleteExistingProject(projectKey)insetup(), resetting all LocalRepository instances intearDown(), and deletinglocal/server-integration-test/reposandrepos-downloadgives each test a fresh LocalVC state and addresses disk-leak concerns. If other test classes share the same LocalVC base path concurrently, deleting the whole tree could interfere with them; if that ever becomes an issue, consider isolating the base path per test class or guarding it with a@ResourceLock.Also applies to: 200-216, 1218-1231
1150-1217: Multi-repo initialization/seeding is solid; consider deriving the student slug via a shared helper
initializeInstructorRepositoriesandinitializeStudentParticipationcorrectly add template/solution participations, create and seed template/solution/tests/student repositories viaLocalVCLocalCITestService, and update the corresponding URIs on the exercise/participations. This gives all repos consistent default content and aligns URIs with LocalVC. The only coupling concern is the manual student slug constructionstudentSlug = (programmingExercise.getProjectKey() + "-" + studentLogin).toLowerCase();; if a central helper already exists for student repository names (e.g. in a participation/util service), reusing it would future-proof these tests against changes in naming conventions.src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.java (1)
53-54: LocalVC repo seeding and cleanup look correct; consider null-guard in tearDown
- Using
RepositoryType.TESTS+RepositoryExportTestUtil.seedBareRepository+wireRepositoryToExerciseis consistent with the LocalVC helpers and keeps the tests realistic.- Committing
.keepin the folder to ensure Git tracks it is a good touch.@AfterEachnow callstestRepo.resetLocalRepo(), which addresses the LocalRepository filesystem cleanup concern raised in the PR description.To be slightly more defensive against setup failures, you might guard
testRepointearDown()(mirroringAuxiliaryRepositoryResourceIntegrationTest):- void tearDown() throws IOException { - testRepo.resetLocalRepo(); - } + void tearDown() throws IOException { + if (testRepo != null) { + testRepo.resetLocalRepo(); + } + }Also applies to: 62-65, 72-77, 78-84, 86-89
src/test/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseResourceTest.java (1)
38-41: Good reuse of export helpers; optionally capture seeded LocalVC repo handle
- Using
ProgrammingExerciseTestService.setupExerciseForExport(...)in the student-requested export tests andRepositoryExportTestUtil.seedStudentRepositoryForParticipation(...)intestExportOwnStudentRepositorynicely centralizes the LocalVC wiring and keeps the tests short and focused.- In
setupLocalVCRepository, deriving the slug usingRepositoryType.TEMPLATE.getName()and seeding the LocalVC bare repo viaRepositoryExportTestUtil.seedLocalVcBareFrom(...), then wiring the URI vialocalVCLocalCITestService.buildLocalVCUri(...), is consistent with the LocalVC test utilities (Based on learnings).If you want the code to be slightly clearer, you could assign the result of
seedLocalVcBareFromto a local variable (even if it’s not used later), documenting that this is the LocalVC-backed repo instance for the controller:- RepositoryExportTestUtil.seedLocalVcBareFrom(localVCLocalCITestService, projectKey, templateRepositorySlug, localRepo); + var localVcTemplateRepo = RepositoryExportTestUtil.seedLocalVcBareFrom(localVCLocalCITestService, projectKey, templateRepositorySlug, localRepo);Purely optional, as behavior is already correct.
Also applies to: 82-83, 198-199, 217-218, 236-239, 357-368
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (5)
282-292: LocalVC wiring fields and repository metadata map look consistentMaking
continuousIntegrationServiceoptional and wiringLocalVCLocalCITestServiceplus the two participation repositories aligns this test helper with LocalVC setups. TheIdentityHashMap<LocalRepository, RepositoryMetadata>that’s cleared insetupandtearDownscopes metadata cleanly to this service instance and avoids accidental cross-repository collisions. No functional issues spotted; just a minor nit thatprojectKeyuppercasing is computed both in callers and insideconfigureLocalRepositoryForSlug, which could be simplified later.Also applies to: 334-347
450-468: LocalVC project pre-cleanup reduces cross-test interferenceThe
deleteLocalVcProjectIfPresenthelper, guarded againstnullinputs and a missing directory, and its use at the beginning of the variousimportFromFile_*andcreateProgrammingExercise_failToCreateProjectInCihelpers, should help avoid stale LocalVC project state between tests. LeveragingversionControlService.deleteProjectinstead of ad-hoc filesystem deletes is a good move for keeping LocalVC internals consistent. No issues beyond potential future extension to other LocalVC-heavy helpers if flakiness appears.Also applies to: 646-648, 683-685, 706-707, 732-734, 746-747, 754-755, 762-773, 779-780, 823-826
586-633:setupExerciseForExportis a solid central export-preparation helperThe new
setupExerciseForExportmethod correctly:
- injects a realistic problem statement with embedded resource references,
- ensures template and solution participations exist (creating them if needed),
- wires LocalVC URIs for template, solution, and tests repositories, and
- calls
createAndConfigureLocalRepositoryto guarantee repos exist at the LocalVC level.Re-fetching the exercise with template and solution participations before wiring URIs is also appropriate. Behavior looks correct for repeated use on the same exercise, assuming
LocalVCLocalCITestService.createAndConfigureLocalRepositoryis idempotent for an existing project.
1802-1830: Refactored instructor-material export helper improves reuse and controlThe new overload of
exportProgrammingExerciseInstructorMaterialencapsulates:
- generation of export-ready exercises (with or without problem statements, embedded files, and build plan),
- restoration of a pre-existing custom problem statement when applicable, and
- delegation to a private helper that writes and commits dummy files before invoking the export REST endpoint.
This reduces duplication across the export tests and keeps the export path consistent. The logging of the original problem statement and the optional restoration logic are both reasonable for debugging failing export tests.
2200-2212: Temp directory handling incopyRepository_testNotCreatedErroravoids FS leaksCreating a dedicated temporary directory for
teamLocalPath, wiring it viagitService.getDefaultLocalPathOfRepo, and deleting it in afinallyblock is a good fix for the previous potential resource leak when the copy operation fails. UsingFiles.deleteIfExistsis sufficient here because the mockedcopyBareRepositoryWithoutHistorythrows before populating the path.src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (3)
32-79: Cleanup helpers make LocalRepository lifecycle manageable in tests
deleteFileSilently,deleteDirectoryIfExists, andresetReposprovide simple, null-safe cleanup primitives for files, directories, andLocalRepositoryinstances. SwallowingIOExceptionis acceptable in this test-only context, and grouping these methods here should help new LocalVC tests avoid leaving Git working copies or bare repos behind. No issues found.
81-183: LocalVC seeding and wiring helpers are coherent and follow naming conventionsThe trio of helpers:
seedBareRepository,seedLocalVcBareFrom, andseedStudentRepositoryForParticipation,together with
wireRepositoryToExerciseandcreateAndWireBaseRepositories, give tests a clear API to:
- create LocalVC-compatible repositories,
- clone/copy bare content into new repos, and
- wire URIs to exercises and participations.
Slug construction (
projectKey.toLowerCase() + "-exercise"/"-solution"/"-" + RepositoryType.TESTS.getName()) matches existing patterns, and leaving persistence of exercises/participations to the caller keeps this utility side-effect-light outside of the filesystem. Copying the bare.gitdirectory inseedLocalVcBareFromis appropriate for test scenarios where only bare content matters.
247-295: Latest-commit and student repo seeding helpers align with LocalVC usage
getLatestCommitcorrectly guards against empty history by throwing if no commit is present.createTemplateWorkingCopy,seedStudentRepoFromTemplateAndWire, andseedStudentRepoWithFilesAndPushprovide a consistent story for:
- creating a working copy for the template repo,
- cloning bare content into a new student bare repo, and
- wiring the student participation’s repository URI and populating it with commits.
The comments making it explicit that callers are responsible for persisting updated participations are helpful. Behavior looks coherent given the LocalVC test context.
src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| @Disabled("Temporary: Programming exercise participation creation in exam contexts needs LocalVC repo setup") |
There was a problem hiding this comment.
🧩 Analysis chain
Verify tracking exists for re-enabling this test.
The test is appropriately disabled during LocalVC infrastructure migration. However, ensure there's a tracking issue or task to re-enable these tests once the LocalVC repo setup is complete.
🏁 Script executed:
#!/bin/bash
# Check for related TODOs or tracking issues in the codebase
rg -n "LocalVC.*repo.*setup" --type java -C 2Length of output: 5715
Add TODO comment with issue tracking reference for re-enabling disabled tests.
The @disabled annotations appropriately explain why tests are temporarily disabled during LocalVC infrastructure migration. However, to ensure these tests are tracked for re-enabling once the infrastructure is complete, add a TODO comment referencing a GitHub issue. Best practice recommends that temporary disables correspond to tickets in your issue tracker.
Update line 183 (and similar disabled tests in src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java) to include:
@Test
// TODO: Re-enable once LocalVC repo setup is aligned with exam participation context - see GitHub issue #<ISSUE_NUMBER>
@Disabled("Temporary: Programming exercise participation creation in exam contexts needs LocalVC repo setup")
@WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
void testStartExerciseWithProgrammingExercise() throws Exception {Link the corresponding GitHub issue in the TODO comment to enable tracking and ensure the test suite is reviewed once infrastructure changes are complete.
🤖 Prompt for AI Agents
In src/test/java/de/tum/cit/aet/artemis/exam/ExamStartTest.java around line 183
(and likewise in
src/test/java/de/tum/cit/aet/artemis/exercise/service/ParticipationServiceTest.java
where similar @Disabled annotations exist), add a single-line TODO comment
immediately above the @Disabled annotation that references a GitHub issue for
tracking re-enabling the test, e.g. "// TODO: Re-enable once LocalVC repo setup
is aligned with exam participation context - see GitHub issue #<ISSUE_NUMBER>",
keeping the existing @Test, @Disabled and @WithMockUser annotations unchanged;
replace <ISSUE_NUMBER> with the actual issue number when available.
| exerciseRepository.save(programming); | ||
|
|
||
| doReturn(true).when(versionControlService).checkIfProjectExists(any(), any()); | ||
| versionControlService.createProjectForExercise(programming); |
There was a problem hiding this comment.
Critical: Resource leak - LocalRepository not cleaned up.
The call to createProjectForExercise(programming) creates file-system resources (LocalRepository instances) that are never cleaned up. The tearDown() method only cleans up programmingExerciseTestService.exerciseRepo, but not the repository created here. This leads to disk space accumulation and test pollution across repeated runs.
Apply one of these solutions:
Solution 1: Track and clean up in tearDown
Store the created programming exercise and clean it up:
+ private ProgrammingExercise createdProgrammingExercise;
+
@AfterEach
void tearDown() throws Exception {
jenkinsRequestMockProvider.reset();
if (programmingExerciseTestService.exerciseRepo != null) {
programmingExerciseTestService.tearDown();
}
+ if (createdProgrammingExercise != null) {
+ programmingExerciseTestService.cleanup(createdProgrammingExercise);
+ createdProgrammingExercise = null;
+ }
ParticipantScoreScheduleService.DEFAULT_WAITING_TIME_FOR_SCHEDULED_TASKS = 500;
participantScoreScheduleService.shutdown();
}And in the test:
- versionControlService.createProjectForExercise(programming);
+ createdProgrammingExercise = programming;
+ versionControlService.createProjectForExercise(createdProgrammingExercise);Solution 2: Use try-finally for local cleanup
- versionControlService.createProjectForExercise(programming);
- doReturn(null).when(continuousIntegrationService).checkIfProjectExists(any(), any());
-
- request.performMvcRequest(
- post("/api/exam/courses/" + course1.getId() + "/exam-import").contentType(MediaType.APPLICATION_JSON).content(objectMapper.writeValueAsString(exam)))
- .andExpect(status().isBadRequest())
- .andExpect(result -> assertThat(result.getResolvedException()).hasMessage("Exam contains programming exercise(s) with invalid short name."));
+ try {
+ versionControlService.createProjectForExercise(programming);
+ doReturn(null).when(continuousIntegrationService).checkIfProjectExists(any(), any());
+
+ request.performMvcRequest(
+ post("/api/exam/courses/" + course1.getId() + "/exam-import").contentType(MediaType.APPLICATION_JSON).content(objectMapper.writeValueAsString(exam)))
+ .andExpect(status().isBadRequest())
+ .andExpect(result -> assertThat(result.getResolvedException()).hasMessage("Exam contains programming exercise(s) with invalid short name."));
+ }
+ finally {
+ programmingExerciseTestService.cleanup(programming);
+ }Based on learnings
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/test/java/de/tum/cit/aet/artemis/exam/ProgrammingExamIntegrationTest.java
around line 158, the call to
versionControlService.createProjectForExercise(programming) creates
LocalRepository filesystem resources that are not cleaned up; modify the test to
either (A) capture and store the returned project/repository reference in a
field and delete/cleanup that repository in the existing tearDown() method so
all created repos are removed after each test, or (B) wrap the call and
subsequent operations in a try-finally block and perform repository cleanup in
the finally block to remove the LocalRepository immediately after use; ensure
the chosen cleanup calls use the same cleanup utility used for
programmingExerciseTestService.exerciseRepo to avoid disk leaks and test
pollution.
src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCLocalCIIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
| private void seedRepositoryForParticipation(ProgrammingExerciseStudentParticipation participation, String filename) throws Exception { | ||
| LocalRepository repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation); | ||
| RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of(filename, "class %s {}".formatted(filename.replace('.', '_'))), "seed " + filename); | ||
| participationRepository.save(participation); | ||
| var latestCommit = RepositoryExportTestUtil.getLatestCommit(repo); | ||
| participationCommitHashes.put(participation.getParticipantIdentifier(), latestCommit.getName()); | ||
| } |
There was a problem hiding this comment.
Track created LocalRepository instances and reset them in teardown.
Avoid accumulating working copies/bare repos and open Git handles.
class ProgrammingSubmissionIntegrationTest extends AbstractProgrammingIntegrationJenkinsLocalVCTest {
@@
- private final Map<String, String> participationCommitHashes = new HashMap<>();
+ private final Map<String, String> participationCommitHashes = new HashMap<>();
+ private final java.util.List<LocalRepository> createdRepos = new java.util.ArrayList<>();
@@
private void seedRepositoryForParticipation(ProgrammingExerciseStudentParticipation participation, String filename) throws Exception {
- LocalRepository repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation);
+ LocalRepository repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation);
+ createdRepos.add(repo);
RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of(filename, "class %s {}".formatted(filename.replace('.', '_'))), "seed " + filename);
participationRepository.save(participation);
var latestCommit = RepositoryExportTestUtil.getLatestCommit(repo);
participationCommitHashes.put(participation.getParticipantIdentifier(), latestCommit.getName());
}
+
+ @AfterEach
+ void cleanupRepos() {
+ de.tum.cit.aet.artemis.programming.util.RepositoryExportTestUtil.resetRepos(createdRepos.toArray(LocalRepository[]::new));
+ createdRepos.clear();
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java
around lines 766 to 772, the test creates LocalRepository instances but doesn't
track or release them, causing open Git handles and accumulated working copies;
update the helper to add each returned LocalRepository to a tracked list (e.g.,
a private List<LocalRepository> createdRepos) when seeding, and implement/extend
the existing teardown/@AfterEach to iterate this list to properly
close/reset/delete each LocalRepository and clear it; also ensure
participationCommitHashes or related maps are cleaned up in teardown to avoid
retained state.
| // GitService is autowired and uses real LocalVC-backed repositories in tests. | ||
| // There's no need to stub getOrCheckoutRepository here — use the real implementation. | ||
| var participation = participationUtilService.addStudentParticipationForProgrammingExerciseForLocalRepo(exercise, login, participationRepo.workingCopyGitRepoFile.toURI()); |
There was a problem hiding this comment.
Clean up LocalRepository resources after tests.
The LocalRepository creates working-copy/bare dirs and holds Git handles. Add teardown in callers to invoke LocalRepository.resetLocalRepo() (or use RepositoryExportTestUtil.resetRepos(...)) to avoid disk bloat and cross-test pollution.
Minimal example for a test class using this service:
+import org.junit.jupiter.api.AfterEach;
+import static de.tum.cit.aet.artemis.programming.util.RepositoryExportTestUtil.resetRepos;
+
public class SomeIntegrationTest {
- // ...
+ private final java.util.List<LocalRepository> createdRepos = new java.util.ArrayList<>();
+ @AfterEach
+ void cleanupRepos() { resetRepos(createdRepos.toArray(LocalRepository[]::new)); }
}When you create repos (e.g., via LocalVCLocalCITestService), add them to createdRepos.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingUtilTestService.java
around lines 169 to 171, tests create LocalRepository-backed repos but never
clean them up; add teardown logic to avoid disk bloat and cross-test pollution
by tracking created LocalRepository instances (or their repo paths) when you
call addStudentParticipationForProgrammingExerciseForLocalRepo and, in the test
class cleanup (e.g., @AfterEach/@AfterAll), call
LocalRepository.resetLocalRepo() or RepositoryExportTestUtil.resetRepos(...) to
remove working-copy/bare dirs and release Git handles; ensure any service that
constructs LocalVCLocalCITestService or LocalRepository adds repos to
createdRepos so they will be reset in teardown.
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
…thub.com:ls1intum/Artemis into chore/programming-exercises/improve-export-tests
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/test/java/de/tum/cit/aet/artemis/exercise/service/ExerciseVersionServiceTest.java (1)
226-241: Critical: LocalRepository resources are not cleaned up.The call to
createAndWireBaseRepositoriescreates LocalRepository instances on the file system (template, solution, test repositories), but these resources are never cleaned up. This will cause disk space accumulation and test pollution across repeated test runs.Solution: Capture the repository handles and clean them up in the
@AfterEachmethod:
- Add a field to store the repository handles:
private RepositoryExportTestUtil.BaseRepositories baseRepositories;
- Update the repository creation to capture handles:
- RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, newProgrammingExercise); + baseRepositories = RepositoryExportTestUtil.createAndWireBaseRepositoriesWithHandles(localVCLocalCITestService, newProgrammingExercise);
- Add cleanup in the
tearDownmethod:@AfterEach void tearDown() { + if (baseRepositories != null) { + baseRepositories.templateRepository().resetLocalRepo(); + baseRepositories.solutionRepository().resetLocalRepo(); + baseRepositories.testsRepository().resetLocalRepo(); + } exerciseVersionRepository.deleteAll(); }Based on PR objectives.
src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java (1)
293-296: LocalRepository cleanup ineffective—studentRepos list never populatedVerification confirms the review concern is valid. The
prepareStudentExamsForConduction()method receives thestudentReposlist parameter but never adds any LocalRepository instances to it. The list remains empty throughout execution, making the cleanup loop at lines 293–296 a no-op. Student repositories created duringExamPrepareExercisesTestUtil.prepareExerciseStart()are not tracked, resulting in orphaned file system resources and unclosed Git handles. TheprepareStudentExamsForConduction()method must be modified to add each student LocalRepository created during participation setup to the providedstudentReposlist.src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (4)
485-505: Same issue: seeded repos not cleanedThis test seeds two student repos and never resets them.
Apply:
- RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); @@ - List<Path> entries = unzipExportedFile(); + List<Path> entries; + try { + entries = unzipExportedFile(); + } + finally { + RepositoryExportTestUtil.resetRepos(repo1, repo2); + }
507-543: Reset created base/template/student repos after assertionstemplateRepository() and the student repo created via seedLocalVcBareFrom(...) are not reset.
Minimal end-of-test cleanup:
@@ - try (Git downloadedGit = Git.open(extractedRepo1.get().toFile())) { + try (Git downloadedGit = Git.open(extractedRepo1.get().toFile())) { RevCommit commit = downloadedGit.log().setMaxCount(1).call().iterator().next(); assertThat(commit.getAuthorIdent().getName()).isEqualTo("student"); assertThat(commit.getFullMessage()).isEqualTo("All student changes in one commit"); } + finally { + RepositoryExportTestUtil.resetRepos(templateRepo, studentRepo); + }
786-805: testsRepo working copy should be reset after generate-tests pathtestsRepo created via createAndConfigureLocalRepository(...) remains on disk.
Append:
var result = request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.OK); assertThat(result).startsWith("Successfully generated the structure oracle"); request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.BAD_REQUEST); + RepositoryExportTestUtil.resetRepos(testsRepo);
2226-2250: setupExerciseForExport wires base repos but never cleans projectRepeated runs can leave LocalVC project folders behind, influencing later tests.
Options:
- Prefer RepositoryExportTestUtil.createAndWireBaseRepositoriesWithHandles(...) here, return BaseRepositories to callers, and reset them after the export assertions.
- Or, after callers finish, invoke cleanupLocalVcProjectForKey(programmingExercise.getProjectKey()) to delete the whole project folder. Choose one strategy and apply consistently across callers. Based on learnings.
♻️ Duplicate comments (7)
src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java (1)
1922-1935: This disabled test has already been flagged.The test remains disabled with an investigation in progress. Ensure this is tracked and resolved before merging, as previously noted.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (5)
2092-2104: Redirect (omitBinaries): reset templateRepo at endTemplate working copy isn’t cleaned.
Add:
@@ request.getWithForwardedUrl("/api/programming/programming-exercises/" + programmingExercise.getId() + "/template-files-content" + queryParams, HttpStatus.OK, "/api/programming/repository/" + savedExercise.getTemplateParticipation().getId() + "/files-content" + queryParams); + RepositoryExportTestUtil.resetRepos(templateRepo);
2106-2117: Redirect (files-content): reset templateRepoSame as above.
Add:
@@ request.getWithForwardedUrl("/api/programming/programming-exercises/" + programmingExercise.getId() + "/template-files-content", HttpStatus.OK, "/api/programming/repository/" + savedExercise.getTemplateParticipation().getId() + "/files-content"); + RepositoryExportTestUtil.resetRepos(templateRepo);
2121-2154: Participation files-at-commit: reset seeded student repoThe student repo created via seedStudentRepositoryForParticipation(...) is not cleaned.
Add:
@@ request.getWithFileContents("/api/programming/programming-exercise-participations/" + studentParticipation.getId() + "/files-content/" + submission.getCommitHash(), HttpStatus.OK, filesWithContentsAsJson); + RepositoryExportTestUtil.resetRepos(repo);
2156-2175: Forbidden variant: reset seeded student repoSame leak in forbidden path.
Add:
@@ request.get("/api/programming/programming-exercise-participations/" + participation1.getId() + "/files-content/" + submission.getCommitHash(), HttpStatus.FORBIDDEN, Map.class); + RepositoryExportTestUtil.resetRepos(repo);
357-366: Capture seeded repos and reset them to prevent leaksseedStudentRepositoryForParticipation(...) returns LocalRepository handles that aren’t cleaned. Capture both and reset in finally or at method end.
Suggested change:
- RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); @@ - return unzipExportedFile(); + try { + return unzipExportedFile(); + } + finally { + RepositoryExportTestUtil.resetRepos(repo1, repo2); + }src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseGitIntegrationTest.java (1)
101-109: Remote LocalRepository must be reset in finally to prevent FS leaksThe LocalRepository created via createAndConfigureLocalRepository(...) isn’t reset. Wrap usage in try/finally and call remoteRepo.resetLocalRepo().
Suggested patch (abbrev., combine with previous comment if applied):
- LocalRepository remoteRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, repoSlug); - - // Write a file and commit on the remote working copy, then push to origin - var readmePath = remoteRepo.workingCopyGitRepoFile.toPath().resolve("README.md"); - Files.writeString(readmePath, "Initial commit"); - remoteRepo.workingCopyGitRepo.add().addFilepattern(".").call(); - GitService.commit(remoteRepo.workingCopyGitRepo).setMessage("Initial commit").call(); - remoteRepo.workingCopyGitRepo.push().setRemote("origin").call(); + LocalRepository remoteRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, repoSlug); + try { + // Write a file and commit on the remote working copy, then push to origin + var readmePath = remoteRepo.workingCopyGitRepoFile.toPath().resolve("README.md"); + Files.writeString(readmePath, "Initial commit"); + remoteRepo.workingCopyGitRepo.add().addFilepattern(".").call(); + GitService.commit(remoteRepo.workingCopyGitRepo).setMessage("Initial commit").call(); + remoteRepo.workingCopyGitRepo.push().setRemote("origin").call(); + // ... rest of test ... + } + finally { + remoteRepo.resetLocalRepo(); + }
🧹 Nitpick comments (11)
src/test/java/de/tum/cit/aet/artemis/exercise/service/ExerciseVersionServiceTest.java (1)
239-241: Consider failing the test on repository creation errors.The exception handling has been broadened from specific Git-related exceptions to a generic
Exception, and the catch block only logs the error without failing the test. If repository creation fails, the test may produce misleading results.Consider whether the test should fail explicitly when repository setup fails:
catch (Exception e) { log.error("Failed to create programming exercise", e); + throw new RuntimeException("Repository setup failed", e); }Alternatively, if silent failure is intentional, add a comment explaining why.
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCIntegrationTest.java (1)
94-102: Consider narrowing the exception type for more precise handling.While the refactor to use
RepositoryExportTestUtil.safeDeleteDirectoryis good, broadening the catch fromIOExceptiontoExceptionmay inadvertently catch runtime exceptions that indicate logic errors rather than filesystem issues.If feasible, consider catching more specific exceptions:
try { RepositoryExportTestUtil.safeDeleteDirectory(someRepository.remoteBareGitRepoFile.toPath()); } -catch (Exception exception) { +catch (IOException exception) { // JGit creates a lock file in each repository that could cause deletion problems. if (exception.getMessage().contains("gc.log.lock")) { return; } throw exception; }Note: If
safeDeleteDirectorythrows checked exceptions other thanIOException, the current broad catch may be necessary.src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java (1)
1508-1523: Clone‑modify‑push helper is robust; small hardening optionalThe try‑with‑resources Git + finally safeDeleteDirectory pattern is correct. Optional: add “git.add().addFilepattern('.')” to future‑proof nested paths; today’s single‑file add is fine.
- git.add().addFilepattern(fileName).call(); + git.add().addFilepattern(".").call();Based on learnings.
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (3)
201-217: Teardown is good; consider also deleting the LocalVC projectresetLocalRepo() + safe directory deletes are solid. To avoid residue when a test fails before @beforeeach runs (or between classes), optionally delete the LocalVC project here as well.
@AfterEach void tearDown() throws IOException { ... - deleteDirectoryIfExists(Path.of("local/server-integration-test/repos")); - deleteDirectoryIfExists(Path.of("local/server-integration-test/repos-download")); + deleteDirectoryIfExists(Path.of("local/server-integration-test/repos")); + deleteDirectoryIfExists(Path.of("local/server-integration-test/repos-download")); + // Best-effort cleanup of LocalVC project artifacts created in this test + try { + deleteExistingProject(projectKey); + } catch (Exception ignored) { + } }
1177-1181: Seeding helper is concise; could reuse shared test util to reduce duplicationCurrent code is fine. If you want less boilerplate, RepositoryExportTestUtil.writeAndCommit/writeFilesAndPush offer the same add/commit/push pattern across suites.
- repository.workingCopyGitRepo.add().addFilepattern(".").call(); - GitService.commit(repository.workingCopyGitRepo).setMessage("Initial commit for " + currentLocalFileName).call(); - repository.workingCopyGitRepo.push().setRemote("origin").call(); + RepositoryExportTestUtil.writeFilesAndPush( + repository, + Map.of( + currentLocalFileName, currentLocalFileContent, + currentLocalFolderName + "/.keep", "" + ), + "Initial commit for " + currentLocalFileName +);Based on learnings.
Also applies to: 1183-1196
227-231: Prefer Path.resolve over string concatenation for FS assertionsMinor portability/readability nit. Applies to similar occurrences below.
- assertThat(Path.of(studentRepository.workingCopyGitRepoFile + "/" + key)).exists(); + assertThat(studentRepository.workingCopyGitRepoFile.toPath().resolve(key)).exists();As per coding guidelines.
src/test/java/de/tum/cit/aet/artemis/exercise/programming/AuxiliaryRepositoryResourceIntegrationTest.java (1)
390-400: Service-unavailable path: remote deletion helper looks gooddeleteRemoteAuxiliaryRepository() safely closes and removes the bare and working copies. Small nit: consider nulling the fields after close to avoid accidental reuse, but not required.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java (4)
854-883: LocalVC-backed setup for template/solution/tests/aux commit history looks goodThe nested
GetCommitHistoryForTemplateSolutionTestOrAuxReposetup correctly:
- Loads an exercise with template/solution and auxiliary repos eagerly.
- Wires LocalVC base repositories via
RepositoryExportTestUtil.createAndWireBaseRepositories.- Seeds deterministic commits in template, solution, tests, and auxiliary repos using the new
commitToRepositoryhelper and stored commit messages.This aligns well with the PR goal of using real LocalVC repositories instead of mocks. One minor point:
userUtilService.addUsersis also called in the outer@BeforeEach; if that method is not idempotent, double invocation per nested test could cause redundant work or constraints violations. Please confirm idempotence or consider removing one of the calls.
931-941: Auxiliary repository configuration helper is correct but could guard explicitly against missing repos
ensureAuxiliaryRepositoryConfiguredcorrectly:
- Uses the first auxiliary repository configured on the exercise.
- Lazily creates and configures a LocalVC repository (including URI) only if the
repositoryUriis stillnull.- Persists updates via
auxiliaryRepositoryRepository.save.Given that
programmingExerciseIntegrationTestService.addAuxiliaryRepositoryToExerciseis called earlier, this is fine; as a defensive improvement, you could optionally assert thatgetAuxiliaryRepositories()is non-empty to fail fast with a clearer message if the fixture ever changes.
968-999: Commit-details view setup correctly seeds student/template/solution commitsThe
GetParticipationRepositoryFilesForCommitsDetailsViewnested setup:
- Creates a fresh programming exercise with eager template/solution/aux relations.
- Wires LocalVC base repos via
RepositoryExportTestUtil.createAndWireBaseRepositories.- Adds a student participation and uses
commitToParticipationRepositoryto create a student commit.- Seeds separate commits in template and solution repos and persists all changes.
- Builds a clear
basePathfor the commit-details endpoint.This yields three independent commit hashes tied to real repositories, which is exactly what the subsequent tests rely on. Only nit: like the previous nested class, it re-calls
userUtilService.addUsers; ifaddUsersis not idempotent with respect to the test database lifecycle, you may want to avoid duplicating that call.
1109-1159: LocalVC commit helpers are sound; consider using repositorySlug() instead of manual.gittrimmingThe helper chain:
commitToParticipationRepositorydelegates viagetVcsRepositoryUri().commitToRepository(VcsRepositoryUri)guards againstnulland normalizes toLocalVCRepositoryUriwhen necessary.commitToRepository(LocalVCRepositoryUri)ensures the bare repo exists (creating it vialocalVCLocalCITestService.createAndConfigureLocalRepositoryif needed), resolves its local path, and callswriteFilesAndPush.writeFilesAndPushclones the bare repo into a temp working copy undertempPath, writes all requested files, commits usingGitService.commit(git).setMessage(message).call(), pushes, and then cleans up the working copy viaRepositoryExportTestUtil.safeDeleteDirectory.This achieves the PR goal of seeding real Git history while:
- Avoiding leaks of temporary working directories.
- Handling both
LocalVCRepositoryUriand genericVcsRepositoryUricases.One small refactor to simplify
ensureLocalVcRepositoryExists:- String slugWithGit = repositoryUri.getRelativeRepositoryPath().getFileName().toString(); - String repositorySlug = slugWithGit.endsWith(".git") ? slugWithGit.substring(0, slugWithGit.length() - 4) : slugWithGit; - localVCLocalCITestService.createAndConfigureLocalRepository(repositoryUri.getProjectKey(), repositorySlug); + String repositorySlug = repositoryUri.repositorySlug(); + localVCLocalCITestService.createAndConfigureLocalRepository(repositoryUri.getProjectKey(), repositorySlug);Using
repositorySlug()(inherited fromVcsRepositoryUri) removes manual.gitstripping and keeps the slug derivation consistent with the rest of the codebase. Otherwise, the logic and cleanup in these helpers look solid.
| // Build the LocalVC URI and checkout to a separate target path | ||
| LocalVCRepositoryUri repoUri = new LocalVCRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, repoSlug)); | ||
| Path targetPath = tempPath.resolve("lcvc-checkout").resolve("student-checkout"); | ||
| var checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true); | ||
|
|
There was a problem hiding this comment.
Also delete the checked-out local clone to avoid leftover worktrees
getOrCheckoutRepositoryWithTargetPath(...) creates a local clone under tempPath that isn’t deleted. Ensure it’s removed even on failures.
Apply:
@@
- LocalVCRepositoryUri repoUri = new LocalVCRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, repoSlug));
- Path targetPath = tempPath.resolve("lcvc-checkout").resolve("student-checkout");
- var checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true);
+ LocalVCRepositoryUri repoUri = new LocalVCRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, repoSlug));
+ Path targetPath = tempPath.resolve("lcvc-checkout").resolve("student-checkout");
+ de.tum.cit.aet.artemis.programming.domain.Repository checkedOut = null;
+ try {
+ checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true);
@@
- gitService.pullIgnoreConflicts(checkedOut);
- gitService.resetToOriginHead(checkedOut);
+ gitService.pullIgnoreConflicts(checkedOut);
+ gitService.resetToOriginHead(checkedOut);
+ }
+ finally {
+ if (checkedOut != null) {
+ gitService.deleteLocalRepository(checkedOut);
+ }
+ }Also applies to: 120-130
🤖 Prompt for AI Agents
In
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseGitIntegrationTest.java
around lines 110-114 (also apply same fix to 120-130), the test uses
gitService.getOrCheckoutRepositoryWithTargetPath(...) which leaves a local clone
under tempPath; wrap the checkout and any subsequent work in a try/finally and
in the finally block reliably delete the checked-out targetPath (recursively) so
the local clone/worktree is removed even on failures; ensure any repository
handles are closed before deletion and reuse the same deletion logic for the
other occurrence at lines 120-130.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java (1)
403-440: Critical: Missing resource cleanup for LocalRepository.The
sourceRepocreated at line 412 is never cleaned up, causing file-system resources (Git instances, directories) to accumulate across test runs. This is the exact issue raised in the PR objectives comment.Add cleanup using
RepositoryExportTestUtil.resetRepos():assertThat(repoFiles).as("export returns exactly one file: README.md").isNotNull().hasSize(1).containsOnlyKeys("README.md").containsEntry("README.md", "Initial commit"); + + // Cleanup local repository + RepositoryExportTestUtil.resetRepos(sourceRepo); }Alternatively, wrap repository operations in try-finally or move cleanup to an
@AfterEachmethod if multiple tests share this pattern.src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (4)
485-505: Critical: Resource leak - repositories not cleaned up.Lines 488-489 seed LocalRepository instances without capturing them, causing filesystem resource leaks across test runs.
Apply the same cleanup pattern:
void testExportSubmissionsByParticipationIds() throws Exception { - // Seed LocalVC student repositories and wire URIs - // use shared util to seed and wire repos for both participations - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); - var participationIds = programmingExerciseStudentParticipationRepository.findAll().stream().map(participation -> participation.getId().toString()).toList(); - final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + String.join(",", participationIds); - // all options false by default, only test if export works at all - var exportOptions = new RepositoryExportOptionsDTO(); - - downloadedFile = request.postWithResponseBodyFile(path, exportOptions, HttpStatus.OK); - assertThat(downloadedFile).exists(); - - List<Path> entries = unzipExportedFile(); - - // Make sure both repositories are present (by login suffix) - assertThat(entries).anyMatch(entry -> entry.toString().endsWith(Path.of(userPrefix + "student1", ".git").toString())) - .anyMatch(entry -> entry.toString().endsWith(Path.of(userPrefix + "student2", ".git").toString())); + try { + var participationIds = programmingExerciseStudentParticipationRepository.findAll().stream().map(participation -> participation.getId().toString()).toList(); + final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + String.join(",", participationIds); + var exportOptions = new RepositoryExportOptionsDTO(); + + downloadedFile = request.postWithResponseBodyFile(path, exportOptions, HttpStatus.OK); + assertThat(downloadedFile).exists(); + + List<Path> entries = unzipExportedFile(); + + assertThat(entries).anyMatch(entry -> entry.toString().endsWith(Path.of(userPrefix + "student1", ".git").toString())) + .anyMatch(entry -> entry.toString().endsWith(Path.of(userPrefix + "student2", ".git").toString())); + } + finally { + RepositoryExportTestUtil.resetRepos(repo1, repo2); + } }Based on PR objectives comment.
507-543: Critical: Multiple repository leaks in anonymization test.This test creates four LocalRepository instances (template, solution, tests via line 509, and student via line 518) without cleaning any of them, causing significant filesystem resource accumulation.
Capture all repositories and clean them:
void testExportSubmissionAnonymizationCombining() throws Exception { - // Ensure base repos exist and retrieve handles for template/solution/tests var baseRepositories = RepositoryExportTestUtil.createAndWireBaseRepositoriesWithHandles(localVCLocalCITestService, programmingExercise); programmingExercise = programmingExerciseRepository.save(programmingExercise); - // Prepare template working copy handle var templateRepo = baseRepositories.templateRepository(); String projectKey = programmingExercise.getProjectKey(); - // Seed student repository from template bare and wire URI String studentSlug = localVCLocalCITestService.getRepositorySlug(projectKey, participation1.getParticipantIdentifier()); var studentRepo = RepositoryExportTestUtil.seedLocalVcBareFrom(localVCLocalCITestService, projectKey, studentSlug, templateRepo); participation1.setRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, studentSlug)); programmingExerciseStudentParticipationRepository.save(participation1); - // Add a student commit to anonymize - localVCLocalCITestService.commitFile(studentRepo.workingCopyGitRepoFile.toPath(), studentRepo.workingCopyGitRepo, "Test.java"); - studentRepo.workingCopyGitRepo.push().setRemote("origin").call(); - - // Rest call with options (combine + anonymize enabled in getOptions()) - final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + participation1.getId(); - downloadedFile = request.postWithResponseBodyFile(path, getOptions(), HttpStatus.OK); - assertThat(downloadedFile).exists(); - - List<Path> entries = unzipExportedFile(); - - // Checks: file present and single anonymized commit - assertThat(entries).anyMatch(entry -> entry.endsWith("Test.java")); - Optional<Path> extractedRepo1 = entries.stream() - .filter(entry -> entry.toString().endsWith(Path.of("-" + participation1.getId() + "-student-submission.git", ".git").toString())).findFirst(); - assertThat(extractedRepo1).isPresent(); - try (Git downloadedGit = Git.open(extractedRepo1.get().toFile())) { - RevCommit commit = downloadedGit.log().setMaxCount(1).call().iterator().next(); - assertThat(commit.getAuthorIdent().getName()).isEqualTo("student"); - assertThat(commit.getFullMessage()).isEqualTo("All student changes in one commit"); + try { + localVCLocalCITestService.commitFile(studentRepo.workingCopyGitRepoFile.toPath(), studentRepo.workingCopyGitRepo, "Test.java"); + studentRepo.workingCopyGitRepo.push().setRemote("origin").call(); + + final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + participation1.getId(); + downloadedFile = request.postWithResponseBodyFile(path, getOptions(), HttpStatus.OK); + assertThat(downloadedFile).exists(); + + List<Path> entries = unzipExportedFile(); + + assertThat(entries).anyMatch(entry -> entry.endsWith("Test.java")); + Optional<Path> extractedRepo1 = entries.stream() + .filter(entry -> entry.toString().endsWith(Path.of("-" + participation1.getId() + "-student-submission.git", ".git").toString())).findFirst(); + assertThat(extractedRepo1).isPresent(); + try (Git downloadedGit = Git.open(extractedRepo1.get().toFile())) { + RevCommit commit = downloadedGit.log().setMaxCount(1).call().iterator().next(); + assertThat(commit.getAuthorIdent().getName()).isEqualTo("student"); + assertThat(commit.getFullMessage()).isEqualTo("All student changes in one commit"); + } + } + finally { + RepositoryExportTestUtil.resetRepos(baseRepositories.templateRepository(), baseRepositories.solutionRepository(), + baseRepositories.testsRepository(), studentRepo); } }Based on PR objectives comment.
577-585: Critical: Resource leak in helper method.Lines 579-580 create LocalRepository instances without capturing them for cleanup. This helper is called by
testExportSubmissionsByStudentLogins()which also needs the cleanup.Refactor to capture and return repositories for cleanup by the caller:
-private File exportSubmissionsByStudentLogins() throws Exception { - // Seed LocalVC student repositories and wire URIs - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); +private record ExportResult(File file, LocalRepository repo1, LocalRepository repo2) {} + +private ExportResult exportSubmissionsByStudentLogins() throws Exception { + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participant-identifiers/" + userPrefix + "student1," + userPrefix + "student2"; - return request.postWithResponseBodyFile(path, getOptions(), HttpStatus.OK); + var file = request.postWithResponseBodyFile(path, getOptions(), HttpStatus.OK); + return new ExportResult(file, repo1, repo2); }Then update
testExportSubmissionsByStudentLogins()at lines 569-575:void testExportSubmissionsByStudentLogins() throws Exception { - downloadedFile = exportSubmissionsByStudentLogins(); - assertThat(downloadedFile).exists(); - List<Path> entries = unzipExportedFile(); - // Assert both participant repos are included. For this endpoint/options, repos are anonymized and use the student-submission.git suffix - assertThat(entries).anyMatch(entry -> entry.toString().endsWith("student-submission.git" + File.separator + ".git")); + var exportResult = exportSubmissionsByStudentLogins(); + try { + downloadedFile = exportResult.file(); + assertThat(downloadedFile).exists(); + List<Path> entries = unzipExportedFile(); + assertThat(entries).anyMatch(entry -> entry.toString().endsWith("student-submission.git" + File.separator + ".git")); + } + finally { + RepositoryExportTestUtil.resetRepos(exportResult.repo1(), exportResult.repo2()); + } }Based on PR objectives comment.
786-805: Critical: Repository leaks in structure oracle test.Lines 788 and 794 create LocalRepository instances (4 total: template, solution, tests from base, plus explicit tests repo) without cleanup.
Capture and clean up all repositories:
void testGenerateStructureOracle() throws Exception { - // Wire base repositories in LocalVC and ensure tests repo has the expected directory - RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, programmingExercise); + var baseRepos = RepositoryExportTestUtil.createAndWireBaseRepositoriesWithHandles(localVCLocalCITestService, programmingExercise); programmingExercise = programmingExerciseRepository.save(programmingExercise); programmingExercise = programmingExerciseRepository.getProgrammingExerciseWithBuildConfigElseThrow(programmingExercise); String projectKey = programmingExercise.getProjectKey(); String testsSlug = projectKey.toLowerCase() + "-" + RepositoryType.TESTS.getName(); var testsRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, testsSlug); - String testsPath = java.nio.file.Path.of("test", programmingExercise.getPackageFolderName()).toString(); - if (programmingExercise.getBuildConfig().hasSequentialTestRuns()) { - testsPath = java.nio.file.Path.of("structural", testsPath).toString(); + + try { + String testsPath = java.nio.file.Path.of("test", programmingExercise.getPackageFolderName()).toString(); + if (programmingExercise.getBuildConfig().hasSequentialTestRuns()) { + testsPath = java.nio.file.Path.of("structural", testsPath).toString(); + } + RepositoryExportTestUtil.writeFilesAndPush(testsRepo, Map.of(testsPath + "/.placeholder", ""), "Init tests dir"); + + final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/generate-tests"; + var result = request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.OK); + assertThat(result).startsWith("Successfully generated the structure oracle"); + request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.BAD_REQUEST); + } + finally { + RepositoryExportTestUtil.resetRepos(baseRepos.templateRepository(), baseRepos.solutionRepository(), + baseRepos.testsRepository(), testsRepo); } - RepositoryExportTestUtil.writeFilesAndPush(testsRepo, Map.of(testsPath + "/.placeholder", ""), "Init tests dir"); - - final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/generate-tests"; - var result = request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.OK); - assertThat(result).startsWith("Successfully generated the structure oracle"); - request.putWithResponseBody(path, programmingExercise, String.class, HttpStatus.BAD_REQUEST); }Based on PR objectives comment.
♻️ Duplicate comments (5)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseGitIntegrationTest.java (1)
95-131: Critical: LocalRepository and checked-out repository resources are never cleaned up.Despite past review comments indicating this was addressed, the current code still creates multiple filesystem resources that are never released:
remoteRepo(line 102):LocalRepositorycreates Git instances and directories butresetLocalRepo()is never calledcheckedOut(line 114): Repository domain object and its working directory attargetPath(line 113) are never closed or deleted- Over repeated test runs, this accumulates files and open Git handles, leading to disk space exhaustion and test pollution
Wrap all resource usage in try-finally blocks and ensure cleanup:
+ @Test + @WithMockUser(username = TEST_PREFIX + "student1", roles = { "USER", "STUDENT" }) + void testGitOperationsWithLocalVC() throws Exception { + // Create a LocalVC repository (acts as remote) and seed with an initial commit + var projectKey = "PROGEXGIT"; + var repoSlug = projectKey.toLowerCase() + "-tests"; + LocalRepository remoteRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, repoSlug); - - // Write a file and commit on the remote working copy, then push to origin - var readmePath = remoteRepo.workingCopyGitRepoFile.toPath().resolve("README.md"); - FileUtils.writeStringToFile(readmePath.toFile(), "Initial commit", java.nio.charset.StandardCharsets.UTF_8); - remoteRepo.workingCopyGitRepo.add().addFilepattern(".").call(); - GitService.commit(remoteRepo.workingCopyGitRepo).setMessage("Initial commit").call(); - remoteRepo.workingCopyGitRepo.push().setRemote("origin").call(); - - // Build the LocalVC URI and checkout to a separate target path - LocalVCRepositoryUri repoUri = new LocalVCRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, repoSlug)); - Path targetPath = tempPath.resolve("lcvc-checkout").resolve("student-checkout"); - var checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true); - - // Verify we can fetch and read last commit hash from the remote - gitService.fetchAll(checkedOut); - var lastHash = gitService.getLastCommitHash(repoUri); - assertThat(lastHash).as("last commit hash should exist on remote").isNotNull().isInstanceOf(ObjectId.class); - - // Create a local change, commit and push via GitService - var localFile = targetPath.resolve("hello.txt"); - Files.createDirectories(localFile.getParent()); - FileUtils.writeStringToFile(localFile.toFile(), "hello world", java.nio.charset.StandardCharsets.UTF_8); - gitService.stageAllChanges(checkedOut); - gitService.commitAndPush(checkedOut, "Add hello.txt", true, null); - - // Pull and reset operations should not throw - gitService.pullIgnoreConflicts(checkedOut); - gitService.resetToOriginHead(checkedOut); + de.tum.cit.aet.artemis.programming.domain.Repository checkedOut = null; + Path targetPath = null; + try { + // Write a file and commit on the remote working copy, then push to origin + var readmePath = remoteRepo.workingCopyGitRepoFile.toPath().resolve("README.md"); + FileUtils.writeStringToFile(readmePath.toFile(), "Initial commit", java.nio.charset.StandardCharsets.UTF_8); + remoteRepo.workingCopyGitRepo.add().addFilepattern(".").call(); + GitService.commit(remoteRepo.workingCopyGitRepo).setMessage("Initial commit").call(); + remoteRepo.workingCopyGitRepo.push().setRemote("origin").call(); + + // Build the LocalVC URI and checkout to a separate target path + LocalVCRepositoryUri repoUri = new LocalVCRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, repoSlug)); + targetPath = tempPath.resolve("lcvc-checkout").resolve("student-checkout"); + checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true); + + // Verify we can fetch and read last commit hash from the remote + gitService.fetchAll(checkedOut); + var lastHash = gitService.getLastCommitHash(repoUri); + assertThat(lastHash).as("last commit hash should exist on remote").isNotNull().isInstanceOf(ObjectId.class); + + // Create a local change, commit and push via GitService + var localFile = targetPath.resolve("hello.txt"); + Files.createDirectories(localFile.getParent()); + FileUtils.writeStringToFile(localFile.toFile(), "hello world", java.nio.charset.StandardCharsets.UTF_8); + gitService.stageAllChanges(checkedOut); + gitService.commitAndPush(checkedOut, "Add hello.txt", true, null); + + // Pull and reset operations should not throw + gitService.pullIgnoreConflicts(checkedOut); + gitService.resetToOriginHead(checkedOut); + } + finally { + // Clean up checked-out repository + if (checkedOut != null) { + checkedOut.close(); + } + if (targetPath != null) { + RepositoryExportTestUtil.safeDeleteDirectory(targetPath); + } + // Clean up LocalRepository (working copy + bare repo) + remoteRepo.resetLocalRepo(); + } + }src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (4)
2121-2175: Critical: Repository leaks in student participation file content tests.Both
testRedirectGetParticipationRepositoryFilesWithContentAtCommit(creates 4 repos: base + student) andtestRedirectGetParticipationRepositoryFilesWithContentAtCommitForbidden(creates 1 repo) leak LocalRepository instances without cleanup.Add cleanup to both tests:
void testRedirectGetParticipationRepositoryFilesWithContentAtCommit(String testPrefix) throws Exception { - // Ensure base repositories (template, solution, tests) exist and URIs are wired for this exercise - RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, programmingExercise); + var baseRepos = RepositoryExportTestUtil.createAndWireBaseRepositoriesWithHandles(localVCLocalCITestService, programmingExercise); programmingExercise = programmingExerciseRepository.save(programmingExercise); - // Create student participation with LocalVC repo - // Use a unique student to avoid repo collisions with other tests in this class String studentLogin = testPrefix + "student3"; var studentParticipation = participationUtilService.addStudentParticipationForProgrammingExercise(programmingExercise, studentLogin); var repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, studentParticipation); programmingExerciseStudentParticipationRepository.save(studentParticipation); - // Write files in one commit and push to origin to ensure the commit exists remotely - var commit = RepositoryExportTestUtil.writeFilesAndPush(repo, - Map.of("README.md", "Initial commit", "A.java", "abc", "B.java", "cde", "C.java", "efg", "test.txt", "Initial commit"), "seed student files"); - - // Persist submission with commit hash - var submission = new ProgrammingSubmission(); - submission.setType(SubmissionType.MANUAL); - submission.setCommitHash(commit.getId().getName()); - programmingExerciseUtilService.addProgrammingSubmission(programmingExercise, submission, studentLogin); - - String filesWithContentsAsJson = """ - { - "test.txt" : "Initial commit", - "C.java" : "efg", - "B.java" : "cde", - "A.java" : "abc", - "README.md" : "Initial commit" - }"""; - - request.getWithFileContents("/api/programming/programming-exercise-participations/" + studentParticipation.getId() + "/files-content/" + submission.getCommitHash(), - HttpStatus.OK, filesWithContentsAsJson); + try { + var commit = RepositoryExportTestUtil.writeFilesAndPush(repo, + Map.of("README.md", "Initial commit", "A.java", "abc", "B.java", "cde", "C.java", "efg", "test.txt", "Initial commit"), "seed student files"); + + var submission = new ProgrammingSubmission(); + submission.setType(SubmissionType.MANUAL); + submission.setCommitHash(commit.getId().getName()); + programmingExerciseUtilService.addProgrammingSubmission(programmingExercise, submission, studentLogin); + + String filesWithContentsAsJson = """ + { + "test.txt" : "Initial commit", + "C.java" : "efg", + "B.java" : "cde", + "A.java" : "abc", + "README.md" : "Initial commit" + }"""; + + request.getWithFileContents("/api/programming/programming-exercise-participations/" + studentParticipation.getId() + "/files-content/" + submission.getCommitHash(), + HttpStatus.OK, filesWithContentsAsJson); + } + finally { + RepositoryExportTestUtil.resetRepos(baseRepos.templateRepository(), baseRepos.solutionRepository(), + baseRepos.testsRepository(), repo); + } } void testRedirectGetParticipationRepositoryFilesWithContentAtCommitForbidden(String testPrefix) throws Exception { - // Seed LocalVC repo for existing participation1 and create a submission for its latest commit var studentLogin = participation1.getParticipantIdentifier(); var repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); programmingExerciseStudentParticipationRepository.save(participation1); - // Write files, commit, and push via util - var commit = RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of("README.md", "Initial commit", "A.java", "abc", "B.java", "cde", "C.java", "efg"), - "seed student files"); - - // Persist submission with commit hash - var submission = new ProgrammingSubmission(); - submission.setType(SubmissionType.MANUAL); - submission.setCommitHash(commit.getId().getName()); - programmingExerciseUtilService.addProgrammingSubmission(programmingExercise, submission, studentLogin); - - // Expect forbidden for current user - request.get("/api/programming/programming-exercise-participations/" + participation1.getId() + "/files-content/" + submission.getCommitHash(), HttpStatus.FORBIDDEN, - Map.class); + try { + var commit = RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of("README.md", "Initial commit", "A.java", "abc", "B.java", "cde", "C.java", "efg"), + "seed student files"); + + var submission = new ProgrammingSubmission(); + submission.setType(SubmissionType.MANUAL); + submission.setCommitHash(commit.getId().getName()); + programmingExerciseUtilService.addProgrammingSubmission(programmingExercise, submission, studentLogin); + + request.get("/api/programming/programming-exercise-participations/" + participation1.getId() + "/files-content/" + submission.getCommitHash(), HttpStatus.FORBIDDEN, + Map.class); + } + finally { + RepositoryExportTestUtil.resetRepos(repo); + } }Based on PR objectives comment.
2092-2117: Critical: Template repository leaks in both redirect tests.Both
test_redirectGetTemplateRepositoryFilesWithContentOmitBinaries(line 2097) andtest_redirectGetTemplateRepositoryFilesWithContent(line 2111) createtemplateRepoinstances without cleanup.Add cleanup to both tests:
void test_redirectGetTemplateRepositoryFilesWithContentOmitBinaries() throws Exception { - // Wire base repos via LocalVC RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, programmingExercise); programmingExercise = programmingExerciseRepository.save(programmingExercise); var templateRepo = RepositoryExportTestUtil.createTemplateWorkingCopy(localVCLocalCITestService, programmingExercise); - RepositoryExportTestUtil.writeFilesAndPush(templateRepo, Map.of("A.java", "abc", "B.jar", "binaryContent"), "seed template files"); - - var savedExercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(programmingExercise.getId()); - var queryParams = "?omitBinaries=true"; - request.getWithForwardedUrl("/api/programming/programming-exercises/" + programmingExercise.getId() + "/template-files-content" + queryParams, HttpStatus.OK, - "/api/programming/repository/" + savedExercise.getTemplateParticipation().getId() + "/files-content" + queryParams); + try { + RepositoryExportTestUtil.writeFilesAndPush(templateRepo, Map.of("A.java", "abc", "B.jar", "binaryContent"), "seed template files"); + + var savedExercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(programmingExercise.getId()); + var queryParams = "?omitBinaries=true"; + request.getWithForwardedUrl("/api/programming/programming-exercises/" + programmingExercise.getId() + "/template-files-content" + queryParams, HttpStatus.OK, + "/api/programming/repository/" + savedExercise.getTemplateParticipation().getId() + "/files-content" + queryParams); + } + finally { + RepositoryExportTestUtil.resetRepos(templateRepo); + } } void test_redirectGetTemplateRepositoryFilesWithContent() throws Exception { - // Wire base repos via LocalVC RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, programmingExercise); programmingExercise = programmingExerciseRepository.save(programmingExercise); var templateRepo = RepositoryExportTestUtil.createTemplateWorkingCopy(localVCLocalCITestService, programmingExercise); - RepositoryExportTestUtil.writeFilesAndPush(templateRepo, Map.of("A.java", "abc", "B.java", "cde", "C.java", "efg"), "seed template files"); - - var savedExercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(programmingExercise.getId()); - request.getWithForwardedUrl("/api/programming/programming-exercises/" + programmingExercise.getId() + "/template-files-content", HttpStatus.OK, - "/api/programming/repository/" + savedExercise.getTemplateParticipation().getId() + "/files-content"); + try { + RepositoryExportTestUtil.writeFilesAndPush(templateRepo, Map.of("A.java", "abc", "B.java", "cde", "C.java", "efg"), "seed template files"); + + var savedExercise = programmingExerciseRepository.findByIdWithTemplateAndSolutionParticipationElseThrow(programmingExercise.getId()); + request.getWithForwardedUrl("/api/programming/programming-exercises/" + programmingExercise.getId() + "/template-files-content", HttpStatus.OK, + "/api/programming/repository/" + savedExercise.getTemplateParticipation().getId() + "/files-content"); + } + finally { + RepositoryExportTestUtil.resetRepos(templateRepo); + } }Based on PR objectives comment.
359-365: Critical: Resource leak - LocalRepository instances not cleaned up.Lines 359-360 create LocalRepository instances that are never captured or cleaned up. These repositories create filesystem resources (bare repos + working copies) that accumulate across test runs, leading to disk exhaustion and test pollution.
Capture the returned repositories and ensure cleanup:
List<Path> exportSubmissionsWithPracticeSubmissionByParticipationIds(boolean excludePracticeSubmissions) throws Exception { - // Seed LocalVC repositories for both participations and wire URIs - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); - RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); + var repo1 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation1); + var repo2 = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation2); // Set one of the participations to practice mode participation1.setPracticeMode(false); participation2.setPracticeMode(true); programmingExerciseStudentParticipationRepository.saveAll(List.of(participation1, participation2)); - // Export with excludePracticeSubmissions - var participationIds = programmingExerciseStudentParticipationRepository.findAll().stream().map(participation -> participation.getId().toString()).toList(); - final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + String.join(",", participationIds); - var exportOptions = new RepositoryExportOptionsDTO(false, false, false, null, excludePracticeSubmissions, false, false, false, false); - - downloadedFile = request.postWithResponseBodyFile(path, exportOptions, HttpStatus.OK); - assertThat(downloadedFile).exists(); - - return unzipExportedFile(); + try { + var participationIds = programmingExerciseStudentParticipationRepository.findAll().stream().map(participation -> participation.getId().toString()).toList(); + final var path = "/api/programming/programming-exercises/" + programmingExercise.getId() + "/export-repos-by-participation-ids/" + String.join(",", participationIds); + var exportOptions = new RepositoryExportOptionsDTO(false, false, false, null, excludePracticeSubmissions, false, false, false, false); + + downloadedFile = request.postWithResponseBodyFile(path, exportOptions, HttpStatus.OK); + assertThat(downloadedFile).exists(); + + return unzipExportedFile(); + } + finally { + RepositoryExportTestUtil.resetRepos(repo1, repo2); + } }Apply this pattern to all methods that seed LocalRepository instances.
Based on PR objectives comment.
1738-1762: Critical: Repository leaks in plagiarism check preparation loop.Lines 1748-1758 create up to two LocalRepository instances in a loop without capturing them for cleanup. If an exception occurs mid-loop, partial state leaks. On success, all repos leak.
Collect repositories and clean them even on exception:
private void prepareTwoSubmissionsForPlagiarismChecks(ProgrammingExercise programmingExercise) throws IOException, GitAPIException { var projectKey = programmingExercise.getProjectKey(); var exampleProgram = """ public class Main { /** * DO NOT EDIT! */ public static void main(String[] args) { Main main = new Main(); int magicNumber = main.calculateMagicNumber(); System.out.println("Magic number: " + magicNumber); } /** * Calculate the magic number. * * @return the magic number. */ private int calculateMagicNumber() { int a = 0; int b = 5; int magicNumber = 0; while (a < b) { magicNumber += b; a++; } return magicNumber; } } """; - // Get student participations (excluding instructor) var studentParticipations = programmingExerciseStudentParticipationRepository.findByExerciseId(programmingExercise.getId()).stream() .filter(p -> p.getParticipant() != null && p.getParticipant().getName() != null && !p.getParticipant().getName().contains("instructor")) .sorted(Comparator.comparing(DomainObject::getId)).toList(); if (studentParticipations.size() < 2) { throw new IllegalStateException("Expected at least 2 student participations for plagiarism checks"); } - // Seed real LocalVC repositories for the first two student participations with identical Java content + var seededRepos = new ArrayList<LocalRepository>(); - for (int i = 0; i < 2 && i < studentParticipations.size(); i++) { - try { - var participation = studentParticipations.get(i); - var repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation); - RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of("Main.java", exampleProgram), "seed plagiarism test content"); - programmingExerciseStudentParticipationRepository.save(participation); + try { + for (int i = 0; i < 2 && i < studentParticipations.size(); i++) { + try { + var participation = studentParticipations.get(i); + var repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation); + seededRepos.add(repo); + RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of("Main.java", exampleProgram), "seed plagiarism test content"); + programmingExerciseStudentParticipationRepository.save(participation); + } + catch (Exception e) { + throw new RuntimeException("Failed to seed plagiarism test repository", e); + } } - catch (Exception e) { - throw new RuntimeException("Failed to seed plagiarism test repository", e); - } + Files.createDirectories(localVCBasePath.resolve(projectKey)); + } + finally { + RepositoryExportTestUtil.resetRepos(seededRepos.toArray(LocalRepository[]::new)); } - - // Ensure the project folder exists - Files.createDirectories(localVCBasePath.resolve(projectKey)); }Note: This ensures cleanup even if the loop throws an exception after creating the first repository.
Based on PR objectives comment.
🧹 Nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (1)
202-217: Consider using RepositoryExportTestUtil.resetRepos for safer cleanup.The current teardown manually calls resetLocalRepo() on each repository with null checks. If one reset throws an exception (other than IOException), subsequent repositories won't be cleaned up. The RepositoryExportTestUtil.resetRepos helper (which you've imported and use elsewhere) wraps each reset in exception handling, ensuring all repositories are cleaned regardless of individual failures.
Apply this diff to improve cleanup robustness:
@AfterEach void tearDown() throws IOException { - if (studentRepository != null) { - studentRepository.resetLocalRepo(); - } - if (templateRepository != null) { - templateRepository.resetLocalRepo(); - } - if (solutionRepository != null) { - solutionRepository.resetLocalRepo(); - } - if (testsRepository != null) { - testsRepository.resetLocalRepo(); - } + RepositoryExportTestUtil.resetRepos(studentRepository, templateRepository, solutionRepository, testsRepository); deleteDirectoryIfExists(Path.of("local/server-integration-test/repos")); deleteDirectoryIfExists(Path.of("local/server-integration-test/repos-download")); }
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Test | ||
| void testStudentRepositoryExportEndpoint() throws Exception { |
There was a problem hiding this comment.
I think coderabbit's suggestion makes sense here
There was a problem hiding this comment.
It makes sense. But also throwing generic exception in tests make sense since we would prefer tests to fail early, and gather as much context possible for the failure. It is a common pattern used in our test repo.
HawKhiem
left a comment
HawKhiem
left a comment
There was a problem hiding this comment.
I think most of code rabbit's suggestions make sense to me. Other than that, no other major issue to me
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1)
361-393: Critical: tearDown does not clean up tracked LocalRepository instances.The
repositoryMetadata.clear()at line 392 removes map entries without callingresetLocalRepo()on the tracked repositories. Even repositories properly registered viaconfigureLocalRepositoryForSlug(line 437) will leak file-system resources and open Git handles.Impact: Disk space accumulation, open file handles, and test pollution across repeated runs.
Apply this diff to properly clean up tracked repositories before clearing the map:
public void tearDown() throws Exception { if (exerciseRepo != null) { exerciseRepo.resetLocalRepo(); } if (testRepo != null) { testRepo.resetLocalRepo(); } if (solutionRepo != null) { solutionRepo.resetLocalRepo(); } if (auxRepo != null) { auxRepo.resetLocalRepo(); } if (sourceExerciseRepo != null) { sourceExerciseRepo.resetLocalRepo(); } if (sourceTestRepo != null) { sourceTestRepo.resetLocalRepo(); } if (sourceSolutionRepo != null) { sourceSolutionRepo.resetLocalRepo(); } if (sourceAuxRepo != null) { sourceAuxRepo.resetLocalRepo(); } if (studentRepo != null) { studentRepo.resetLocalRepo(); } if (studentTeamRepo != null) { studentTeamRepo.resetLocalRepo(); } + // Clean up all repositories tracked in metadata + for (LocalRepository repo : new ArrayList<>(repositoryMetadata.keySet())) { + try { + repo.resetLocalRepo(); + } + catch (IOException ignored) { + // Log and continue - cleanup failures shouldn't break tearDown + } + } repositoryMetadata.clear(); }
♻️ Duplicate comments (2)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1)
575-615: Critical: LocalRepository resource leak in setupExerciseForExport (duplicate concern).The LocalRepository instances created at lines 610-612 are not tracked in
repositoryMetadata, so they will never be cleaned up even after the tearDown fix above. This is the same issue identified in the past review.Impact: Every call to
setupExerciseForExportleaks three LocalRepository instances (template, solution, tests), accumulating disk space and leaving Git resources open.Apply this diff to track the created repositories in metadata:
String projectKey = programmingExercise.getProjectKey(); String templateRepositorySlug = projectKey.toLowerCase() + "-exercise"; String solutionRepositorySlug = projectKey.toLowerCase() + "-solution"; String testsRepositorySlug = projectKey.toLowerCase() + "-tests"; var templateParticipation = programmingExercise.getTemplateParticipation(); templateParticipation.setRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, templateRepositorySlug)); templateProgrammingExerciseParticipationRepository.save(templateParticipation); var solutionParticipation = programmingExercise.getSolutionParticipation(); solutionParticipation.setRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, solutionRepositorySlug)); solutionProgrammingExerciseParticipationRepository.save(solutionParticipation); programmingExercise.setTestRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, testsRepositorySlug)); - localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, templateRepositorySlug); - localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, solutionRepositorySlug); - localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, testsRepositorySlug); + // Track repositories in metadata so tearDown can clean them up + LocalRepository templateRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, templateRepositorySlug); + LocalRepository solutionRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, solutionRepositorySlug); + LocalRepository testsRepo = localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, testsRepositorySlug); + + repositoryMetadata.put(templateRepo, new RepositoryMetadata(projectKey, templateRepositorySlug)); + repositoryMetadata.put(solutionRepo, new RepositoryMetadata(projectKey, solutionRepositorySlug)); + repositoryMetadata.put(testsRepo, new RepositoryMetadata(projectKey, testsRepositorySlug)); return programmingExerciseRepository.save(programmingExercise); }This fix works together with the tearDown fix above to ensure all dynamically created LocalRepository instances are properly cleaned up.
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java (1)
74-75: Track LocalRepository instances and reset them in@AfterEachto avoid leaking Git handles and temp repos.
seedRepositoryForParticipation(...)createsLocalRepositoryinstances that allocate working copies/bare repos on disk, but these handles are not tracked or cleaned up intearDown(). Similarly, the base repositories created ininit()viaRepositoryExportTestUtil.createAndWireBaseRepositories(...)are never reset. Over repeated runs this can accumulate file-system state and open Git resources, leading to flaky tests and disk bloat.You already have
RepositoryExportTestUtil.resetRepos(...)for cleanup; consider tracking all created repos and resetting them in@AfterEach. For example:class ProgrammingSubmissionIntegrationTest extends AbstractProgrammingIntegrationJenkinsLocalVCTest { @@ - private final Map<String, String> participationCommitHashes = new HashMap<>(); + private final Map<String, String> participationCommitHashes = new HashMap<>(); + private final List<LocalRepository> createdRepos = new ArrayList<>(); @@ @BeforeEach - void init() throws Exception { + void init() throws Exception { @@ - exercise = ExerciseUtilService.getFirstExerciseWithType(course, ProgrammingExercise.class); - RepositoryExportTestUtil.createAndWireBaseRepositories(localVCLocalCITestService, exercise); + exercise = ExerciseUtilService.getFirstExerciseWithType(course, ProgrammingExercise.class); + RepositoryExportTestUtil.BaseRepositories baseRepos = + RepositoryExportTestUtil.createAndWireBaseRepositoriesWithHandles(localVCLocalCITestService, exercise); + createdRepos.add(baseRepos.templateRepository()); + createdRepos.add(baseRepos.solutionRepository()); + createdRepos.add(baseRepos.testsRepository()); @@ @AfterEach void tearDown() throws Exception { + RepositoryExportTestUtil.resetRepos(createdRepos.toArray(LocalRepository[]::new)); + createdRepos.clear(); + participationCommitHashes.clear(); jenkinsRequestMockProvider.reset(); } @@ - private void seedRepositoryForParticipation(ProgrammingExerciseStudentParticipation participation, String filename) throws Exception { - LocalRepository repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation); + private void seedRepositoryForParticipation(ProgrammingExerciseStudentParticipation participation, String filename) throws Exception { + LocalRepository repo = RepositoryExportTestUtil.seedStudentRepositoryForParticipation(localVCLocalCITestService, participation); + createdRepos.add(repo); RepositoryExportTestUtil.writeFilesAndPush(repo, Map.of(filename, "class %s {}".formatted(filename.replace('.', '_'))), "seed " + filename); participationRepository.save(participation); var latestCommit = RepositoryExportTestUtil.getLatestCommit(repo); participationCommitHashes.put(participation.getParticipantIdentifier(), latestCommit.getName()); }This keeps LocalVC-backed repos deterministic for the tests while ensuring they are fully torn down after each test execution.
Also applies to: 76-83, 100-103, 772-778
🧹 Nitpick comments (3)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java (3)
12-18: GitService spy wiring looks good, but consider resetting stubs between testsUsing
@MockitoSpyBean GitService gitServiceSpyplus the extra Mockito imports is a good fit for simulating rare Git failure scenarios on top of the real LocalVC-backed bean. One thing to double‑check: stubbings likedoThrow(...)/doReturn(...)on this spy will survive beyond the individual test method unless they are reset somewhere (base classes or this class).If there is no global reset for
gitServiceSpyin a shared base test, consider adding a reset in this class’@AfterEachto keep tests isolated, e.g.:@AfterEach void tearDown() throws Exception { programmingExerciseTestService.tearDown(); jenkinsRequestMockProvider.reset(); aeolusRequestMockProvider.reset(); org.mockito.Mockito.reset(gitServiceSpy); }This avoids later tests unexpectedly seeing
commitAndPush/copyBareRepositoryWithoutHistory/getDefaultLocalPathOfRepostill stubbed from a previous scenario.Also applies to: 43-45, 61-63
282-289: Combined GitException + ZIP failure stubbing inimportExerciseFromFile_exception_directoryDeletedThe additional
doThrow(new GitException())ongitServiceSpy.commitAndPush(...)complements the existing ZIP extraction failure and should still drive the test towards the “directory gets scheduled for deletion” path.If the intent is to separately cover “Git commit fails” vs “ZIP extraction fails”, you might consider splitting this into two focused tests (one stubbing only
commitAndPush, one onlyextractZipFileRecursively) to make the failure mode being exercised explicit. As it stands, it’s correct but a bit harder to tell which failure is actually under test.
483-488:configureRepository_testBadRequestError: targeted GitService failure stubbingThe new
doThrow(new IOException())ongitServiceSpy.copyBareRepositoryWithoutHistory(...)is a straightforward way to hit the “Bad Request due to copy failure” path and aligns with the LocalVC‑backed setup.The only caveat is the same as above: ensure the stub is reset after the test (either via a shared
@AfterEachor local reset), otherwise later tests might still observecopyBareRepositoryWithoutHistorythrowing.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java(6 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java(14 hunks)src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java(11 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java(30 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
🧠 Learnings (45)
📓 Common learnings
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java:1455-1465
Timestamp: 2025-08-11T12:57:51.535Z
Learning: In the Artemis project, when ekayandan responds with "same as above" to a code review suggestion, it means they want to defer the suggested change to a follow-up PR to keep the current PR scope minimal and focused on the core functionality being implemented.
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 9303
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:296-300
Timestamp: 2024-10-20T18:37:45.365Z
Learning: When reviewing code changes in `StudentExamService.saveSubmission`, if the PR aims to improve readability without changing logic, avoid suggesting changes that alter logic, such as adding exceptions in the default case of switch statements.
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java:108-117
Timestamp: 2025-06-10T12:26:42.449Z
Learning: In ProgrammingExerciseImportFromFileService, the current directory traversal logic for sharing imports (walking importExerciseDir before extraction) is working correctly in practice and should not be changed to more complex solutions, even if there are theoretical issues with the approach.
📚 Learning: 2024-11-26T20:43:17.588Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-07-07T11:43:11.736Z
Learnt from: bassner
Repo: ls1intum/Artemis PR: 10705
File: src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideServiceTest.java:54-57
Timestamp: 2025-07-07T11:43:11.736Z
Learning: In the Artemis test framework, the AbstractArtemisIntegrationTest base class provides common MockitoSpyBean fields like instanceMessageSendService as protected fields, making them available to all test subclasses through inheritance.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java
📚 Learning: 2025-08-10T18:33:22.476Z
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java:3-9
Timestamp: 2025-08-10T18:33:22.476Z
Learning: In the Artemis test framework, `MockitoBean` from `org.springframework.test.context.bean.override.mockito` is the standard annotation used for mocking beans in test classes, not `MockBean`. This is used consistently across test base classes like `AbstractProgrammingIntegrationLocalCILocalVCTest`, `AbstractSpringIntegrationIndependentTest`, and `AbstractSpringIntegrationLocalVCSamlTest`. The project also uses `MockitoSpyBean` from the same package.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: julian-christl
Repo: ls1intum/Artemis PR: 8052
File: src/test/java/de/tum/in/www1/artemis/lecture/CompetencyIntegrationTest.java:310-310
Timestamp: 2024-06-10T19:44:09.116Z
Learning: Modifications to parameters in `competencyProgressUtilService.createCompetencyProgress` for debugging purposes are considered irrelevant to the test outcomes but helpful for clarity during debugging.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-08T08:50:28.791Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 11248
File: src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java:401-402
Timestamp: 2025-08-08T08:50:28.791Z
Learning: In src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java, method findStudentParticipationWithLatestSubmissionResultAndFeedbacksElseThrow(long), using List.of() for latestSubmission.setResults(...) is acceptable because the results list is not mutated afterward and is only returned to the client; no follow-up code appends to it.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: Strohgelaender
Repo: ls1intum/Artemis PR: 8574
File: src/main/java/de/tum/in/www1/artemis/service/tutorialgroups/TutorialGroupService.java:0-0
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `tryToFindMatchingUsers` method in `TutorialGroupService.java` has been updated to skip registrations without a student, enhancing the method's robustness. This change was implemented in commit `bef30f9751de0913143e8cb28cc0088264052261`.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java
📚 Learning: 2025-08-27T09:46:36.480Z
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java:1839-1839
Timestamp: 2025-08-27T09:46:36.480Z
Learning: In the Artemis codebase, when stubbing GitService.getBareRepository() in tests, it's valid to return a JGit Repository (org.eclipse.jgit.lib.Repository) even though the method signature returns the domain Repository (de.tum.cit.aet.artemis.programming.domain.Repository), because the domain Repository extends JGit's FileRepository, making them polymorphically compatible.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-10-08T15:35:48.767Z
Learnt from: jakubriegel
Repo: ls1intum/Artemis PR: 8050
File: src/test/java/de/tum/in/www1/artemis/plagiarism/PlagiarismUtilService.java:125-136
Timestamp: 2024-10-08T15:35:48.767Z
Learning: The `createTeamTextExerciseAndSimilarSubmissions` method in `PlagiarismUtilService.java` uses fixed inputs as it is designed to be a test helper method for simplifying the setup of team exercises and submissions in tests.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-06-15T04:13:22.541Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 10921
File: src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java:1334-1339
Timestamp: 2025-06-15T04:13:22.541Z
Learning: In Artemis ExamIntegrationTest, time difference assertions use ChronoUnit.MILLIS.between().isLessThan(1) without Math.abs() because the server only stores and retrieves timestamp values without calling now(), making differences predictable and consistent due to serialization/storage precision rather than timing variations.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java
📚 Learning: 2025-08-11T13:22:05.140Z
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/test/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingResourceImportTest.java:128-131
Timestamp: 2025-08-11T13:22:05.140Z
Learning: In the ExerciseSharingResourceImportTest class in Artemis, mockServer.verify() is not needed in the tearDown method as the test design doesn't require strict verification of all mocked HTTP expectations. The mockServer field is managed differently in this test class.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: jakubriegel
Repo: ls1intum/Artemis PR: 8050
File: src/test/java/de/tum/in/www1/artemis/plagiarism/PlagiarismUtilService.java:62-66
Timestamp: 2024-10-08T15:35:42.972Z
Learning: The `createCourseWithUsers` method in `PlagiarismUtilService.java` uses fixed inputs as it is designed to be a test helper method for simplifying the setup of courses and users in tests.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java
📚 Learning: 2025-09-20T16:47:54.380Z
Learnt from: MoritzSpengler
Repo: ls1intum/Artemis PR: 11382
File: src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizTrainingService.java:43-54
Timestamp: 2025-09-20T16:47:54.380Z
Learning: In QuizTrainingService.submitForTraining, cross-course mismatch protection is handled through PreAuthorize("hasAccessToCourse(#courseId)") authorization at the REST layer, ensuring users can only submit for courses they have access to, rather than through explicit courseId validation in the service method.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-09-25T20:28:36.905Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11419
File: src/main/java/de/tum/cit/aet/artemis/exam/domain/ExamUser.java:16-17
Timestamp: 2025-09-25T20:28:36.905Z
Learning: In the Artemis codebase, ExamUser entity uses ExamSeatDTO as a transient field for performance reasons. SamuelRoettgermann tested domain value objects but they caused 60x slower performance. This architectural exception is approved by maintainers due to significant performance benefits and Artemis naming convention requirements.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testSubmitStudentExam_differentUser` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-09-20T16:47:54.380Z
Learnt from: MoritzSpengler
Repo: ls1intum/Artemis PR: 11382
File: src/main/java/de/tum/cit/aet/artemis/quiz/service/QuizTrainingService.java:43-54
Timestamp: 2025-09-20T16:47:54.380Z
Learning: In QuizTrainingService.submitForTraining, cross-course validation is handled by the REST layer through authCheckService.checkHasAtLeastRoleInCourseElseThrow() which validates user access to the course before the service method is called, eliminating the need for additional courseId validation in the service layer.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-26T13:23:05.331Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 11318
File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java:549-552
Timestamp: 2025-08-26T13:23:05.331Z
Learning: The method findGradeScoresForAllExercisesForCourseAndStudent in StudentParticipationRepository handles both individual and team exercises by combining results from separate queries for individual grades, individual quiz grades, and team grades.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-06-17T12:31:09.178Z
Learnt from: jfr2102
Repo: ls1intum/Artemis PR: 10983
File: src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java:110-126
Timestamp: 2025-06-17T12:31:09.178Z
Learning: The query `findByExamIdWithEagerLatestLegalSubmissionsRatedResultAndIgnoreTestRunParticipation` in StudentParticipationRepository fetches all rated results (not just the latest) because the second correction round feature requires access to multiple assessment results per submission for proper correction round management.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-19T09:13:51.235Z
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitArchiveHelper.java:0-0
Timestamp: 2025-08-19T09:13:51.235Z
Learning: The domain Repository class (de.tum.cit.aet.artemis.programming.domain.Repository) extends JGit's FileRepository, making it polymorphically compatible with JGit APIs while providing additional Artemis-specific methods like getParticipation() and getLocalPath().
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-02-11T12:05:49.151Z
Learnt from: janthoXO
Repo: ls1intum/Artemis PR: 9406
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java:209-209
Timestamp: 2025-02-11T12:05:49.151Z
Learning: In ProgrammingExerciseParticipationResource, exam-related authorization checks and sensitive information filtering for results and feedbacks are handled by resultService.filterSensitiveInformationIfNecessary().
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-26T14:24:26.644Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 11334
File: src/main/java/de/tum/cit/aet/artemis/programming/repository/ProgrammingExerciseStudentParticipationRepository.java:103-114
Timestamp: 2025-08-26T14:24:26.644Z
Learning: The Artemis codebase has an existing index `idx_submission_participation_submission_date` on the `submission` table with columns `participation_id` and `submission_date DESC`. This index provides performance benefits for queries that filter submissions by participation and need ordering by submission date. The `submission` table stores all submission types including ProgrammingSubmission via single-table inheritance with discriminator values.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-10-22T21:31:54.240Z
Learnt from: Elfari1028
Repo: ls1intum/Artemis PR: 11491
File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java:376-378
Timestamp: 2025-10-22T21:31:54.240Z
Learning: In Artemis, ExerciseVersionService.createExerciseVersion(...) (src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseVersionService.java) eagerly re-fetches the target exercise (via type-specific findForVersioningById) before building the ExerciseSnapshotDTO. Controllers (e.g., ExerciseResource.toggleSecondCorrectionEnabled) do not need to reload the exercise before invoking createExerciseVersion.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-09-15T11:18:26.439Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-02-06T17:28:16.450Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 10265
File: src/main/resources/templates/dart/test/test/behavior_test.dart:57-67
Timestamp: 2025-02-06T17:28:16.450Z
Learning: In Artemis test files, reflection is intentionally used to bypass potential implementation errors in constructors and setters, allowing tests to produce meaningful results about core functionality even when students haven't implemented everything correctly.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java
📚 Learning: 2024-07-17T12:13:45.428Z
Learnt from: SimonEntholzer
Repo: ls1intum/Artemis PR: 8929
File: src/main/java/de/tum/in/www1/artemis/repository/ParticipationVCSAccessTokenRepository.java:27-29
Timestamp: 2024-07-17T12:13:45.428Z
Learning: The method naming convention `deleteByParticipation_id` in `ParticipationVCSAccessTokenRepository.java` is necessary due to specific framework or library constraints in the Artemis project.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-02-06T17:24:13.928Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 10265
File: src/main/resources/templates/dart/exercise/lib/bubble_sort.dart:1-1
Timestamp: 2025-02-06T17:24:13.928Z
Learning: In programming exercise template files (e.g., src/main/resources/templates/dart/exercise/*), implementations should be left empty or incomplete as they are meant to be completed by students as part of the exercise.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-05-11T22:58:13.489Z
Learnt from: theblobinthesky
Repo: ls1intum/Artemis PR: 10581
File: src/main/webapp/app/exercise/import/from-file/exercise-import-from-file.component.ts:57-60
Timestamp: 2025-05-11T22:58:13.489Z
Learning: The ProgrammingExerciseBuildConfig constructor initializes all fields with default values, including new fields like allowBranching (false) and branchRegex ('.*'), so explicit handling of these fields isn't needed when importing old exercise configurations.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-30T20:20:17.236Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11111
File: src/test/java/de/tum/cit/aet/artemis/exam/ExamRoomIntegrationTest.java:0-0
Timestamp: 2025-08-30T20:20:17.236Z
Learning: In ExamRoomIntegrationTest.validateAdminOverview(), the first assertion should use subset containment (contains) rather than exact equality because the admin overview shows all newest unique exam rooms in the system including those from previous uploads, not just rooms from the current upload being tested. The test only needs to verify that the expected rooms from the current upload are present in the response.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:2836-2846
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testAbandonStudentExamNotInTime` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:975-980
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testSubmitStudentExam_notInTime` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-10-27T11:50:42.955Z
Learnt from: bensofficial
Repo: ls1intum/Artemis PR: 9608
File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:202-203
Timestamp: 2024-10-27T11:50:42.955Z
Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, exception messages are used internally, and minor inconsistencies are acceptable due to performance considerations.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java
📚 Learning: 2025-09-01T13:47:02.624Z
Learnt from: Michael-Breu-UIbk
Repo: ls1intum/Artemis PR: 10989
File: src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.with-sharing.spec.ts:167-169
Timestamp: 2025-09-01T13:47:02.624Z
Learning: In Jest tests, prefer using jest.spyOn() over direct method assignment for mocking service methods, as spies are automatically restored by Jest's cleanup mechanisms (jest.restoreAllMocks()), while direct assignment bypasses this system and can lead to test pollution where mocked methods affect subsequent tests.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java
📚 Learning: 2025-08-14T21:24:50.201Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11111
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java:150-152
Timestamp: 2025-08-14T21:24:50.201Z
Learning: In the ExamRoomService.parseAndStoreExamRoomDataFromZipFile method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomService.java, IllegalArgumentException cannot be thrown in the try block, so only IOException needs to be caught when parsing ZIP files containing exam room JSON data.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-06-10T12:26:42.449Z
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java:108-117
Timestamp: 2025-06-10T12:26:42.449Z
Learning: In ProgrammingExerciseImportFromFileService, the current directory traversal logic for sharing imports (walking importExerciseDir before extraction) is working correctly in practice and should not be changed to more complex solutions, even if there are theoretical issues with the approach.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-30T22:03:22.332Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11111
File: src/main/java/de/tum/cit/aet/artemis/exam/web/admin/AdminExamResource.java:84-99
Timestamp: 2025-08-30T22:03:22.332Z
Learning: In ExamRoomService.parseAndStoreExamRoomDataFromZipFile, IOException is already properly caught and converted to BAD_REQUEST (400) response, which includes ZipException since it extends IOException. The error handling is appropriate for client-facing errors.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java
📚 Learning: 2024-10-31T20:40:39.930Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:9-12
Timestamp: 2024-10-31T20:40:39.930Z
Learning: In the Artemis project, files under the `exercise` directory are incomplete exercises intended to be completed by the student. TODO comments in these files are intentional and should not be implemented.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-10-31T20:40:30.235Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 9626
File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:3-4
Timestamp: 2024-10-31T20:40:30.235Z
Learning: In this project, files under the `exercise` directory are incomplete exercises to be completed by students. Therefore, review comments suggesting implementations in these files may not be necessary.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-09-25T11:25:54.261Z
Learnt from: Elfari1028
Repo: ls1intum/Artemis PR: 11351
File: src/main/java/de/tum/cit/aet/artemis/versioning/dto/QuizExerciseSnapshotDTO.java:52-53
Timestamp: 2025-09-25T11:25:54.261Z
Learning: In the Artemis versioning system, quiz questions processed by QuizExerciseSnapshotDTO should always have valid IDs (non-null getId()) since versioning works with persisted entities, making null filtering unnecessary according to Elfari1028.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-08T08:56:49.931Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 11248
File: src/main/java/de/tum/cit/aet/artemis/programming/web/repository/RepositoryResource.java:288-291
Timestamp: 2025-08-08T08:56:49.931Z
Learning: In the Artemis codebase, the LocalCI profile is only active when LocalVC is active (LocalCI ⇒ LocalVC). Therefore, in RepositoryResource.commitChanges, guarding the LocalVCServletService invocation with profileService.isLocalCIActive() is correct and guarantees the bean’s presence.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-08-11T13:21:25.531Z
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitRepositoryExportService.java:184-186
Timestamp: 2025-08-11T13:21:25.531Z
Learning: The `getBareRepository` method in `GitService` returns a domain `Repository` object (from `de.tum.cit.aet.artemis.programming.domain.Repository`), not a JGit `Repository` (from `org.eclipse.jgit.lib.Repository`).
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-06-15T13:49:43.096Z
Learnt from: ahmetsenturk
Repo: ls1intum/Artemis PR: 10916
File: src/main/java/de/tum/cit/aet/artemis/atlas/web/LearnerProfileResource.java:36-41
Timestamp: 2025-06-15T13:49:43.096Z
Learning: In the Artemis codebase, the development team has decided to allow direct injection of repositories into REST resources rather than always delegating to service layers. This architectural decision was communicated and decided with peer developers.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:2804-2810
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `postWithoutLocation` method used in the `testAbandonStudentExam` test case already checks the response status, ensuring that the abandonment of the exam is accepted.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-01-25T17:22:31.410Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 10202
File: src/main/resources/templates/ruby/test/test/test_structural.rb:5-18
Timestamp: 2025-01-25T17:22:31.410Z
Learning: When reviewing exercise templates, avoid adding tests that would pass immediately with the starter code. Tests should verify the actual implementation that students need to complete, not just the presence of classes and methods defined in the template.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
📚 Learning: 2025-01-09T06:36:04.666Z
Learnt from: chrisknedl
Repo: ls1intum/Artemis PR: 10118
File: src/main/resources/templates/aeolus/java/plain_maven_blackbox.sh:33-41
Timestamp: 2025-01-09T06:36:04.666Z
Learning: In the Artemis build agent's Aeolus templates for Java/Maven blackbox tests, the existence of tool directories and config files is manually verified before script execution, making runtime existence checks redundant.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java
🧬 Code graph analysis (3)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java (1)
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
RepositoryExportTestUtil(38-331)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java (3)
src/main/java/de/tum/cit/aet/artemis/programming/domain/VcsRepositoryUri.java (1)
VcsRepositoryUri(11-180)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
LocalVCRepositoryUri(17-318)src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
RepositoryExportTestUtil(38-331)
src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1)
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
RepositoryExportTestUtil(38-331)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (2)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java (1)
98-100: Consider localizing the spy bean or adding explicit reset.The class-level
@MockitoSpyBeanmeans every test method interacts with the spy, even though the comment indicates it should only be used for "simulating non-feasible failure scenarios." This increases the risk of test pollution if stubs from one test (e.g., line 1052'sdoThrow) are not properly cleaned up before the next test runs.Consider either:
- Moving the spy bean to the specific test method that needs it (if framework supports it), or
- Adding a
@BeforeEachor@AfterEachmethod to reset the spy usingMockito.reset(gitServiceSpy).⛔ Skipped due to learnings
Learnt from: Wallenstein61 Repo: ls1intum/Artemis PR: 10989 File: src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java:3-9 Timestamp: 2025-08-10T18:33:22.476Z Learning: In the Artemis test framework, `MockitoBean` from `org.springframework.test.context.bean.override.mockito` is the standard annotation used for mocking beans in test classes, not `MockBean`. This is used consistently across test base classes like `AbstractProgrammingIntegrationLocalCILocalVCTest`, `AbstractSpringIntegrationIndependentTest`, and `AbstractSpringIntegrationLocalVCSamlTest`. The project also uses `MockitoSpyBean` from the same package.Learnt from: SamuelRoettgermann Repo: ls1intum/Artemis PR: 11378 File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16 Timestamp: 2025-09-15T11:18:26.439Z Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.Learnt from: bassner Repo: ls1intum/Artemis PR: 10705 File: src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideUnhideServiceTest.java:54-57 Timestamp: 2025-07-07T11:43:11.736Z Learning: In the Artemis test framework, the AbstractArtemisIntegrationTest base class provides common MockitoSpyBean fields like instanceMessageSendService as protected fields, making them available to all test subclasses through inheritance.Learnt from: magaupp Repo: ls1intum/Artemis PR: 9751 File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148 Timestamp: 2024-11-26T20:43:17.588Z Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.Learnt from: ekayandan Repo: ls1intum/Artemis PR: 11027 File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java:1839-1839 Timestamp: 2025-08-27T09:46:36.480Z Learning: In the Artemis codebase, when stubbing GitService.getBareRepository() in tests, it's valid to return a JGit Repository (org.eclipse.jgit.lib.Repository) even though the method signature returns the domain Repository (de.tum.cit.aet.artemis.programming.domain.Repository), because the domain Repository extends JGit's FileRepository, making them polymorphically compatible.Learnt from: bensofficial Repo: ls1intum/Artemis PR: 9608 File: src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java:202-203 Timestamp: 2024-10-27T11:50:42.955Z Learning: In `src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIService.java`, exception messages are used internally, and minor inconsistencies are acceptable due to performance considerations.Learnt from: magaupp Repo: ls1intum/Artemis PR: 9626 File: src/main/resources/templates/c_sharp/exercise/BubbleSort.cs:3-4 Timestamp: 2024-10-31T20:40:30.235Z Learning: In this project, files under the `exercise` directory are incomplete exercises to be completed by students. Therefore, review comments suggesting implementations in these files may not be necessary.Learnt from: magaupp Repo: ls1intum/Artemis PR: 8802 File: src/main/resources/templates/rust/exercise/src/context.rs:1-1 Timestamp: 2024-10-08T15:35:42.972Z Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.Learnt from: magaupp Repo: ls1intum/Artemis PR: 8802 File: src/main/resources/templates/rust/exercise/src/context.rs:1-1 Timestamp: 2024-08-05T00:11:50.650Z Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.Learnt from: Michael-Breu-UIbk Repo: ls1intum/Artemis PR: 10989 File: src/main/webapp/app/programming/manage/detail/programming-exercise-detail.component.with-sharing.spec.ts:167-169 Timestamp: 2025-09-01T13:47:02.624Z Learning: In Jest tests, prefer using jest.spyOn() over direct method assignment for mocking service methods, as spies are automatically restored by Jest's cleanup mechanisms (jest.restoreAllMocks()), while direct assignment bypasses this system and can lead to test pollution where mocked methods affect subsequent tests.src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java (1)
525-528:testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir: modernized failure injection and signature updateSwitching from static
Filesmocking to stubbingzipFileService.createTemporaryZipFile(any(Path.class), anyList(), anyLong())is cleaner and closer to the production abstraction, and the updated callprogrammingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, false, false);correctly reflects the new helper signature (status + three boolean flags).
No issues here; this should robustly simulate the “temp ZIP creation fails” scenario.
...ava/de/tum/cit/aet/artemis/programming/ProgrammingExerciseLocalVCJenkinsIntegrationTest.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
356-363: Align student bare-repo slug with LocalVC slug conventions
deleteStudentBareRepocurrently builds the slug asexercise.getShortName() + "-" + username. Other LocalVC helpers (e.g.LocalVCRepositoryUriandLocalVCLocalCITestService) appear to follow a project-key–based slug scheme. If the actual student repo slug is derived differently (e.g. fromprojectKey), this method may silently delete the wrong path or nothing at all.Consider delegating slug construction to the same logic used when creating the student repo (e.g. via
LocalVCLocalCITestService.getRepositorySlug(projectKey, username)or by mirroring its pattern), so cleanup can’t drift from creation logic.src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java (1)
448-477: Optional: reuse RepositoryExportTestUtil for seeding README.md contentThe manual steps to remove
test.txt, writeREADME.md, commit, and push are correct but could be slightly simplified by reusing the new helper methods (e.g.writeFilesAndPush) if you want to reduce duplication across tests in future:- Path readme = sourceRepo.workingCopyGitRepoFile.toPath().resolve("README.md"); - FileUtils.writeStringToFile(readme.toFile(), "Initial commit", java.nio.charset.StandardCharsets.UTF_8); - sourceRepo.workingCopyGitRepo.add().addFilepattern(".").call(); - GitService.commit(sourceRepo.workingCopyGitRepo).setMessage("Initial commit").call(); - sourceRepo.workingCopyGitRepo.push().setRemote("origin").call(); + RepositoryExportTestUtil.writeFilesAndPush(sourceRepo, Map.of("README.md", "Initial commit"), "Initial commit");Not required for correctness, but may help keep future tests DRY.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java(8 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/**/*.java
⚙️ CodeRabbit configuration file
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
Files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java
🧠 Learnings (21)
📓 Common learnings
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-08-05T00:11:50.650Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitService.java:1455-1465
Timestamp: 2025-08-11T12:57:51.535Z
Learning: In the Artemis project, when ekayandan responds with "same as above" to a code review suggestion, it means they want to defer the suggested change to a follow-up PR to keep the current PR scope minimal and focused on the core functionality being implemented.
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java:108-117
Timestamp: 2025-06-10T12:26:42.449Z
Learning: In ProgrammingExerciseImportFromFileService, the current directory traversal logic for sharing imports (walking importExerciseDir before extraction) is working correctly in practice and should not be changed to more complex solutions, even if there are theoretical issues with the approach.
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Learnt from: chrisknedl
Repo: ls1intum/Artemis PR: 10118
File: src/main/resources/templates/aeolus/java/plain_maven_blackbox.sh:33-41
Timestamp: 2025-01-09T06:36:04.666Z
Learning: In the Artemis build agent's Aeolus templates for Java/Maven blackbox tests, the existence of tool directories and config files is manually verified before script execution, making runtime existence checks redundant.
📚 Learning: 2024-11-26T20:43:17.588Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 9751
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java:143-148
Timestamp: 2024-11-26T20:43:17.588Z
Learning: In `src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseFactory.java`, the test package name assigned in `populateUnreleasedProgrammingExercise` does not need to adhere to naming conventions.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java
📚 Learning: 2025-08-11T13:22:05.140Z
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/test/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingResourceImportTest.java:128-131
Timestamp: 2025-08-11T13:22:05.140Z
Learning: In the ExerciseSharingResourceImportTest class in Artemis, mockServer.verify() is not needed in the tearDown method as the test design doesn't require strict verification of all mocked HTTP expectations. The mockServer field is managed differently in this test class.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-10-08T15:35:42.972Z
Learnt from: magaupp
Repo: ls1intum/Artemis PR: 8802
File: src/main/resources/templates/rust/exercise/src/context.rs:1-1
Timestamp: 2024-10-08T15:35:42.972Z
Learning: Code inside the `exercise` directories in the Artemis platform is provided to students and is intended for them to implement. TODO comments in these directories are meant to guide students and should not be addressed in the PR.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/main/java/de/tum/in/www1/artemis/service/exam/StudentExamService.java:295-303
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `abandonStudentExam` method in `StudentExamService` should not include a null check for the `studentExam` parameter as per the project's coding practices. It is expected that the `studentExam` will never be null at this point in the code, and a `NullPointerException` would indicate a significant issue elsewhere in the codebase.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-06-15T04:13:22.541Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 10921
File: src/test/java/de/tum/cit/aet/artemis/exam/ExamIntegrationTest.java:1334-1339
Timestamp: 2025-06-15T04:13:22.541Z
Learning: In Artemis ExamIntegrationTest, time difference assertions use ChronoUnit.MILLIS.between().isLessThan(1) without Math.abs() because the server only stores and retrieves timestamp values without calling now(), making differences predictable and consistent due to serialization/storage precision rather than timing variations.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-08-26T13:23:05.331Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 11318
File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java:549-552
Timestamp: 2025-08-26T13:23:05.331Z
Learning: The method findGradeScoresForAllExercisesForCourseAndStudent in StudentParticipationRepository handles both individual and team exercises by combining results from separate queries for individual grades, individual quiz grades, and team grades.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-08-08T08:50:28.791Z
Learnt from: tobias-lippert
Repo: ls1intum/Artemis PR: 11248
File: src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java:401-402
Timestamp: 2025-08-08T08:50:28.791Z
Learning: In src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseParticipationService.java, method findStudentParticipationWithLatestSubmissionResultAndFeedbacksElseThrow(long), using List.of() for latestSubmission.setResults(...) is acceptable because the results list is not mutated afterward and is only returned to the client; no follow-up code appends to it.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-02-11T12:05:49.151Z
Learnt from: janthoXO
Repo: ls1intum/Artemis PR: 9406
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ProgrammingExerciseParticipationResource.java:209-209
Timestamp: 2025-02-11T12:05:49.151Z
Learning: In ProgrammingExerciseParticipationResource, exam-related authorization checks and sensitive information filtering for results and feedbacks are handled by resultService.filterSensitiveInformationIfNecessary().
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-09-15T11:18:26.439Z
Learnt from: SamuelRoettgermann
Repo: ls1intum/Artemis PR: 11378
File: src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java:13-16
Timestamp: 2025-09-15T11:18:26.439Z
Learning: In ExamRoomDistributionService.distributeRegisteredStudents method in src/main/java/de/tum/cit/aet/artemis/exam/service/ExamRoomDistributionService.java, the team has decided not to use Transactional annotation despite multiple repository operations, based on human reviewer consultation.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-10-22T21:31:54.240Z
Learnt from: Elfari1028
Repo: ls1intum/Artemis PR: 11491
File: src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseResource.java:376-378
Timestamp: 2025-10-22T21:31:54.240Z
Learning: In Artemis, ExerciseVersionService.createExerciseVersion(...) (src/main/java/de/tum/cit/aet/artemis/exercise/service/ExerciseVersionService.java) eagerly re-fetches the target exercise (via type-specific findForVersioningById) before building the ExerciseSnapshotDTO. Controllers (e.g., ExerciseResource.toggleSecondCorrectionEnabled) do not need to reload the exercise before invoking createExerciseVersion.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-06-17T12:31:09.178Z
Learnt from: jfr2102
Repo: ls1intum/Artemis PR: 10983
File: src/main/java/de/tum/cit/aet/artemis/exercise/repository/StudentParticipationRepository.java:110-126
Timestamp: 2025-06-17T12:31:09.178Z
Learning: The query `findByExamIdWithEagerLatestLegalSubmissionsRatedResultAndIgnoreTestRunParticipation` in StudentParticipationRepository fetches all rated results (not just the latest) because the second correction round feature requires access to multiple assessment results per submission for proper correction round management.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-10-08T15:35:48.767Z
Learnt from: jakubriegel
Repo: ls1intum/Artemis PR: 8050
File: src/test/java/de/tum/in/www1/artemis/plagiarism/PlagiarismUtilService.java:125-136
Timestamp: 2024-10-08T15:35:48.767Z
Learning: The `createTeamTextExerciseAndSimilarSubmissions` method in `PlagiarismUtilService.java` uses fixed inputs as it is designed to be a test helper method for simplifying the setup of team exercises and submissions in tests.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-10-08T15:35:48.768Z
Learnt from: SimonEntholzer
Repo: ls1intum/Artemis PR: 8929
File: src/main/java/de/tum/in/www1/artemis/repository/ParticipationVCSAccessTokenRepository.java:27-29
Timestamp: 2024-10-08T15:35:48.768Z
Learning: The method naming convention `deleteByParticipation_id` in `ParticipationVCSAccessTokenRepository.java` is necessary due to specific framework or library constraints in the Artemis project.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-08-19T09:13:51.235Z
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/main/java/de/tum/cit/aet/artemis/programming/service/GitArchiveHelper.java:0-0
Timestamp: 2025-08-19T09:13:51.235Z
Learning: The domain Repository class (de.tum.cit.aet.artemis.programming.domain.Repository) extends JGit's FileRepository, making it polymorphically compatible with JGit APIs while providing additional Artemis-specific methods like getParticipation() and getLocalPath().
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.javasrc/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java
📚 Learning: 2025-08-27T09:46:36.480Z
Learnt from: ekayandan
Repo: ls1intum/Artemis PR: 11027
File: src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java:1839-1839
Timestamp: 2025-08-27T09:46:36.480Z
Learning: In the Artemis codebase, when stubbing GitService.getBareRepository() in tests, it's valid to return a JGit Repository (org.eclipse.jgit.lib.Repository) even though the method signature returns the domain Repository (de.tum.cit.aet.artemis.programming.domain.Repository), because the domain Repository extends JGit's FileRepository, making them polymorphically compatible.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:988-993
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testSubmitStudentExam_differentUser` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:2836-2846
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testAbandonStudentExamNotInTime` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2024-06-10T19:44:09.116Z
Learnt from: valentin-boehm
Repo: ls1intum/Artemis PR: 7384
File: src/test/java/de/tum/in/www1/artemis/exam/StudentExamIntegrationTest.java:975-980
Timestamp: 2024-06-10T19:44:09.116Z
Learning: The `testSubmitStudentExam_notInTime` method does not require additional checks to verify the state of `studentExam1` after receiving a `HttpStatus.FORBIDDEN` because the control flow in the `StudentExamResource` is straightforward and ensures no state change occurs.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-06-06T14:47:54.300Z
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java:122-135
Timestamp: 2025-06-06T14:47:54.300Z
Learning: In Artemis sharing platform integration (under "sharing" profile), endpoints in ExerciseSharingResource use checksum-based authentication with shared secrets instead of PreAuthorize annotations. These endpoints (like getProblemStatement, getExerciseDetails, loadShoppingBasket) validate security through sharingInfo.checkChecksum() calls rather than role-based access control, as they serve as API interfaces for external sharing platform communication.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java
📚 Learning: 2025-06-10T12:26:42.449Z
Learnt from: Wallenstein61
Repo: ls1intum/Artemis PR: 10989
File: src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java:108-117
Timestamp: 2025-06-10T12:26:42.449Z
Learning: In ProgrammingExerciseImportFromFileService, the current directory traversal logic for sharing imports (walking importExerciseDir before extraction) is working correctly in practice and should not be changed to more complex solutions, even if there are theoretical issues with the approach.
Applied to files:
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java
🧬 Code graph analysis (2)
src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/LocalVCRepositoryUri.java (1)
LocalVCRepositoryUri(17-318)src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
RepositoryExportTestUtil(42-399)
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
src/main/webapp/app/programming/shared/entities/programming-exercise.model.ts (1)
ProgrammingExercise(47-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build linux/arm64 Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build linux/amd64 Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-style
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
- GitHub Check: bean-instantiation-check
🔇 Additional comments (4)
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java (1)
44-107: Lifecycle tracking + automatic cleanup look solidThe ThreadLocal-based
trackRepository/cleanupTrackedRepositoriesdesign is a good fit for JUnit 5 parallel runs and nicely centralizesLocalRepositorycleanup, especially with the auto-tracking in the seeding helpers. This directly mitigates the LocalRepository leak risk mentioned in the PR objectives while keeping tests simple.src/test/java/de/tum/cit/aet/artemis/athena/AthenaResourceIntegrationTest.java (3)
113-120: Good integration of LocalVC helpers with centralized cleanupAutowiring
LocalVCLocalCITestService/ participation helpers and introducing@AfterEach cleanupRepositories()that delegates toRepositoryExportTestUtil.cleanupTrackedRepositories()nicely ensures allLocalRepositoryinstances registered viatrackRepository(or the seeding helpers) are consistently cleaned up after each test. This addresses the earlier LocalRepository resource-leak concern without complicating individual tests.Also applies to: 177-180
417-446: Repository export setup now uses tracked LocalRepository and LocalVC TESTS slugSwitching to
RepositoryExportTestUtil.trackRepository(new LocalRepository(defaultBranch))and deriving the tests repo slug viaRepositoryType.TESTS.getName()before wiring the LocalVC URI keeps the setup consistent with the new LocalVC-based test utilities, while ensuring the source repo is included in the automatic cleanup. The updated wiring and subsequentcopyBareRepositoryWithoutHistorycalls look coherent.
448-495: Student repository export test is thorough and aligns with LocalVC utilitiesThe new
testStudentRepositoryExportEndpoint:
- Creates a real programming participation + submission via
ParticipationUtilService.- Seeds a LocalVC repo (
athena-src) with a singleREADME.mdcommit and pushes it.- Clones that into a LocalVC student bare repo via
RepositoryExportTestUtil.seedLocalVcBareFrom(...).- Persists the correct LocalVC repository URI on the student participation.
- Calls the internal Athena export endpoint and asserts the exact file map.
Combined with the
@AfterEachcleanup that handles both repos via tracking, this gives solid end-to-end coverage of the student-repo export path without leaking resources.
src/test/java/de/tum/cit/aet/artemis/programming/util/RepositoryExportTestUtil.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java (1)
354-359: Based on the verification, Linux filesystems are case-sensitive and Git is case-sensitive. The review comment correctly identifies a real issue: Linux treats files/directories with different casing as completely different files/directories.From the codebase analysis:
- LocalVCRepositoryUri stores and uses the projectKey directly in filesystem path construction (line 50)
- Test files consistently apply
.toUpperCase()on projectKey (6+ locations)- The ParticipationUtilService line 354 calls
.toLowerCase(), diverging from this patternThis will create separate repository directories on Linux, breaking consistency and causing duplicate/orphaned repositories.
Apply uppercase normalization to match codebase conventions.
Using
.toUpperCase()ensures consistency with test utilities and other LocalVC call sites, preventing case-mismatched repository paths.- var localVcRepoUri = new LocalVCRepositoryUri(localVCBaseUri, exercise.getProjectKey().toLowerCase(), repoName); + var localVcRepoUri = new LocalVCRepositoryUri(localVCBaseUri, exercise.getProjectKey().toUpperCase(), repoName);src/test/java/de/tum/cit/aet/artemis/programming/RepositoryIntegrationTest.java (2)
827-859: Potential resource leak: temp directory created outside try block.The temp directory is created at line 828 before the try-with-resources block begins at line 829. If an exception occurs between these lines (e.g., from
new LocalVCRepositoryUri()), the directory won't be cleaned up by the finally block.Consider moving the directory creation inside the try block or wrapping the entire sequence:
- Path remoteClonePath = Files.createTempDirectory("repositoryintegration-remote-clone"); - try (Repository remoteRepository = gitService.getOrCheckoutRepositoryWithLocalPath(repositoryUri, remoteClonePath, true, true); - Git remoteGit = Git.wrap(remoteRepository)) { + Path remoteClonePath = null; + try { + remoteClonePath = Files.createTempDirectory("repositoryintegration-remote-clone"); + try (Repository remoteRepository = gitService.getOrCheckoutRepositoryWithLocalPath(repositoryUri, remoteClonePath, true, true); + Git remoteGit = Git.wrap(remoteRepository)) { + // ... test code ... + } + } + finally { + if (remoteClonePath != null) { + FileUtils.deleteQuietly(remoteClonePath.toFile()); + } } - finally { - FileUtils.deleteQuietly(remoteClonePath.toFile()); - }
866-922: Potential resource leak: temp directory created outside try block.Same issue as in
testPullChanges- the temp directory is created at line 867 before the try-with-resources block at line 868.Apply the same fix pattern as suggested for
testPullChangesto ensure the temp directory is always cleaned up.
♻️ Duplicate comments (4)
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (1)
2227-2227: Remove overly broadExceptionfrom throws clause.Declaring a bare
Exceptionalongside specific checked exceptions defeats the purpose of Java's checked exception system and is considered poor practice.Either remove
Exceptionentirely if the three specific exceptions (IOException,GitAPIException,URISyntaxException) cover all cases:-private void setupExerciseForExport() throws IOException, GitAPIException, URISyntaxException, Exception { +private void setupExerciseForExport() throws IOException, GitAPIException, URISyntaxException {Or replace it with the specific checked exception type that the method body actually throws (inspect the method implementation to identify what else is thrown beyond the three already declared).
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingSubmissionIntegrationTest.java (1)
773-779: The past review concern about LocalRepository cleanup is already addressed.The
seedStudentRepositoryForParticipationcall on line 774 internally registers the repository for automatic cleanup (per utility class design). ThecleanupTrackedRepositories()call in tearDown (line 102) handles all tracked repositories, including those created by this helper. No additional manual tracking is needed.src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseGitIntegrationTest.java (1)
96-132: Close checkedOut Repository and clean targetPath to prevent resource leaks.The test creates a checked-out repository and target directory that are never explicitly cleaned:
checkedOut Repository: Returned from
getOrCheckoutRepositoryWithTargetPath()(line 115), it holds git file locks ontargetPath. Must callcloseBeforeDelete()before the test ends.targetPath directory: Created by the checkout operation but never deleted. Since
tempPathis configuration-managed (not JUnit's @tempdir), subdirectories must be cleaned explicitly.The
remoteRepocleanup viatrackRepository()+@AfterEachis already correct and requires no change.Add a try-finally block to guarantee cleanup of the checked-out repository:
// Build the LocalVC URI and checkout to a separate target path LocalVCRepositoryUri repoUri = new LocalVCRepositoryUri(localVCLocalCITestService.buildLocalVCUri(null, null, projectKey, repoSlug)); Path targetPath = tempPath.resolve("lcvc-checkout").resolve("student-checkout"); -var checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true); - -// Verify we can fetch and read last commit hash from the remote -gitService.fetchAll(checkedOut); -var lastHash = gitService.getLastCommitHash(repoUri); -assertThat(lastHash).as("last commit hash should exist on remote").isNotNull().isInstanceOf(ObjectId.class); - -// Create a local change, commit and push via GitService -var localFile = targetPath.resolve("hello.txt"); -Files.createDirectories(localFile.getParent()); -FileUtils.writeStringToFile(localFile.toFile(), "hello world", java.nio.charset.StandardCharsets.UTF_8); -gitService.stageAllChanges(checkedOut); -gitService.commitAndPush(checkedOut, "Add hello.txt", true, null); - -// Pull and reset operations should not throw -gitService.pullIgnoreConflicts(checkedOut); -gitService.resetToOriginHead(checkedOut); +var checkedOut = gitService.getOrCheckoutRepositoryWithTargetPath(repoUri, targetPath, true, true); +try { + // Verify we can fetch and read last commit hash from the remote + gitService.fetchAll(checkedOut); + var lastHash = gitService.getLastCommitHash(repoUri); + assertThat(lastHash).as("last commit hash should exist on remote").isNotNull().isInstanceOf(ObjectId.class); + + // Create a local change, commit and push via GitService + var localFile = targetPath.resolve("hello.txt"); + Files.createDirectories(localFile.getParent()); + FileUtils.writeStringToFile(localFile.toFile(), "hello world", java.nio.charset.StandardCharsets.UTF_8); + gitService.stageAllChanges(checkedOut); + gitService.commitAndPush(checkedOut, "Add hello.txt", true, null); + + // Pull and reset operations should not throw + gitService.pullIgnoreConflicts(checkedOut); + gitService.resetToOriginHead(checkedOut); +} +finally { + checkedOut.closeBeforeDelete(); + RepositoryExportTestUtil.safeDeleteDirectory(targetPath); +}src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1)
576-616: Track repositories created in setupExerciseForExport to prevent leaks.The three
createAndConfigureLocalRepository(...)calls at lines 611–613 returnLocalRepositoryobjects but are not tracked, so they won't be cleaned up bycleanupTrackedRepositories().- localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, templateRepositorySlug); - localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, solutionRepositorySlug); - localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, testsRepositorySlug); + RepositoryExportTestUtil.trackRepository(localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, templateRepositorySlug)); + RepositoryExportTestUtil.trackRepository(localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, solutionRepositorySlug)); + RepositoryExportTestUtil.trackRepository(localVCLocalCITestService.createAndConfigureLocalRepository(projectKey, testsRepositorySlug));
🧹 Nitpick comments (10)
src/test/java/de/tum/cit/aet/artemis/exam/StudentExamIntegrationTest.java (1)
237-240: Consider adding documentation for the commit hash maps.These maps track participation commit hashes for verification in programming exercise submission tests. A brief JavaDoc comment would improve readability:
+ // Maps participation ID to commit hashes for verifying programming submissions + // initialCommitHashes: first commit after participation setup + // updatedCommitHashes: commit after subsequent changes private final Map<Long, String> programmingInitialCommitHashes = new HashMap<>(); private final Map<Long, String> programmingUpdatedCommitHashes = new HashMap<>();src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseIntegrationTestService.java (1)
431-432: Remove redundant file deletions.The
Files.deleteIfExists()calls on lines 431-432 are redundant becauseprojectFilePathandpomPathreside insiderepo1.workingCopyGitRepoFile.toPath()(as seen on lines 404-405). WhencleanupTrackedRepositories()runs intearDown()(line 292), it invokesresetLocalRepo()on all tracked repositories, which deletes entire repository directories—including these files.Remove the explicit file deletions:
String modifiedPom = Files.readString(repoRoot.resolve("pom.xml")); assertThat(modifiedPom).contains((userPrefix + "student1").toLowerCase()); - Files.deleteIfExists(projectFilePath); - Files.deleteIfExists(pomPath); }The cleanup will still occur automatically in
tearDown().src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.java (1)
260-260: Verify disabled tests are tracked for re-enablement.Five tests are currently disabled (
testCommitChanges,testSaveFilesAndCommit,testPullChanges,testResetToLastCommit,testGetStatus). These cover critical Git operations and represent significant test coverage.Ensure these tests are tracked in an issue or task list for re-enablement after the LocalVC migration is complete.
Would you like me to help generate updated implementations for these disabled tests, or is there already a tracking issue for this work?
Also applies to: 296-296, 327-327, 362-362, 418-418
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseGitIntegrationTest.java (1)
117-120: Optional: Consider verifying commit hash value.The current assertion confirms
lastHashis non-null and of typeObjectId, which validates the basic functionality. For stricter verification, you could compare it against the actual HEAD commit fromremoteRepo.workingCopyGitRepo.Example enhancement (not required for this PR):
// Get expected hash from remote ObjectId expectedHash = remoteRepo.workingCopyGitRepo.getRepository().resolve("HEAD"); assertThat(lastHash).as("last commit hash should match remote HEAD").isEqualTo(expectedHash);src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseParticipationIntegrationTest.java (5)
112-115: Repository cleanup strategy properly addresses LocalRepository resource leaksCalling
RepositoryExportTestUtil.cleanupTrackedRepositories()in@AfterEachtogether with consistently wrapping new LocalVC repos inRepositoryExportTestUtil.trackRepository(...)(e.g. inensureAuxiliaryRepositoryConfiguredandensureLocalVcRepositoryExists) ensures LocalRepository instances and their directories are reset and removed after each test, preventing disk bloat and cross-test interference.If this cleanup pattern is used in multiple classes, consider a small shared base/helper in the test hierarchy to avoid duplication.
Also applies to: 944-952, 1151-1152
875-895: Seeding all base and auxiliary repositories with real commits is a solid improvementThe
setup()inGetCommitHistoryForTemplateSolutionTestOrAuxReponow wires LocalVC repos viacreateAndWireBaseRepositories, commits distinct messages to template/solution/tests, and commits to a properly configured auxiliary repo. This removes fragile stubbing and gives the commit-history endpoint realistic data to work with.You could centralize the repeated “commit message constants + initial commit” pattern into a small helper if more tests need similar seeding, but this is not urgent.
Also applies to: 890-895
981-1012: Commit-details view tests comprehensively cover student/template/solution sourcesThe nested
GetParticipationRepositoryFilesForCommitsDetailsViewsetup seeds:
- a student participation repo,
- the template repo, and
- the solution repo
with distinct commits and then verifies the
files-content-commit-detailsendpoint for each parameter combination. This eliminates the previous reliance on mocks and accurately tests how the controller interacts with LocalVC.If more repositories (e.g. auxiliary) are later added to this view, consider extracting a small shared seeding utility to keep setup concise.
Also applies to: 1020-1039
1122-1153: LocalVC commit helpers encapsulate URI handling and repository bootstrapping wellThe overloads of
commitToRepository(...):
- Normalize any
VcsRepositoryUrito aLocalVCRepositoryUri, throwing a clearIllegalStateExceptionif the URI is missing.- Ensure the local bare repo exists (creating and tracking it if needed) via
ensureLocalVcRepositoryExists.- Delegate to a single implementation that works on
LocalVCRepositoryUri.This keeps LocalVC assumptions localized in one place and makes higher-level tests concise.
If other test classes need the same behavior, consider moving these helpers to
RepositoryExportTestUtilor a dedicated LocalVC test helper to avoid duplication.
1154-1172: writeFilesAndPush correctly manages working copies and resources
writeFilesAndPush:
- clones from the bare repo using a
file://URI into a temp directory undertempPath,- writes all requested files, commits with the supplied message via
GitService.commit(git), and pushes, and- cleans up the working copy in a
finallyblock usingRepositoryExportTestUtil.safeDeleteDirectory.Resource usage (JGit handle + temp directories) is properly bounded; no leaks are apparent.
You might consider reusing any existing
writeFilesAndPush(LocalRepository, ...)helper fromRepositoryExportTestUtilin the future to avoid slightly similar commit logic in multiple places, but this is optional.src/test/java/de/tum/cit/aet/artemis/programming/util/ProgrammingExerciseTestService.java (1)
361-394: Optional: rely solely on centralized cleanup to reduce duplication.cleanupTrackedRepositories() already resets tracked repos. The per-field reset blocks duplicate the work and add noise.
Option:
- Keep cleanupTrackedRepositories() and remove the explicit per-field resets (or guard them behind a debug flag).
src/test/java/de/tum/cit/aet/artemis/exercise/participation/util/ParticipationUtilService.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/ProgrammingExerciseGitIntegrationTest.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/programming/TestRepositoryResourceIntegrationTest.java
Show resolved
Hide resolved
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
Checklist
General
Motivation and Context
The programming exercise integration tests previously relied on broad GitService mocks in base test classes. This led to fragile behavior (e.g. NotAMockException resets, unreliable plagiarism checks, and unrealistic repository state). The PR migrates these tests to use real LocalVC-backed repositories to improve reliability, better reflect production behavior, and unblock further work on export and plagiarism robustness. A single localized SpyBean override is retained for one JPlag ZIP test to ensure parsable content without reintroducing global mocking.
Description
Removed GitService mock usage from shared/base integration test setup; now autowiring the real bean.
Refactored plagiarism preparation (prepareTwoSubmissionsForPlagiarismChecks) to seed actual LocalVC repositories with committed Java sources instead of Mockito stubbing.
Fixed teardown instability by removing invalid Mockito reset of a real bean (NotAMockException).
Added localized @SpyBean override only inside ProgrammingExerciseIntegrationJenkinsLocalVCTest to inject minimal Java code for getOrCheckoutRepositoryForJPlag, avoiding base-class pollution.
Ensured repository content is written and committed before plagiarism checks to satisfy JPlag token parsing.
Preserved existing test semantics (authorization, export flows, timeline updates) while improving determinism.
Minor cleanup and defensive exception handling around repository seeding utilities.
Review Progress
Code Review
Test Coverage
Summary by CodeRabbit
Bug Fixes
API Changes
Tests