Fix flaky tests in IT pipeline#1321
Open
milanmajchrak wants to merge 5 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce CI flakiness by hardening test teardown and making test assertions/configuration updates more deterministic, without changing production behavior.
Changes:
- Add defensive handling in integration-test teardown for a transient Hibernate
ConcurrentModificationExceptionduring cleanup. - Centralize authentication plugin-sequence updates in REST ITs via a clear-then-set helper to prevent cross-test config leakage.
- Make ORCID sandbox-backed unit test assertions less brittle by removing a high, unstable
numFound()threshold.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java |
Adds CME handling during @After cleanup to avoid failing tests due to a known Hibernate race. |
dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.java |
Introduces helper for plugin sequence updates and replaces direct property writes to avoid intermittent header pollution. |
dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.java |
Replaces brittle ORCID sandbox result-count assertion with stable success + low-threshold checks. |
…uth sequence reset, ORCID assertion hardening)
vidiecan
previously approved these changes
May 26, 2026
…ad) + Hibernate CME diagnostics ORCID CachingOrcidRestConnectorTest no longer hits the live ORCID sandbox: search/getLabel/search_fail mock the HTTP layer (httpGet made protected) with a canned expanded-search response, so they are deterministic instead of asserting against fluctuating sandbox data. Shibboleth WWW-Authenticate flakiness: add a test-only config-definition.xml with config-reload=false. Runtime setProperty(...AuthenticationMethod...) overrides were silently discarded whenever the auto-reload listener rebuilt the combined config (restoring clarin-dspace.cfg's [Password, ClarinShib] default), intermittently leaking 'password realm' into the header. Verified: with auto-reload off the override survives; the explicit reloadConfig() reset in @after still works. Hibernate ConcurrentModificationException in @after cleanup: the per-session JDBC ResourceRegistry is not thread-safe, so the CME means two threads touch one Session. Capture a full thread dump on CME (target/cme-dumps/) to identify the colliding thread in CI; keep a resilient retry so an already-passed test isn't failed by this teardown race. (Context.finalize() ruled out: sessions are thread-local.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Disabling config auto-reload globally in the test environment broke AuthorizeConfigIT.testReloadConfiguration, which deliberately verifies that AuthorizeConfiguration picks up live changes written to local.cfg via the auto-reload mechanism. Auto-reload is a tested feature here, so it must not be disabled to work around the Shibboleth WWW-Authenticate flakiness. The Shibboleth flakiness (runtime setProperty override discarded when the combined config is rebuilt) needs a reload-safe fix in the auth test instead; tracked separately. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…henticate flakiness) The flaky 'password realm' leak in AuthenticationRestControllerIT had this root cause: configurationService.setProperty(plugin.sequence...AuthenticationMethod, ...) only updates the in-memory view of the combined configuration. That view is discarded whenever it is rebuilt, and the auto-reload listener rebuilds it as soon as any reloadable cfg file's mtime changes mid-run (e.g. another test writing local.cfg). When that rebuild lands between the override and the request, clarin-dspace.cfg's default [PasswordAuthentication, ClarinShibAuthentication] returns and 'password realm' leaks into the header. The previous clear-then-set helper did not help (it is equivalent to a plain setProperty). Fix: set the sequence via a JVM system property (highest-precedence override layer, re-read on every rebuild) + reloadConfig(), and clear it in @after. This survives auto-reload without disabling it (so AuthorizeConfigIT, which verifies auto-reload, still passes). Verified in the real /api/authn/status endpoint: an explicit reloadConfig() after a setProperty override reproduces the leak, while the system-property approach keeps the header Shibboleth-only across rebuilds. Full AuthenticationRestControllerIT (43 tests) passes, and running it alongside ClarinAuthenticationRestControllerIT / AnonymousAdditionalAuthorizationFilterIT confirms the property does not leak across classes. Co-Authored-By: Claude Opus 4.8 <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.
Fix flaky tests in IT pipeline
Summary
This PR addresses three flaky test sources observed in CI:
ConcurrentModificationExceptionin IT teardownWWW-Authenticateheader pollution in Shibboleth-only auth testsWhat changed
dspace-api/src/test/java/org/dspace/AbstractIntegrationTestWithDatabase.java@Aftercleanup.dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthenticationRestControllerIT.javasetProperty(...AuthenticationMethod, ...)calls with helper usage to avoid stale/auth-method leakage between tests.dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.javanumFound() > 1000expectation with stable assertions (isOk()and low threshold).Why
These failures were non-deterministic and caused false negatives in CI while application behavior was otherwise correct.
Expected impact