Skip to content

SharingGRDBTestSupport with assertQuery - #133

Merged
stephencelis merged 7 commits into
pointfreeco:mainfrom
rcarver:assert-query
Aug 26, 2025
Merged

stephencelis merged 7 commits into
pointfreeco:mainfrom
rcarver:assert-query

Conversation

@rcarver

@rcarver rcarver commented Aug 21, 2025

Copy link
Copy Markdown
Contributor

Adds SharingGRDBTestSupport as a new library, which exposes assertQuery such as from StructuredQueriesTestSupport.

However, the API is slightly different

  1. It doesn't snapshot the SQL by default as its more common to look at db content only in this context.
  2. It doesn't require an execute argument, as it uses @Dependency(\.defaultDatabase)
assertQuery(Reminder.all)  // Snapshots results only
assertQuery(               // Snapshots SQL + results
  includeSQL: true, 
  Reminder.all
)

I ended up just copying the implementation from StructuredQueries since printTable is private and its version of assertQuery would need to be modified for the includeSQL option. Could open those up with SPI if you think it's worth it?

@rcarver
rcarver marked this pull request as ready for review August 21, 2025 05:24
)
}
do {
@Dependency(\.defaultDatabase) var database

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of an optional database: Database? parameter like the @Fetch family of property wrappers, which defaults to this dependency?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

@stephencelis

Copy link
Copy Markdown
Member

@rcarver This looks good! But we got a test failure for assertQuery in release since we don't pretty-print queries in release mode. Want to wrap those tests in #if DEBUG?

@rcarver

rcarver commented Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

@stephencelis done! Confirmed it fixed via swift test -c release but I guess you have to kick off the CI in this repo?

@stephencelis stephencelis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@stephencelis
stephencelis merged commit 43441e4 into pointfreeco:main Aug 26, 2025
9 of 10 checks passed
mackoj pushed a commit to mackoj/sqlite-data that referenced this pull request Dec 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants