Conversation
Member
infeo
commented
Mar 5, 2026
infeo
commented
- use Awailtility.untilAsserted
- add assertInUseFile method to check for valid inUse file
- remove assertNull for useTokens map (tokens are not initially inserted)
* use Awailtility.untilAsserted * add assertInUseFile method to check for valid inUse file * remove assertNull for useTokens map (tokens are not initially inserted)
Contributor
WalkthroughThe RealUseTokenTest class was updated to replace an Executor with an ExecutorService, add a private helper assertInUseFile(expectedOwner, filePath) that validates OWNER_KEY and LASTUPDATED_KEY in on-disk in-use files, and refactor many tests to rely on that helper and file-state/watch-event checks instead of direct useTokens map assertions. Tests were renamed or added (e.g., testFileContent → testValidFileContent, testMoveToNeverPersistedClosed, testMoveToClosed), some annotated with Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java`:
- Around line 195-231: The tests race on filesystem events; drain and settle
close-related events before calling moveToInternal so move-related assertions
only see new events. For both testMoveToNeverPersistedClosed and
testMoveToClosed: after token.close() use Awaitility (or a short loop) to wait
until watchKey.pollEvents() is non-empty (i.e., close-generated events have
arrived), then call watchKey.pollEvents() once to clear/drain those events; only
after that call token.moveToInternal(targetPath) and assert that
watchKey.pollEvents() is empty and useTokens.get(targetPath) is null. Also avoid
using watchKey.pollEvents() inside Awaitility predicates (consume events there)
— use a separate drain step instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a690190b-8677-4e10-90f0-e6184bb18231
📒 Files selected for processing (1)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java
* add checks that token is removed from map * remove watchKey where possible * shutdown executorService after each test
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java (1)
206-239:⚠️ Potential issue | 🟠 MajorClosed-move event checks are still flaky due to event consumption/timing.
In
testMoveToClosed, Line 233 consumes events inside the Awaitility predicate; and both tests assert emptiness immediately after move (Line 216 / Line 238), which can miss delayed FS events or mix close vs. move events.Suggested stabilization
public void testMoveToNeverPersistedClosed() throws IOException { @@ try (var token = new RealUseToken(filePath, "test3000", cryptor, useTokens, tokenPersistor, CREATION_DELAY_MILLIS, StandardOpenOption.CREATE_NEW, encWrapper)) { token.close(); + Awaitility.await().pollDelay(FILE_OPERATION_MAX).until(() -> true); + watchKey.pollEvents(); // drain potential close-related events token.moveToInternal(targetPath); + Awaitility.await().pollDelay(FILE_OPERATION_DELAY).until(() -> true); MatcherAssert.assertThat(watchKey.pollEvents(), Matchers.empty()); Assertions.assertNull(useTokens.get(targetPath)); } } @@ public void testMoveToClosed() throws IOException { @@ try (var token = new RealUseToken(filePath, "test3000", cryptor, useTokens, tokenPersistor, CREATION_DELAY_MILLIS, StandardOpenOption.CREATE_NEW, encWrapper)) { Awaitility.await().atLeast(FILE_OPERATION_DELAY).atMost(FILE_OPERATION_MAX) // .untilAsserted(() -> assertInUseFile("test3000", filePath)); watchKey.pollEvents(); //clear watchEvents token.close(); - Awaitility.await().atMost(FILE_OPERATION_MAX) // - .until(() -> !watchKey.pollEvents().isEmpty()); + Awaitility.await().atMost(FILE_OPERATION_MAX) + .untilAsserted(() -> Assertions.assertTrue(Files.notExists(filePath))); + watchKey.pollEvents(); //drain close-related events token.moveToInternal(targetPath); + Awaitility.await().pollDelay(FILE_OPERATION_DELAY).until(() -> true); MatcherAssert.assertThat(watchKey.pollEvents(), Matchers.empty()); Assertions.assertNull(useTokens.get(targetPath)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java` around lines 206 - 239, Both tests (testMoveToNeverPersistedClosed and testMoveToClosed) are flaky due to consuming watchKey events inside Awaitility and asserting empty immediately after token.moveToInternal; change the event handling to (1) avoid consuming events inside the Awaitility predicate in testMoveToClosed—use Awaitility only to wait for the file to appear (assertInUseFile("test3000", filePath)) and then call watchKey.pollEvents() once to clear buffered events, and (2) after calling token.moveToInternal(targetPath) in both tests, don't assert emptiness immediately—use Awaitility to wait for a short stabilization period (e.g., until watchKey.pollEvents() stays empty for a small interval) before asserting MatcherAssert.assertThat(watchKey.pollEvents(), Matchers.empty()) and Assertions.assertNull(useTokens.get(targetPath)); reference watchKey, token.moveToInternal, testMoveToClosed, and testMoveToNeverPersistedClosed when making the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java`:
- Around line 72-79: The teardown code can NPE if beforeEach failed to
initialize tokenPersistor; update the RealUseTokenTest teardown to null-check
tokenPersistor before calling
tokenPersistor.shutdown()/awaitTermination()/shutdownNow() (e.g., if
(tokenPersistor != null) { ... }), preserving the existing InterruptedException
handling so shutdown is only attempted when tokenPersistor is initialized.
---
Duplicate comments:
In `@src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java`:
- Around line 206-239: Both tests (testMoveToNeverPersistedClosed and
testMoveToClosed) are flaky due to consuming watchKey events inside Awaitility
and asserting empty immediately after token.moveToInternal; change the event
handling to (1) avoid consuming events inside the Awaitility predicate in
testMoveToClosed—use Awaitility only to wait for the file to appear
(assertInUseFile("test3000", filePath)) and then call watchKey.pollEvents() once
to clear buffered events, and (2) after calling token.moveToInternal(targetPath)
in both tests, don't assert emptiness immediately—use Awaitility to wait for a
short stabilization period (e.g., until watchKey.pollEvents() stays empty for a
small interval) before asserting MatcherAssert.assertThat(watchKey.pollEvents(),
Matchers.empty()) and Assertions.assertNull(useTokens.get(targetPath));
reference watchKey, token.moveToInternal, testMoveToClosed, and
testMoveToNeverPersistedClosed when making the edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bc6fecd-529a-4ab9-98d9-6e702294cd20
📒 Files selected for processing (1)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java (1)
65-79:⚠️ Potential issue | 🟡 MinorMake teardown null-safe for partially initialized fixtures.
If setup fails before full initialization,
afterEachcan throw and hide the real failure (e.g., Line 67/Line 73 dereference nullable fields).Proposed hardening
`@AfterEach` public void afterEach() { try { - watchService.close(); + if (watchService != null) { + watchService.close(); + } } catch (IOException _) { //no-op } try { - tokenPersistor.shutdown(); - if (!tokenPersistor.awaitTermination(3000, TimeUnit.MILLISECONDS)) { - tokenPersistor.shutdownNow(); + if (tokenPersistor != null) { + tokenPersistor.shutdown(); + if (!tokenPersistor.awaitTermination(3000, TimeUnit.MILLISECONDS)) { + tokenPersistor.shutdownNow(); + } } } catch (InterruptedException e) { Thread.currentThread().interrupt(); + if (tokenPersistor != null) { + tokenPersistor.shutdownNow(); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java` around lines 65 - 79, The teardown in afterEach should be null-safe: guard dereferences of watchService and tokenPersistor before calling methods on them (e.g., check watchService != null before calling watchService.close(), and check tokenPersistor != null before calling tokenPersistor.shutdown(), awaitTermination(...), and shutdownNow()). Preserve existing exception handling (IOException for close and InterruptedException for awaitTermination) and re-set the thread interrupt status when catching InterruptedException. Ensure you reference the existing afterEach method and the fields watchService and tokenPersistor when applying these null checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java`:
- Around line 65-79: The teardown in afterEach should be null-safe: guard
dereferences of watchService and tokenPersistor before calling methods on them
(e.g., check watchService != null before calling watchService.close(), and check
tokenPersistor != null before calling tokenPersistor.shutdown(),
awaitTermination(...), and shutdownNow()). Preserve existing exception handling
(IOException for close and InterruptedException for awaitTermination) and re-set
the thread interrupt status when catching InterruptedException. Ensure you
reference the existing afterEach method and the fields watchService and
tokenPersistor when applying these null checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebad3023-5d5a-40b6-ab22-3db2c88943b3
📒 Files selected for processing (1)
src/test/java/org/cryptomator/cryptofs/inuse/RealUseTokenTest.java
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
1 participant
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.