Conversation
#12769 introduced an @AfterEach cleanupPlatforms() in Lti13InitiationIntegrationTest that calls ltiPlatformConfigurationRepository.deleteAll(). The change was intended to prevent LtiPlatformConfiguration rows from leaking into later LTI test classes — but the leakage was actually load-bearing: LtiIntegrationTest.getAllConfiguredLtiPlatformsAsAdmin and updateLtiPlatformConfigurationAsAdmin do platform.setId(1L); save(...), which Spring Data JPA treats as merge() because the entity ID is set, and merge() requires the row to exist. Without an existing row with id=1, the UPDATE matches zero rows and Hibernate throws ObjectOptimisticLockingFailureException. The pre-existing tests relied on the fact that earlier LTI test classes (alphabetically Lti13InitiationIntegrationTest first) inserted rows with auto-generated IDs starting at 1. The new cleanup wiped them between classes and broke this implicit ordering dependency. Drop the cleanup and document the dependency in a comment. Intra-class isolation is still preserved by the UUID-keyed registrationIds. The proper long-term fix is to make LtiIntegrationTest self-contained (test-local data fixtures), which is out of scope here.
github-actions
Bot
added
tests
lti
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe PR removes per-test database cleanup from ChangesTest cleanup removal
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores stability of the server-side LTI test suite on develop by removing a newly introduced per-test database cleanup in Lti13InitiationIntegrationTest that unintentionally broke other LTI integration tests running in the same JVM.
Changes:
- Removed the
@AfterEachcleanup that calledltiPlatformConfigurationRepository.deleteAll(). - Dropped the now-unused
org.junit.jupiter.api.AfterEachimport. - Added an explanatory comment documenting why cleanup is intentionally omitted to avoid breaking dependent LTI tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+43
to
+47
| // No @AfterEach cleanup: each test uses a UUID-keyed registrationId so intra-class collisions are impossible, | ||
| // and LtiIntegrationTest.getAllConfiguredLtiPlatformsAsAdmin / updateLtiPlatformConfigurationAsAdmin implicitly | ||
| // rely on auto-generated IDs from prior LTI tests existing in the shared DB. Wiping rows here causes those tests | ||
| // to fail with ObjectOptimisticLockingFailureException ("Row was already updated or deleted") because | ||
| // Spring Data treats save(entity-with-id-set) as merge(), which requires the row to exist. |
End-to-End Test Results
Test Strategy: Two-phase execution
❌ Failed Tests (Phase 2)
Flakiness Scores for Failed Tests
Overall: ❌ Phase 2 (remaining tests) failed 🔗 Workflow Run · 📊 Test Report Phase 1 · 📊 Test Report Phase 2 |
Member
Author
|
Close in favor of another PR with a more comprehensive LTI test setup |
krusche
added a commit
that referenced
this pull request
May 24, 2026
This @AfterEach was introduced alongside the integration test in #12769 to prevent platform-row leakage, but other LTI tests (LtiIntegrationTest.getAllConfiguredLtiPlatformsAsAdmin and updateLtiPlatformConfigurationAsAdmin) hardcode platform id 1 and implicitly rely on persistent rows — the deleteAll() resets the sequence and causes optimistic-locking failures on subsequent inserts with the same id. The same fix is the entire content of #12772, which now becomes redundant once this PR lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Drop the
@AfterEach cleanupPlatforms()introduced in #12769 that callsltiPlatformConfigurationRepository.deleteAll(). The cleanup was added in response to a static-analysis concern about row leakage but actually broke two unrelated tests inLtiIntegrationTest— and the leakage turns out to be load-bearing infrastructure that those tests implicitly rely on.Checklist
General
Server
Motivation and Context
Follow-up to #12769. After that PR landed on
develop, the server-tests CI started failing on every run with:This blocks all PRs targeting
developuntil reverted.Description
Root cause.
Lti13InitiationIntegrationTest.@AfterEach cleanupPlatforms()(from #12769) callsltiPlatformConfigurationRepository.deleteAll()to avoid leaking rows into later LTI test classes. That cleanup runs in the shared JVM after every initiation test.But
LtiIntegrationTest.getAllConfiguredLtiPlatformsAsAdminandLtiIntegrationTest.updateLtiPlatformConfigurationAsAdmindo:Spring Data JPA's
SimpleJpaRepository.save(entity)decides betweenpersist()andmerge()viaEntityInformation.isNew(entity).LtiPlatformConfigurationextendsDomainObject(noPersistableimplementation), soisNew()returnstrueonly if the@Idfield isnull. Settingid=1LmakesisNew()returnfalse, which routes the call toem.merge()— and Hibernate's merge of a detached entity that doesn't exist in the DB issues anUPDATEthat matches zero rows →StaleObjectStateException→ObjectOptimisticLockingFailureException.Before #12769, these tests passed because
Lti13InitiationIntegrationTest(alphabetically first) inserted rows with auto-generated IDs starting at 1, and those rows persisted across test classes in the same JVM. The "leak" was therefore load-bearing —LtiIntegrationTestwas silently depending on it without anyone noticing.Fix. Drop the
@AfterEachand theorg.junit.jupiter.api.AfterEachimport. Document the inter-class dependency in a comment so the next person who feels the urge to add cleanup at least sees why it's intentional. The truly correct fix is to makeLtiIntegrationTestself-contained — either by usingpersist()directly with an unmanaged entity or by inserting via the test repository'ssaveAndFlush()without settingid— but that's a larger refactor and not in this PR's scope.Intra-class collisions remain impossible because each test in
Lti13InitiationIntegrationTestuses a UUID-keyedregistrationId.Steps for Testing
./gradlew test --tests Lti13InitiationIntegrationTest --tests LtiIntegrationTest -x webappTestserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
./gradlew test --tests "de.tum.cit.aet.artemis.lti.*"passes locally with this branch.Summary by CodeRabbit