-
Notifications
You must be signed in to change notification settings - Fork 61
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix corner cases in tests discovered after running on Github Actions #1579
Conversation
This PR is now ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catches 🎣
File(path).deleteRecursively() | ||
val rootPath: Path = Paths.get(path) | ||
val pathsToDelete: List<Path> = | ||
Files.walk(rootPath).sorted(Comparator.reverseOrder()).collect(Collectors.toList()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the sort needed to delete files first?
packages/test-base/src/jvmTest/kotlin/io/realm/kotlin/test/jvm/RealmTests.kt
Outdated
Show resolved
Hide resolved
delay(1000) | ||
counter-- | ||
} | ||
assertEquals(expectedThreadCount, totalThreadCount(), "Unexpected thread count after closing realm: ${newThreads()}") | ||
val totalThreadCount = totalThreadCount() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of debugging, we should print the stack trace of all non-daemon thread to find where they're are blocking
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but it would massively expand the output and so far all the cases we had for this, it was enough to get the name of the thread to figure out what was going on. So for now I think I would prefer to keep as as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Some minor comments
packages/library-base/src/commonMain/kotlin/io/realm/kotlin/Configuration.kt
Outdated
Show resolved
Hide resolved
@@ -76,5 +76,9 @@ class SystemNotificationTests { | |||
writer1.write { copyToRealm(Sample()) } | |||
writer2.write { copyToRealm(Sample()) } | |||
} | |||
writer1.close() | |||
writer2.close() | |||
liveRealmContext.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually green on CI? I tried to clean this up as part of another PR but ended up splitting into this separate issue #1563
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Maybe closing the writers individually first make it work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is working on GHA.
try { | ||
Files.deleteIfExists(p) | ||
} catch (e: java.nio.file.FileSystemException) { | ||
// Sometimes (on Windows) we need the give a GC a chance to run and close all native pointers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦
This PR is an attempt to cleanup #881