feat: add a Test Compatibility Kit for the storages#1885
Conversation
It uses generics to receive a testify's suite for a concrete Storage type
At the moment, Aerospike and Clickhouse are the only ones not implementing it, even though it's mentioned in their docs.
mdelapenya
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a generic, container-backed TCK framework for storage tests and migrates MySQL tests to it. Introduces a generic StorageWithConn interface. Implements suite lifecycle (per-test/per-suite), helper tests, and documentation for the TCK. Replaces prior MySQL unit tests with a TCK-driven suite. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor T as Test Runner
participant S as TCK Suite
participant C as Container Factory
participant F as Store Factory
participant DB as Container (e.g., MySQL)
participant ST as Storage
T->>S: New(ctx, t, suite, opts)
Note over S: Choose lifecycle hook (PerTest/PerSuite)
rect rgba(220,235,245,0.4)
alt PerSuite
T->>S: SetupSuite
S->>C: NewContainer()(ctx, tb)
C-->>S: DB
S->>F: NewStore()(ctx, tb, DB)
F-->>S: ST
else PerTest
T->>S: SetupTest
S->>C: NewContainer()(ctx, tb)
C-->>S: DB
S->>F: NewStore()(ctx, tb, DB)
F-->>S: ST
end
end
loop Core storage tests
S->>ST: Set/Get/Delete/Reset/Close/Conn
ST-->>S: Results
end
rect rgba(245,230,220,0.4)
alt PerSuite
T->>S: TearDownSuite
S-->>ST: Close
S-->>DB: Terminate
else PerTest
T->>S: TearDownTest
S-->>ST: Close
S-->>DB: Terminate
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @mdelapenya, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant improvement to the testing infrastructure by establishing a Test Compatibility Kit (TCK). The primary goal is to standardize the testing process for all storage implementations, ensuring uniform behavior and simplifying the addition of new storage backends. By abstracting common test patterns into the TCK, it paves the way for a more maintainable and robust codebase, starting with the MySQL module as a concrete example of its application.
Highlights
- New Test Compatibility Kit (TCK) Module: Introduced a new
testhelpers/tckmodule to provide a standardized Test Compatibility Kit for all storage implementations. This TCK aims to ensure consistent behavior across different storage backends and reduce test code duplication. - MySQL Module Migration to TCK: The existing tests in the MySQL storage submodule have been refactored and migrated to utilize the new TCK. This serves as a reference implementation for future migrations of other storage modules.
- Enhanced Storage Interface and Connection Handling: Added a new
StorageWithConngeneric interface to allow storage implementations to expose their underlying connection. TheConn()method has been implemented in the Aerospike and Clickhouse modules, and the TCK includes tests for this functionality, with an option to skip if not applicable. - Comprehensive TCK Documentation and Unit Tests: New documentation (
testhelpers/tck/README.md) has been added, detailing the TCK's purpose, usage, and implementation guidelines. The TCK itself includes unit tests using a mock storage based onsync.Mapto validate its functionality.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces an excellent Test Compatibility Kit (TCK) to standardize storage implementation tests, which will greatly improve maintainability and consistency. The refactoring of the MySQL tests serves as a great example of its usage. I've identified one critical issue in the TCK suite logic that could break test isolation in PerSuite mode. Additionally, I've found a couple of minor documentation typos and inconsistencies in the new README file. Once the critical issue is addressed, this will be a fantastic addition to the project.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
aerospike/aerospike.go (2)
237-254: Bug: exp=0 must mean “no expiration” per Storage contract; current code forces 1s TTLSet() forces ttl>=1 which breaks the “0 means no expiration” contract and can cause unexpected expirations.
- expiration := s.expiration - if exp != 0 { - expiration = exp - } - - // Convert to seconds with a minimum of 1 - ttl := uint32(expiration.Seconds()) - if ttl < 1 { - ttl = 1 - } - - writePolicy := aerospike.NewWritePolicy(0, ttl) + expiration := s.expiration + if exp != 0 { + expiration = exp + } + + // 0 means "no expiration" per Storage interface. + var ttl uint32 + if expiration == 0 { + ttl = 0 + } else { + ttl = uint32(expiration.Seconds()) + if ttl == 0 { + ttl = 1 + } + } + + writePolicy := aerospike.NewWritePolicy(0, ttl)
277-315: Stale cache: Reset deletes _schema_info but leaves s.schemaInfo populatedAfter Reset, s.schemaInfo points to stale metadata. Clear or rebuild it.
func (s *Storage) Reset() error { @@ for result := range recordset.Results() { @@ } - return nil + // Avoid stale schema cache; Reset purges _schema_info too. + s.schemaInfo = nil + return nil }aerospike/aerospike_test.go (1)
27-43: Fix Aerospike test flakiness: wait for server readinessTests are failing with “FAIL_FORBIDDEN: Operation not allowed at this time.” Add an explicit wait for the ready log and port before returning the container.
import ( "context" "os" "testing" "time" "github.com/aerospike/aerospike-client-go/v8" "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go" tcaerospike "github.com/testcontainers/testcontainers-go/modules/aerospike" + "github.com/testcontainers/testcontainers-go/wait" ) @@ - // Start container - ctr, err := tcaerospike.Run(ctx, image) + // Start container and wait until Aerospike reports migrations complete and port is ready + ctr, err := tcaerospike.Run( + ctx, + image, + testcontainers.WithWaitStrategy( + wait.ForAll( + wait.ForListeningPort(aerospikePort), + wait.ForLog(aerospikeReadyLog), + ), + ), + ) testcontainers.CleanupContainer(t, ctr) require.NoError(t, err)mysql/mysql_test.go (2)
101-109: Don’t ignore sql.Open error and use NotNil for assertions.Handle error and simplify assertion.
- db, _ := sql.Open("mysql", dsn) + db, err := sql.Open("mysql", dsn) + require.NoError(t, err) ... - require.True(t, newConfigStore.db != nil) + require.NotNil(t, newConfigStore.db)
121-125: Fix Row.Scan usage: nil destinations are invalid.Scan requires pointer destinations; use throwaway vars.
- row := testStore.db.QueryRow(testStore.sqlSelect, "john") - err = row.Scan(nil, nil) + row := testStore.db.QueryRow(testStore.sqlSelect, "john") + var v []byte + var e int64 + err = row.Scan(&v, &e) require.Equal(t, sql.ErrNoRows, err)
🧹 Nitpick comments (11)
aerospike/aerospike.go (1)
192-195: Doc comment: use GoDoc style and clarify ownershipStart comment with the function name and clarify that callers must not close the returned client.
-// Return aerospike client +// Conn returns the underlying *aerospike.Client. +// The client is shared and managed by Storage; callers must not close it directly. func (s *Storage) Conn() *aerospike.Client {storage.go (1)
8-13: Interface looks good; tighten commentSmall wording tweak to avoid repetition and note optional nature.
-type StorageWithConn[T any] interface { - // Conn returns a connection to the storage. - // Implementations should return a connection to the storage, - // using the proper driver for the storage. - Conn() T -} +type StorageWithConn[T any] interface { + // Conn returns the underlying connection for this storage (driver/client specific). + // Exposing Conn is optional and not required by Storage. + Conn() T +}clickhouse/clickhouse.go (1)
114-117: LGTM: Conn accessor matches TCK needs; nit on commentConsider GoDoc style for discoverability.
-// Return clickhouse session +// Conn returns the underlying ClickHouse connection. func (s *Storage) Conn() driver.Conn {clickhouse/clickhouse_test.go (1)
86-95: LGTM: Conn() smoke testOptionally, assert Ping on the returned connection to catch early connectivity issues.
defer testStore.Close() - require.NotNil(t, testStore.Conn()) + conn := testStore.Conn() + require.NotNil(t, conn) + require.NoError(t, conn.Ping(context.Background()))testhelpers/tck/README.md (4)
150-154: Clarify generics section to match function signature and examples.State explicitly that tck.New has three type params and that you may omit all for inference. Align with the fixed example above.
161-163: Fix typos and grammar."immportant" → "important"; rephrase for clarity.
-The TCK automatically handles container cleanup, but ensure your `mustStart*` functions call `testcontainers.CleanupContainer(t, container)`. This is immportant when you add new Test functions that are not part of the TCK. +The TCK automatically handles container cleanup, but ensure your `mustStart*` functions call `testcontainers.CleanupContainer(t, container)`. This is important when you add new test functions that are not part of the TCK.
53-54: Style: prefer “verify” over “check.”Minor wording improvement.
-Please check that a Testcontainers module exists for the container type: visit ... +Please verify that a Testcontainers module exists for the container type: visit ...
38-45: Fix markdownlint MD010 (hard tabs).Replace tabs with spaces in the interface snippet to satisfy MD010.
- type TCKSuite[T storage.Storage, D any, C testcontainers.Container] interface { - // NewStore is a function that returns a new store. - // It is called by the [New] function to create a new store. - NewStore() func(ctx context.Context, tb testing.TB, ctr C) (T, error) - - // NewContainer is a function that returns a new container. - // It is called by the [New] function to create a new container. - NewContainer() func(ctx context.Context, tb testing.TB) (C, error) - } +type TCKSuite[T storage.Storage, D any, C testcontainers.Container] interface { + // NewStore is a function that returns a new store. + // It is called by the [New] function to create a new store. + NewStore() func(ctx context.Context, tb testing.TB, ctr C) (T, error) + + // NewContainer is a function that returns a new container. + // It is called by the [New] function to create a new container. + NewContainer() func(ctx context.Context, tb testing.TB) (C, error) +}testhelpers/tck/suite_test.go (1)
42-49: Call SetupSuite before SetupTest in the per-suite case.Better mirrors testify’s lifecycle and catches container/store init regressions.
t.Run("per-suite/setup-test", func(t *testing.T) { - s, err := New[*mockStorage, any, testcontainers.Container](context.Background(), t, &mockErrorSuite{}, PerSuite()) + s, err := New[*mockStorage, any, testcontainers.Container](context.Background(), t, &mockErrorSuite{}, PerSuite()) require.NoError(t, err) - s.SetupTest() + s.SetupSuite() + s.SetupTest() s.Require().NotNil(s.store) })mysql/mysql_test.go (1)
118-121: Use a slightly larger TTL to avoid flakiness.time.Nanosecond can be overly brittle across platforms.
- err := testStore.Set("john", testVal, time.Nanosecond) + err := testStore.Set("john", testVal, 1*time.Millisecond)testhelpers/tck/suite.go (1)
320-327: Tighten Eventually polling to reduce flakiness.Shorter interval, slightly longer timeout yields faster/stabler runs.
-}, 2*time.Second, 1*time.Second, "Key should expire") +}, 3*time.Second, 100*time.Millisecond, "Key should expire")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
mysql/go.modis excluded by!**/*.modtesthelpers/tck/go.modis excluded by!**/*.modtesthelpers/tck/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (9)
aerospike/aerospike.go(1 hunks)aerospike/aerospike_test.go(1 hunks)clickhouse/clickhouse.go(1 hunks)clickhouse/clickhouse_test.go(1 hunks)mysql/mysql_test.go(3 hunks)storage.go(1 hunks)testhelpers/tck/README.md(1 hunks)testhelpers/tck/suite.go(1 hunks)testhelpers/tck/suite_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-07-01T15:48:53.094Z
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-07-01T15:48:53.094Z
Learning: The `Test_Reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to ensure the storage is empty after a reset operation by checking that a previously set key returns an empty byte slice.
Applied to files:
clickhouse/clickhouse_test.gomysql/mysql_test.gotesthelpers/tck/suite.go
📚 Learning: 2024-11-23T19:50:06.387Z
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/datasources/database/db_mock.go:13-19
Timestamp: 2024-11-23T19:50:06.387Z
Learning: In test code within `clean-code/app/datasources/database/db_mock.go`, adding safety checks like context validation, safe type assertions, and extra documentation is not necessary.
Applied to files:
clickhouse/clickhouse_test.go
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
testhelpers/tck/suite_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.
Applied to files:
mysql/mysql_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
mysql/mysql_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
PR: gofiber/fiber#3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.
Applied to files:
mysql/mysql_test.go
🧬 Code graph analysis (6)
aerospike/aerospike.go (3)
clickhouse/clickhouse.go (1)
Storage(13-16)cassandra/cassandra.go (1)
Storage(23-30)surrealdb/surrealdb.go (1)
Storage(14-19)
clickhouse/clickhouse_test.go (1)
clickhouse/config.go (2)
Config(29-52)Memory(21-21)
clickhouse/clickhouse.go (1)
aerospike/aerospike.go (1)
Storage(12-19)
testhelpers/tck/suite_test.go (1)
testhelpers/tck/suite.go (2)
PerTest(52-58)PerSuite(61-67)
mysql/mysql_test.go (3)
storage.go (1)
Storage(17-52)mysql/mysql.go (2)
Storage(14-24)New(43-107)testhelpers/tck/suite.go (2)
New(87-108)PerTest(52-58)
testhelpers/tck/suite.go (2)
storage.go (2)
Storage(17-52)StorageWithConn(8-13)mockstorage/mockstorage_test.go (1)
TestGetWithContext(281-304)
🪛 GitHub Actions: Tests Aerospike
aerospike/aerospike_test.go
[error] 84-84: go test ./... -v -race failed: Test_AeroSpikeDB_Get failed with FAIL_FORBIDDEN: Operation not allowed at this time.
[error] 105-105: go test ./... -v -race failed: Test_AeroSpikeDB_Delete failed with FAIL_FORBIDDEN: Operation not allowed at this time.
[error] 129-129: go test ./... -v -race failed: Test_AeroSpikeDB_SetWithExpiration failed with FAIL_FORBIDDEN: Operation not allowed at this time.
[error] 160-160: go test ./... -v -race failed: Test_AeroSpikeDB_Reset failed with FAIL_FORBIDDEN: Operation not allowed at this time.
🪛 LanguageTool
testhelpers/tck/README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...etc.) - TTL (Time-To-Live) functionality - Storage reset and cleanup - Connection h...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...unctionality - Storage reset and cleanup - Connection handling for stores that impl...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...te storage type (e.g., *mysql.Storage) - D: The driver type returned by Conn() m...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ned by Conn() method (e.g., *sql.DB) - C: The testcontainer type (e.g., `*mysql....
(QB_NEW_EN)
[style] ~53-~53: Try using a more formal synonym for ‘check’.
Context: ...(e.g., *mysql.MySQLContainer) Please check that a Testcontainers module exists for...
(VERIFY)
[grammar] ~150-~150: There might be a mistake here.
Context: ... tck.New(), specify the correct types: - First parameter (T): Your storage po...
(QB_NEW_EN)
[grammar] ~151-~151: There might be a mistake here.
Context: ... storage pointer type (e.g., *Storage) - Second parameter (D): Driver type fr...
(QB_NEW_EN)
[grammar] ~152-~152: There might be a mistake here.
Context: ...nn()method, orany if not applicable - **Third parameter** (C`): Container type ...
(QB_NEW_EN)
[grammar] ~161-~161: Ensure spelling is correct
Context: ...leanupContainer(t, container)`. This is immportant when you add new Test functions that ar...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~161-~161: There might be a mistake here.
Context: ... functions that are not part of the TCK. If they need a store (and its container)...
(QB_NEW_EN)
[grammar] ~166-~166: There might be a mistake here.
Context: ... storage with appropriate test settings: - Enable Reset: true if your storage sup...
(QB_NEW_EN)
[grammar] ~177-~177: There might be a mistake here.
Context: ...cenarios ### PerTest Mode (Recommended) Use when you need complete isolation bet...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...torageTCK{}, tck.PerTest()) ``` Pros: - Complete test isolation - No cross-test ...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ...``` Pros: - Complete test isolation - No cross-test contamination - Easier deb...
(QB_NEW_EN)
[grammar] ~186-~186: There might be a mistake here.
Context: ... isolation - No cross-test contamination - Easier debugging of individual test fail...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...ing of individual test failures Cons: - Slower execution due to container startu...
(QB_NEW_EN)
[grammar] ~190-~190: There might be a mistake here.
Context: ...cution due to container startup overhead - Higher resource usage, although mitigate...
(QB_NEW_EN)
[grammar] ~193-~193: There might be a mistake here.
Context: ...rs' cleanup mechanism ### PerSuite Mode Use when container startup is expensive ...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...orageTCK{}, tck.PerSuite()) ``` Pros: - Faster execution - Lower resource usage ...
(QB_NEW_EN)
[grammar] ~201-~201: There might be a mistake here.
Context: ...ite()) ``` Pros: - Faster execution - Lower resource usage Cons: - Tests ...
(QB_NEW_EN)
[grammar] ~204-~204: There might be a mistake here.
Context: ...xecution - Lower resource usage Cons: - Tests may affect each other - Requires c...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
testhelpers/tck/README.md
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: compare (clickhouse)
- GitHub Check: compare (mysql)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
🔇 Additional comments (3)
aerospike/aerospike_test.go (1)
65-70: LGTM: basic Conn smoke testmysql/mysql_test.go (1)
20-25: No changes needed for mysqlImage tag
Thedocker.io/mysql:9image exists and is actively maintained, so the default tag is valid and CI will not break.testhelpers/tck/suite.go (1)
256-262: Close the store in TearDownTest for PerSuite.Without this, each test leaks a store handle.
func (s *StorageTestSuite[T, D, C]) TearDownTest() { - if s.hook == perTest { - s.Require().NoError(s.cleanup()) - } + switch s.hook { + case perTest: + s.Require().NoError(s.cleanup()) + case perSuite: + if s.store != nil && !s.closedStore { + s.Require().NoError(s.store.Close()) + s.closedStore = true + } + } }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
testhelpers/tck/README.md (1)
69-97: Unify types in the example (ExampleContainer vs example.Container).The function signatures mix
*ExampleContainerand*example.Container; use one consistently.-func (s *ExampleStorageTCK) NewStore() func(ctx context.Context, tb testing.TB, ctr *ExampleContainer) (*Storage, error) { - return func(ctx context.Context, tb testing.TB, ctr *example.Container) (*Storage, error) { +func (s *ExampleStorageTCK) NewStore() func(ctx context.Context, tb testing.TB, ctr *ExampleContainer) (*Storage, error) { + return func(ctx context.Context, tb testing.TB, ctr *ExampleContainer) (*Storage, error) { conn, err := ctr.ConnectionString(ctx) require.NoError(tb, err) @@ -func (s *ExampleStorageTCK) NewContainer() func(ctx context.Context, tb testing.TB) (*example.Container, error) { - return func(ctx context.Context, tb testing.TB) (*example.Container, error) { +func (s *ExampleStorageTCK) NewContainer() func(ctx context.Context, tb testing.TB) (*ExampleContainer, error) { + return func(ctx context.Context, tb testing.TB) (*ExampleContainer, error) { return mustStartExample(tb), nil } }
🧹 Nitpick comments (5)
testhelpers/tck/README.md (5)
62-62: Fix duplicated heading text (“Example Example”).Change to “Implementation Guide: Example”.
-## Implementation Guide: Example Example +## Implementation Guide: Example
53-54: Tighten phrasing (“Please check” → “Please verify”).More formal wording and clarify “Testcontainers module container type”.
-Please check that a Testcontainers module exists for the container type: visit [testcontainers website](https://testcontainers.com/modules/?language=go) for more information. +Please verify that a suitable Testcontainers module exists for your container type. See the [Testcontainers modules catalog](https://testcontainers.com/modules/?language=go) for details.
148-152: Clarify generic parameter descriptions.Minor grammar and capitalization tweaks for readability.
-When calling `tck.New()`, specify the correct types: -- **First parameter** (`T`): Your storage pointer type (e.g., `*Storage`) -- **Second parameter** (`D`): Driver type from `Conn()` method, or `any` if not applicable -- **Third parameter** (`C`): Container type from `NewContainer()` method +When calling `tck.New`, specify the correct type parameters: +- `T`: Your storage pointer type (e.g., `*Storage`) +- `D`: The driver type returned by `Conn()` (or `any` if not applicable) +- `C`: The container type returned by `NewContainer()`
159-161: Tighten cleanup guidance and fix grammar.Streamline the paragraph and avoid future-tense ambiguity.
-The TCK automatically handles container cleanup, but ensure your `mustStart*` functions call `testcontainers.CleanupContainer(t, container)`. This is important when you add new Test functions that are not part of the TCK. -If they need a store (and its container), they must call `testcontainers.CleanupContainer(t, container)` to avoid having those containers running until the end of the test execution. At that time, Testcontainers' Ryuk prunes all containers for you, but you probably want to clean up the container immediately. +The TCK handles container cleanup, but ensure your `mustStart*` helpers call `testcontainers.CleanupContainer(t, container)`. For ad‑hoc tests outside the TCK, call `CleanupContainer` to avoid leaving containers running until the test process exits. Although Ryuk will prune them, it’s better to clean up immediately.
191-205: Stay consistent with the example naming.These snippets switch to
MySQLStorageTCKwhile the guide usesExample*. Consider using a single storage name throughout to reduce copy/paste errors.-s, err := tck.New(ctx, t, &MySQLStorageTCK{}, tck.PerSuite()) +s, err := tck.New(ctx, t, &ExampleStorageTCK{}, tck.PerSuite())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
testhelpers/tck/README.md(1 hunks)testhelpers/tck/suite.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- testhelpers/tck/suite.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
testhelpers/tck/README.md
🪛 LanguageTool
testhelpers/tck/README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...etc.) - TTL (Time-To-Live) functionality - Storage reset and cleanup - Connection h...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...unctionality - Storage reset and cleanup - Connection handling for stores that impl...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...te storage type (e.g., *mysql.Storage) - D: The driver type returned by Conn() m...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ned by Conn() method (e.g., *sql.DB) - C: The testcontainer type (e.g., `*mysql....
(QB_NEW_EN)
[style] ~53-~53: Try using a more formal synonym for ‘check’.
Context: ...(e.g., *mysql.MySQLContainer) Please check that a Testcontainers module exists for...
(VERIFY)
[grammar] ~148-~148: There might be a mistake here.
Context: ... tck.New(), specify the correct types: - First parameter (T): Your storage po...
(QB_NEW_EN)
[grammar] ~149-~149: There might be a mistake here.
Context: ... storage pointer type (e.g., *Storage) - Second parameter (D): Driver type fr...
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...nn()method, orany if not applicable - **Third parameter** (C`): Container type ...
(QB_NEW_EN)
[grammar] ~159-~159: There might be a mistake here.
Context: ... functions that are not part of the TCK. If they need a store (and its container)...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ... storage with appropriate test settings: - Enable Reset: true if your storage sup...
(QB_NEW_EN)
[grammar] ~175-~175: There might be a mistake here.
Context: ...cenarios ### PerTest Mode (Recommended) Use when you need complete isolation bet...
(QB_NEW_EN)
[grammar] ~182-~182: There might be a mistake here.
Context: ...torageTCK{}, tck.PerTest()) ``` Pros: - Complete test isolation - No cross-test ...
(QB_NEW_EN)
[grammar] ~183-~183: There might be a mistake here.
Context: ...``` Pros: - Complete test isolation - No cross-test contamination - Easier deb...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ... isolation - No cross-test contamination - Easier debugging of individual test fail...
(QB_NEW_EN)
[grammar] ~187-~187: There might be a mistake here.
Context: ...ing of individual test failures Cons: - Slower execution due to container startu...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ...cution due to container startup overhead - Higher resource usage, although mitigate...
(QB_NEW_EN)
[grammar] ~191-~191: There might be a mistake here.
Context: ...rs' cleanup mechanism ### PerSuite Mode Use when container startup is expensive ...
(QB_NEW_EN)
[grammar] ~198-~198: There might be a mistake here.
Context: ...orageTCK{}, tck.PerSuite()) ``` Pros: - Faster execution - Lower resource usage ...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...ite()) ``` Pros: - Faster execution - Lower resource usage Cons: - Tests ...
(QB_NEW_EN)
[grammar] ~202-~202: There might be a mistake here.
Context: ...xecution - Lower resource usage Cons: - Tests may affect each other - Requires c...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
testhelpers/tck/README.md
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: compare (clickhouse)
- GitHub Check: compare (mysql)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
🔇 Additional comments (1)
testhelpers/tck/README.md (1)
101-124: Good: cleanup is called before error assertion (safe with testcontainers-go).This matches prior learnings that
testcontainers.CleanupContainer(t, c)is safe even ifcis nil/partially initialized.
|
I was intending to make a solution like this, but you took it further and did even better. I truly appreciate your work. |
|
@mdelapenya Can i handle other storage implementations? |
|
Yes please! I'd wait until the team reviews and approves this PR. Once merged, then feel free to continue continuing 🙏 |
|
@ReneWerner87 I'm not sure why aerospike's tests fail. Seems an error from the latest docker image. I can remove the commits in this PR adding the Conn method to aerospike and clickhouse, to reduce the surface of this PR, and investigate the aerospike error in a follow-up PR (at some point, anybody touching that module will face it). |
|
OK, tests for MySQL are passing! The cool thing of this TCK is that all modules implementing will have for free the same tests: |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
testhelpers/tck/README.md (3)
179-181: Fix tck.New generics (missing third type parameter).
tck.Newexpects<T, D, C>. Prefer inference for readability.Apply one of:
-s, err := tck.New[*Storage, *sql.DB](ctx, t, &ExampleStorageTCK{}, tck.PerTest()) +s, err := tck.New(ctx, t, &ExampleStorageTCK{}, tck.PerTest())or explicitly:
-s, err := tck.New[*Storage, *sql.DB](ctx, t, &ExampleStorageTCK{}, tck.PerTest()) +s, err := tck.New[*Storage, *sql.DB, *example.Container](ctx, t, &ExampleStorageTCK{}, tck.PerTest())
195-197: Fix tck.New generics (missing third type parameter).Same issue in PerSuite example.
Apply one of:
-s, err := tck.New[*Storage, *sql.DB](ctx, t, &ExampleStorageTCK{}, tck.PerSuite()) +s, err := tck.New(ctx, t, &ExampleStorageTCK{}, tck.PerSuite())or:
-s, err := tck.New[*Storage, *sql.DB](ctx, t, &ExampleStorageTCK{}, tck.PerSuite()) +s, err := tck.New[*Storage, *sql.DB, *example.Container](ctx, t, &ExampleStorageTCK{}, tck.PerSuite())
69-99: Fix inconsistent types in the example (ExampleContainer vs example.Container).Signatures and bodies disagree on the container type, which invites copy/paste bugs.
Apply:
-func (s *ExampleStorageTCK) NewStore() func(ctx context.Context, tb testing.TB, ctr *ExampleContainer) (*Storage, error) { - return func(ctx context.Context, tb testing.TB, ctr *example.Container) (*Storage, error) { +func (s *ExampleStorageTCK) NewStore() func(ctx context.Context, tb testing.TB, ctr *example.Container) (*Storage, error) { + return func(ctx context.Context, tb testing.TB, ctr *example.Container) (*Storage, error) {Also ensure Step 3 uses the same container type:
-s, err := tck.New[*ExampleStorage, *ExampleDriver, *ExampleContainer]( +s, err := tck.New[*ExampleStorage, *ExampleDriver, *example.Container](
🧹 Nitpick comments (4)
testhelpers/tck/README.md (4)
13-14: Clarify Conn wording to avoid interface-name drift.Prefer neutral phrasing so the doc stays correct if the gating interface name changes. Also double-check the actual interface name in code (StorageWithConn vs StorageWithoutConn) and align consistently throughout the repo.
Apply:
-- Connection handling for stores that implement `StorageWithConn` +- Connection handling when Conn() is supported
38-45: Replace hard tabs with spaces (markdownlint MD010).Tabs in this code block will fail common linters.
Apply:
type TCKSuite[T storage.Storage, D any, C testcontainers.Container] interface { - // NewStore is a function that returns a new store. - // It is called by the [New] function to create a new store. - NewStore() func(ctx context.Context, tb testing.TB, ctr C) (T, error) + // NewStore is a function that returns a new store. + // It is called by the [New] function to create a new store. + NewStore() func(ctx context.Context, tb testing.TB, ctr C) (T, error) - // NewContainer is a function that returns a new container. - // It is called by the [New] function to create a new container. - NewContainer() func(ctx context.Context, tb testing.TB) (C, error) + // NewContainer is a function that returns a new container. + // It is called by the [New] function to create a new container. + NewContainer() func(ctx context.Context, tb testing.TB) (C, error) }
48-52: Use correct library terminology (“Testcontainers”).Minor wording fix for accuracy.
Apply:
-- `C`: The testcontainer type (e.g., `*mysql.MySQLContainer`) +- `C`: The Testcontainers module type (e.g., `*mysql.MySQLContainer`)
161-162: Normalize “ad‑hoc” to “ad hoc”.Removes the special hyphen and matches common spelling.
Apply:
-The TCK handles container cleanup, but ensure your `mustStart*` helpers call `testcontainers.CleanupContainer(t, container)`. For ad‑hoc tests outside the TCK, call `CleanupContainer` to avoid leaving containers running until the test process exits. Although Ryuk will prune them, it’s better to clean up immediately. +The TCK handles container cleanup, but ensure your `mustStart*` helpers call `testcontainers.CleanupContainer(t, container)`. For ad hoc tests outside the TCK, call `CleanupContainer` to avoid leaving containers running until the test process exits. Although Ryuk will prune them, it’s better to clean up immediately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
testhelpers/tck/README.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
testhelpers/tck/README.md
🪛 LanguageTool
testhelpers/tck/README.md
[grammar] ~11-~11: There might be a mistake here.
Context: ...etc.) - TTL (Time-To-Live) functionality - Storage reset and cleanup - Connection h...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...unctionality - Storage reset and cleanup - Connection handling for stores that impl...
(QB_NEW_EN)
[grammar] ~49-~49: There might be a mistake here.
Context: ...te storage type (e.g., *mysql.Storage) - D: The driver type returned by Conn() m...
(QB_NEW_EN)
[grammar] ~50-~50: There might be a mistake here.
Context: ...ned by Conn() method (e.g., *sql.DB) - C: The testcontainer type (e.g., `*mysql....
(QB_NEW_EN)
[grammar] ~150-~150: There might be a mistake here.
Context: ...w, specify the correct type parameters: - T: Your storage pointer type (e.g., *Sto...
(QB_NEW_EN)
[grammar] ~151-~151: There might be a mistake here.
Context: ... storage pointer type (e.g., *Storage) - D: The driver type returned by Conn() (...
(QB_NEW_EN)
[grammar] ~152-~152: There might be a mistake here.
Context: ...by Conn() (or any if not applicable) - C: The container type returned by `NewCon...
(QB_NEW_EN)
[grammar] ~161-~161: Ensure spelling is correct
Context: ...rs.CleanupContainer(t, container). For ad‑hoc tests outside the TCK, call CleanupCon...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~165-~165: There might be a mistake here.
Context: ... storage with appropriate test settings: - Enable Reset: true if your storage sup...
(QB_NEW_EN)
[grammar] ~176-~176: There might be a mistake here.
Context: ...cenarios ### PerTest Mode (Recommended) Use when you need complete isolation bet...
(QB_NEW_EN)
[grammar] ~183-~183: There might be a mistake here.
Context: ...torageTCK{}, tck.PerTest()) ``` Pros: - Complete test isolation - No cross-test ...
(QB_NEW_EN)
[grammar] ~184-~184: There might be a mistake here.
Context: ...``` Pros: - Complete test isolation - No cross-test contamination - Easier deb...
(QB_NEW_EN)
[grammar] ~185-~185: There might be a mistake here.
Context: ... isolation - No cross-test contamination - Easier debugging of individual test fail...
(QB_NEW_EN)
[grammar] ~188-~188: There might be a mistake here.
Context: ...ing of individual test failures Cons: - Slower execution due to container startu...
(QB_NEW_EN)
[grammar] ~189-~189: There might be a mistake here.
Context: ...cution due to container startup overhead - Higher resource usage, although mitigate...
(QB_NEW_EN)
[grammar] ~192-~192: There might be a mistake here.
Context: ...rs' cleanup mechanism ### PerSuite Mode Use when container startup is expensive ...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...orageTCK{}, tck.PerSuite()) ``` Pros: - Faster execution - Lower resource usage ...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...ite()) ``` Pros: - Faster execution - Lower resource usage Cons: - Tests ...
(QB_NEW_EN)
[grammar] ~203-~203: There might be a mistake here.
Context: ...xecution - Lower resource usage Cons: - Tests may affect each other - Requires c...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
testhelpers/tck/README.md
38-38: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
42-42: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
|
@ReneWerner87 what are your thoughts on this one? |
|
@mdelapenya I consider this feature to be a very useful enhancement. A central TCK reduces duplication in the storage modules, creates clear semantics and makes onboarding new backends much easier. Looks good, I think we could establish this everywhere. |
|
@muhammedkucukaslan gates are open for you! |
|
Thanks a lot for your work, @mdelapenya. I’ll cover other storages within this week. |
3 participants
What does this PR do?
It adds a new test module with a TCK (Test Compatibility Kit). This TCK will allow all storage implementations to share the same behaviours at test time, so it's easier to add a new storage and verify it satisfies the Storage interface semantics.
It converts the tests in the MySQL submodule as reference to verify whether this PR is valuable or not.
This PR also adds documentation for the TCK, with technical details of its implementation and a guide on how to use it in a given Storage.
Finally, unit tests for the TCK are present, using a mock storage based on sync.Map.
IMPORTANT: Working on this, I noticed that both Aerospike and Clickhouse referenced the Conn method in their docs, but did not implemented it. So I implemented them in both modules. You'll notice that I added a new small interface for StorageWithConn, used in the TCK to skip the test for the Conn if a storage does not have a Conn method. I on purpose did not want to increase the size of the existing Storage interface, but I'm open to add the Conn method in there if you consider it, removing the new interface.
Why is it important?
Simplify the codebase by abstracting all the tests in the submodules into the TCK, making possible the removal of all the duplicated tests across the modules.
Related work
Follow-ups
If this PR seen as valuable, I can work on the migration of the rest of the modules to the TCK.
Summary by CodeRabbit
New Features
Documentation
Tests