test: de-flake ORCID cache tests and ZIP-download IT#1344
Conversation
Two independent flaky tests keep turning the dtq-dev pipeline red after #1321: 1. CachingOrcidRestConnectorTest.testCachable / testCacheableWithError still hit the live ORCID sandbox (#1321 only mocked getLabel/search/search_fail). They use the real Spring @Cacheable bean (to exercise the CGLIB caching proxy), so they could not be spied. Point the bean's apiURL at a local MockWebServer serving the canned orcid-expanded-search.xml instead -- keeps the real caching proxy and HTTP transport under test, removes the network dependency. No production change. 2. MetadataBitstreamControllerIT.downloadAllZip compared the response to a locally-built ZIP byte-for-byte; a ZIP entry's DOS timestamp (2s resolution) defaults to "now" on both sides and differs across a 2s boundary. Assert the unzipped entry name + content instead of raw bytes. Test-only changes. Verified locally (ORCID class 8/8 green, repeated offline runs; webapp test module compiles; checkstyle clean). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTests now use local mocking or explicit commits to make ORCID, ZIP, SSR, and statistics assertions depend on test-controlled state. ChangesTest isolation and timing updates
🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@dspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java`:
- Around line 91-101: The ZIP assertion in MetadataBitstreamControllerIT is
masking duplicate entries because the ZipInputStream loop stores them in a Map
keyed by entry name, so repeated filenames overwrite each other. Update the test
to track each ZIP entry independently in the same loop (for example by counting
entries or collecting them in a list) and assert that exactly one entry is
returned before checking its name and content. Use the existing ZipInputStream
iteration and the bts.getName()/BITSTREAM_CONTENT assertions to locate the test
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96ae7c62-ae49-4926-ad24-0c0cadd8b02d
📒 Files selected for processing (2)
dspace-api/src/test/java/org/dspace/external/CachingOrcidRestConnectorTest.javadspace-server-webapp/src/test/java/org/dspace/app/rest/MetadataBitstreamControllerIT.java
Address CodeRabbit: a Map keyed by entry name could mask duplicate ZIP entries (same filename overwrites). Track the entry count separately and assert it is 1, so an unexpected extra entry fails the test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two more intermittent failures on dtq-dev, both addressed at the root cause (test-only changes): 1. AuthorizationRestRepositoryIT.findByObjectSSRTest flaked with 400 instead of 200. The test sets dspace.server.ssr.url (used by Utils.getBaseObjectRestFromUri to resolve the request URI) and the AlwaysThrowExceptionFeature.turnoff flag via configurationService.setProperty(...). Such in-memory overrides are silently dropped when the combined config is rebuilt by the auto-reload listener (fires on any reloadable cfg file mtime change mid-run). When ssr.url is dropped the URI no longer resolves -> 400; if turnoff were also dropped the /search/object path would let alwaysexception throw -> 500. Fix: set both via JVM system properties (+ reloadConfig) so they sit in the highest-precedence override layer and survive auto-reload, cleared in @after. Same pattern as #1321's Shibboleth fix (AuthenticationRestControllerIT#setAuthenticationMethodSequence). Applied to all three SSR tests. 2. StatisticsRestRepositoryIT.topCountriesReport_Community_Visited flaked with an empty report (points: []). postView() commits with waitSearcher=false, so the just-posted view events can be invisible to the immediately-following report query (and a dropped solr-statistics.autoCommit override would skip the commit entirely). Fix: after posting the view events, force solrLoggerService.commit() (waitSearcher =true) so the events are flushed and visible before the report is queried. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Why
dtq-dev's scheduledbuild.ymlkeeps going red intermittently even after #1321 merged. The samecommit SHA passes some scheduled runs and fails others — a pure flakiness signal. Investigating the recent
failing runs (via the job logs) pinpointed four independent, genuinely-flaky tests. All four are fixed
here at the root cause — test-only changes, no retries / no symptom-masking.
CachingOrcidRestConnectorTest.testCachable/testCacheableWithErrorexpected:<Connor, John> but was:<null>MetadataBitstreamControllerIT.downloadAllZipAuthorizationRestRepositoryIT.findByObjectSSRTestexpected:<200> but was:<400>setPropertyStatisticsRestRepositoryIT.topCountriesReport_Community_Visitedpoints: []waitSearcher=falserace1.
CachingOrcidRestConnectorTest— live ORCID sandbox dependency#1321 mocked the HTTP layer for
getLabel/search/search_fail, but lefttestCachableandtestCacheableWithErrorhitting the live ORCID sandbox (https://pub.sandbox.orcid.org/v3.0). They use thereal Spring bean (not a Mockito spy) on purpose — to exercise the real
@CacheableCGLIB proxy — so theHTTP layer couldn't be stubbed. Every CI run made a real network call; when the sandbox was slow/unavailable/
changed,
getLabelreturnednull.Fix: point the real bean's
apiURLat a localokhttp3.mockwebserver.MockWebServerserving the cannedorcid-expanded-search.xml. Keeps the real@Cacheableproxy + HTTP transport under test, removes thenetwork dependency. Mirrors the existing
OpenAIRERestConnectorTest.testCachablenow also assertsgetRequestCount() == 1(proves the 2nd call is cache-served). Verified locally (class 8/8 green, repeatedoffline runs).
2.
MetadataBitstreamControllerIT.downloadAllZip— time-dependent ZIP bytesThe test compared the server response to a locally-built ZIP byte-for-byte. A ZIP entry's DOS timestamp
(2-second resolution) defaults to "now" on both sides and differs across a 2-second boundary.
Fix: assert the unzipped entry name + content (and exactly one entry) instead of raw bytes — removes the
timestamp dependency and is a stronger assertion.
3.
AuthorizationRestRepositoryIT.findByObjectSSRTest— config auto-reload dropssetPropertyThe test sets
dspace.server.ssr.url(used byUtils.getBaseObjectRestFromUrito resolve the request URI) andAlwaysThrowExceptionFeature.turnoffviaconfigurationService.setProperty(...). Those in-memory overrides aresilently dropped when the combined config is rebuilt by the auto-reload listener (fires on any reloadable
*.cfgmtime change mid-run). Whenssr.urlis dropped the URI no longer resolves →400; and because the/search/objectpath enumerates all SITE features (incl.alwaysexception, no try/catch), a droppedturnoffwould let it throw →
500.Fix: set both values via JVM system properties (+
reloadConfig()) so they live in thehighest-precedence
<system/>override layer and are re-read on every rebuild → survive auto-reload; cleared in@After. Exactly the pattern #1321 used for the ShibbolethWWW-Authenticateleak(
AuthenticationRestControllerIT#setAuthenticationMethodSequence). Applied to all three SSR tests.4.
StatisticsRestRepositoryIT.topCountriesReport_Community_Visited— Solr searcher raceAfter posting two view events the test immediately queried the TopCountries report and intermittently got
points: [].SolrLoggerServiceImpl.postViewcommits withsolr.commit(false, false)(waitSearcher=false),so it can return before the new searcher is registered and the report query hits the stale searcher (and a
dropped
solr-statistics.autoCommit=falseoverride would skip the commit entirely).Fix: after posting the view events, force
solrLoggerService.commit()(no-arg →waitSearcher=true) so theevents are flushed and visible before the report query — robust to both the searcher race and a dropped
autoCommitoverride.Verification & review
current
dspace-api;dspace-apicheckstyle clean.the SSR "both overrides must survive" and the Solr "commit covers both causes" points were validated).
searchflakes show no recurrence post-merge — those fixes are holding.All changes are confined to test code (
*Test.java/*IT.java).Summary by CodeRabbit