Require successful initial opening in the recovery test - #614
Merged
Merged
Conversation
Extract the scoped recovery assertion identified in #588. Initial opening and cleanup failures must fail independently; close any Map unexpectedly returned by recovery outside assertThrows.
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Java 26 and Java 27 CI failures remain unexplained despite the test change appearing correct.
Pull request overview
Scopes the recovery-failure assertion so initial map opening and cleanup must succeed independently.
Changes:
- Replaces annotation-based exception handling with
assertThrows. - Closes any unexpectedly recovered map outside the assertion.
File summaries
| File | Description |
|---|---|
Issue229Test.java |
Tightens recovery exception validation and cleanup. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Use Closeable.closeQuietly for the nullable recovery result, keeping cleanup outside the scoped assertion. Addresses review feedback on PR #614.
peter-lawrey-admin
approved these changes
Sep 8, 2026
peter-lawrey
added a commit
to OpenHFT/Chronicle-Values
that referenced
this pull request
Sep 8, 2026
…#180) The file manager is reused during concurrent value-model creation. A HashMap.compute failure can be cached by ValueModel and spread to Map tests. Use concurrent maps and sets, resolve class resources outside remapping callbacks, and explain the concurrency and snapshot boundaries in //! notes. The latch-controlled registration regressions reproduce the original ConcurrentModificationException and protect retained entries and snapshots. Refs OpenHFT/Chronicle-Map#614; VAL-TEST-402.
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.
Issue229Testpreviously acceptedChronicleHashRecoveryFailedExceptionanywhere in the test, including the initial Map opening. Require that opening to succeed, then use JUnit 4'sassertThrowsaround recovery alone. UseCloseable.closeQuietly(unexpectedRecovery.get())for null-safe cleanup outside that assertion, so cleanup cannot satisfy the expected recovery failure.This extracts the exception-scope improvement identified in #588. It changes one test class and keeps the existing framework and POM.
Validation:
mvn clean verify -Dtest=Issue229Test -Dsurefire.rerunFailingTestsCount=0passed on Java 21 and Java 8 after thecloseQuietlyrevision (one test, no skips).CI evidence for the preceding head,
215114709dd5bb6564ff7416827083c843e77e28: nine supported contexts passed; Mac build 316 failed with two assertion failures and 13 errors. The supplied Mac log explicitly reportsIssue229Test: one test, zero failures/errors/skips. Its earlier failures includeConcurrentModificationExceptionatMyJavaFileManager.addFileObjectsin Chronicle Values, followed by value-generation and serialised-size errors. This is separate from the recovery assertion being changed here; a Mac rerun has not been verified for thecloseQuietlyrevision.Java 26 and later checks are informational, not merge blockers. Their failures are not included in the supported-CI assessment above.
Validation environment and dependency receipt
Linux x86_64; Maven 3.9.11; OpenJDK 21.0.12 and 8u502. Surefire 3.1.2, JUnit 4.13.2, Vintage 5.10.0. Failing-test reruns explicitly disabled with
-Dsurefire.rerunFailingTestsCount=0. Builds start fromdevelopat0c25269da8d12e3e1cd10c7f4c74ae35a017ab2c.Resolved Chronicle dependencies: Core 2026.6; Bytes 2026.4; Wire 2026.10-SNAPSHOT; Values 2026.2; Threads 2026.3; Algorithms 2026.2; Affinity 2026.2; Posix 2026.2; Test Framework 2026.2; JLBH 2026.2. Parent and third-party BOM: 2026.0. XStream: 1.4.21.
Mutable-input SHA-256:
The unchanged baseline also passed full
mvn verify: 1,316 tests, 82 existing skips. Windows/macOS and downstream projects were not run locally.