-
Notifications
You must be signed in to change notification settings - Fork 60
Tests for GRDB+TaskLocal in async context #104
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
Conversation
abc7a51 to
bb18a7f
Compare
|
Hi @rcarver, thanks for taking the time to do this, but I'm not sure we need these tests in our library. They are only testing behavior in GRDB proper. It would also force us to update our dependence on GRDB to be the newest in order to guarantee tests pass, but I don't think it's necessary to force that update on everyone using this library. I am going to close this for now, but if you have reason to believe that we should definitely have these tests in SharingGRDB we're open to discussing more! |
|
@mbrandonw totally makes sense, and was not sure how to handle this. I'd note that using GRDB < 7.6.0 (just released) in async contexts:
Absolutely up to you about including the tests, really just wanted to prove that GRDB is working now |
|
Hi @rcarver, we typically like to be a little conservative with bumping our dependencies. We've seen many times people get stuck in a difficult spot where they can no longer update dependencies because they have to restrict one library's version for whatever reason. So we think it's ok for people to just update to the newest GRDB themselves if they run into this problem. |
|
@mbrandonw I found a new issue with GRDB+SnapshotTesting. It's technically different, but related enough I thought I'd keep the discussion here for now (all issues contribute to not being able to use DescriptionAny use of This is unrelated the async issue, as I can confirm with GRDB 7.4.0 to 7.6.0 import GRDB
import InlineSnapshotTesting
import Testing
@MainActor
@Test
func snapshotInsideReadMainActor() throws {
let database = try DatabaseQueue()
try database.read { db in
assertInlineSnapshot(of: "foo", as: .lines) // 💥 crashes
}
}The same crash happens in XCTest without tagging import GRDB
import InlineSnapshotTesting
import XCTest
final class SnapshotXCTests: XCTestCase {
func testInlineSnapshotInsideDatabaseRead() throws {
let database = try DatabaseQueue()
try database.read { db in
assertInlineSnapshot(of: "foo", as: .lines) // 💥 crashes
}
}
}You can also see the crash by simply adding Here is a full set of sync/async MainActor/non-MainActor tests just to fully understand what works and what doesn't. import GRDB
import InlineSnapshotTesting
import Testing
@Test
func snapshotInsideRead() throws {
let database = try DatabaseQueue()
try database.read { db in
assertInlineSnapshot(of: "foo", as: .lines) {
"""
foo
"""
}
}
}
@MainActor
@Test("always fails")
func snapshotInsideReadMainActor() throws {
let database = try DatabaseQueue()
try database.read { db in
assertInlineSnapshot(of: "foo", as: .lines) // 💥 crashes
}
}
@Test("works with GRDB 7.6.0 only")
func snapshotInsideAsyncRead() async throws {
let database = try DatabaseQueue()
try await database.read { db in
assertInlineSnapshot(of: "foo", as: .lines) {
"""
foo
"""
}
}
}
@MainActor
@Test("also only works with GRDB 7.6.0")
func snapshotInsideAsyncReadMainActor() async throws {
let database = try DatabaseQueue()
try await database.read { db in
assertInlineSnapshot(of: "foo", as: .lines) {
"""
foo
"""
}
}
} |
|
Hi @rcarver, good catch! This is easy to fix and we will have a PR soon. |
|
@rcarver Can you point your project to this branch of swift-snapshot-testing and report back? pointfreeco/swift-snapshot-testing#1012 |
|
@stephencelis works great!
|
This adds tests to ensure GRDB behaves correctly in async contexts, and thus that
SharingGRDBand related tools behave correctly.TaskLocalbehave as expected inside async read/writeassertQuerybehave correctly in async read/writeThe GRDB side in progress per this discussion: groue/GRDB.swift#1793
To verify the changes to GRDB:
developmentbranch and confirm tests pass