Skip to content

Conversation

@rcarver
Copy link
Contributor

@rcarver rcarver commented Aug 20, 2025

Per discussion in Slack, this adds assertSelect to StructuredQueriesTestSupport. It's like assertQuery but skips snapshotting the sql fragment.

The implementation is a little weird, maybe assertQuery with all the options should be private?

@rcarver rcarver changed the title Add assertSelect test helepr Add assertSelect test helper Aug 20, 2025
@stephencelis
Copy link
Member

stephencelis commented Aug 20, 2025

Hey @rcarver! Thanks for exploring this. Brandon and I chatted about it and have a couple thoughts:

  1. What do you think about adding this to SharingGRDB, instead, in a SharingGRDBTestSupport module? It seems like it'd be most useful there and could better incorporate the dependency. We could reconsider a helper in StructuredQueries should the library grow to have more drivers in the future.

  2. How about we reuse assertQuery naming and simply add a boolean of whether or not we want to snapshot the SQL. Bikeshedding:

    assertQuery(Reminder.all)  // Snapshots results only
    assertQuery(               // Snapshots SQL + results
      snapshotSQL: true,       // Or maybe 'includeSQL: true'?
      Reminder.all
    )

You down to tackle that?

@rcarver
Copy link
Contributor Author

rcarver commented Aug 20, 2025

@stephencelis cool that makes sense. Questions:

Would SharingGRDBTestSupport depend on StructuredQueriesTestSupport and use its assertQuery? If so, would there be some ambiguity with function assertQuery in both? Or do you see StructuredQueriesTestSupport as separate and we'd copy in the existing code including printTable ?

@stephencelis
Copy link
Member

It could depend on it but not re-export it, so the symbols shouldn't clash. If you run into any issues, though, let us know!

@rcarver
Copy link
Contributor Author

rcarver commented Aug 21, 2025

Closing in favor of https://github.com/pointfreeco/sharing-grdb/pull/133

@rcarver rcarver closed this Aug 21, 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