Skip to content

GORM migration: extract sqltest suite#7120

Open
MarcosDY wants to merge 5 commits into
spiffe:mainfrom
MarcosDY:gormv2-extract-sqltest
Open

GORM migration: extract sqltest suite#7120
MarcosDY wants to merge 5 commits into
spiffe:mainfrom
MarcosDY:gormv2-extract-sqltest

Conversation

@MarcosDY

@MarcosDY MarcosDY commented Jul 4, 2026

Copy link
Copy Markdown
Collaborator

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
The SQL datastore test suite.
This is a test-only refactor, no production datastore behavior changes.
It's the second step of the ongoing work to allow an optional gorm v2 datastore to live alongside the existing one, following the sqlcommon extraction that already merged (#7114).

Description of change
The test suite (the big PluginSuite) used to live inside the sqlstore package's test file, so only the v1 datastore could run it. This PR lifts it out into a new shared pkg/server/datastore/sqltest package, parameterized over a datastore factory, so a future gorm v2 datastore can be validated against the exact same tests.

A few decisions worth calling out, in case you'd have done them differently:

  • The raw-SQL escape hatch (RawScan/RawExec/DatabaseType/Rebind) is exported on the concrete *Plugin in a non-test file. A handful of tests need raw DB access that bypasses the ORM (PRAGMA checks, inserting deliberately-corrupt rows, etc.). Because the shared suite is now a separate package, these methods can't live in a _test.go file or behind a build tag. So they have to be exported. They're documented as test-support only and aren't wired to anything in production. Open to suggestions if there's a cleaner pattern I missed.
  • RawQuerier is embedded in DataStoreUnderTest so the escape-hatch contract is checked at compile time.
  • Placeholder rewriting has a single source of truth. The suite briefly had its own copy of the ? → $1 rebind logic. That's now a Rebind() method delegating to the production maybeRebind, so the tests can't drift from what production actually generates.
  • The suite still uses v1's HCL config keys (database_type, connection_string, etc.). This is intentional, the planned v2 datastore reuses the same config schema on purpose (same parser), so there's nothing to abstract yet.

No docs change — this is test infrastructure only.

Which issue this PR fixes
Part of #1832

MarcosDY added 3 commits July 4, 2026 15:33
Signed-off-by: Marcos Yacob <marcosyacob@gmail.com>
Signed-off-by: Marcos Yacob <marcosyacob@gmail.com>
Signed-off-by: Marcos Yacob <marcosyacob@gmail.com>
Copilot AI review requested due to automatic review settings July 4, 2026 19:15
@MarcosDY MarcosDY changed the title Gormv2 extract sqltest GORM migration: extract sqltest suite Jul 4, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the SQL datastore conformance test suite out of the sqlstore package into a new shared pkg/server/datastore/sqltest package so multiple SQL datastore implementations (e.g., future GORM v2) can be validated against the same test corpus.

Changes:

  • Introduces pkg/server/datastore/sqltest with a parameterized shared suite (Config, DataStoreUnderTest, and RawQuerier escape hatch).
  • Adapts the existing sqlstore (v1) tests to run the shared suite via sqltest.Run(...).
  • Exposes test-support raw SQL helpers (RawScan, RawExec, DatabaseType, Rebind) on *sqlstore.Plugin for cross-package suite needs.

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/server/datastore/sqltest/suite.go Defines the shared suite entrypoint (Run) and the datastore-under-test contract.
pkg/server/datastore/sqltest/rawquerier.go Declares the raw SQL escape-hatch interface exercised by the shared tests.
pkg/server/datastore/sqltest/datastore_suite.go Contains the extracted, parameterized conformance tests previously hosted in sqlstore tests.
pkg/server/datastore/sqlstore/sqltest_assert_test.go Compile-time assertions that *Plugin satisfies the shared suite contracts.
pkg/server/datastore/sqlstore/sqlstore.go Adds exported raw SQL helper methods on *Plugin for the shared suite.
pkg/server/datastore/sqlstore/sqlstore_test.go Replaces the in-package PluginSuite with sqltest.Run(...) and keeps remaining whitebox tests local.

Comment thread pkg/server/datastore/sqlstore/sqlstore_test.go Outdated
Comment thread pkg/server/datastore/sqlstore/sqlstore.go
Comment thread pkg/server/datastore/sqlstore/sqlstore.go
Comment thread pkg/server/datastore/sqlstore/sqltest_assert_test.go
Signed-off-by: Marcos Yacob <marcosyacob@gmail.com>
sorindumitru
sorindumitru previously approved these changes Jul 6, 2026

@sorindumitru sorindumitru 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.

LGTM, thanks @MarcosDY

Signed-off-by: Marcos Yacob <marcosyacob@gmail.com>
@MarcosDY

MarcosDY commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

@sorindumitru there was a flaky test on windows I applied a change to try to resolve lets me know what do you think

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.

4 participants