refactor: optimize SurrealDB tests with test suite pattern#1855
refactor: optimize SurrealDB tests with test suite pattern#1855muhammedkucukaslan wants to merge 0 commit into
Conversation
muhammedkucukaslan
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
WalkthroughThe SurrealDB test suite was refactored from standalone test functions to a structured suite using Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant SurrealDBTestSuite
participant SurrealDBContainer
participant Storage
TestRunner->>SurrealDBTestSuite: SetupSuite()
SurrealDBTestSuite->>SurrealDBContainer: createSurrealDBContainer()
SurrealDBTestSuite->>Storage: createStore()
loop For each test
TestRunner->>SurrealDBTestSuite: SetupTest()
SurrealDBTestSuite->>Storage: Reset state
TestRunner->>SurrealDBTestSuite: Run test method (e.g., TestCreate)
SurrealDBTestSuite->>Storage: setValue/getValue/...
end
TestRunner->>SurrealDBTestSuite: TearDownSuite()
SurrealDBTestSuite->>SurrealDBContainer: Terminate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 comments)
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 @muhammedkucukaslan, 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!
I've refactored the SurrealDB test suite to enhance its structure, readability, and maintainability. This involved adopting the testify/suite package, abstracting common test setup and teardown logic, and streamlining the overall testing approach.
Highlights
- Test Suite Integration: I've integrated
testify/suiteto create aSurrealDBTestSuite, centralizing test logic and replacing individualTest_Surrealdb_*functions. - Boilerplate Reduction: I've eliminated repetitive test setup code by moving container initialization and store closing into
SetupSuiteandTearDownSuite, and introduced helper methods for common assertions. - Naming Convention Improvement: I've updated variable names like
surrealDbtodefaultSurrealDBImageandsurrealDbUsertotestUserfor better clarity and consistency. - Test Optimization: I've removed redundant tests, specifically
Test_Surrealdb_Get_WithNoErrandTest_Surrealdb_GarbageCollector_RemovesExpiredKeys, as their coverage is now provided by other, more comprehensive tests. - Benchmark Refactoring: I've updated all benchmarks to utilize a new
createBenchmarkStorehelper, ensuring a cleaner and more consistent test container setup for performance tests.
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 or fill out our survey to provide feedback.
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 is a great refactoring of the SurrealDB tests. By introducing testify/suite, you've significantly reduced boilerplate code and improved the structure and readability of the tests. The setup and teardown logic is now cleanly handled by the suite, and helper methods make individual tests much more concise. The improvements to benchmark tests, especially using unique keys in BenchmarkSetAndDelete, make them more realistic and valuable. I have one suggestion to improve test robustness by replacing a time.Sleep with testify/suite.Eventually to avoid potential flakiness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
surrealdb/surrealdb_test.go (1)
151-162: Consider using unique keys in BenchmarkSet for more realistic benchmarking.Using the same key repeatedly might benefit from database-level optimizations that wouldn't occur in real usage patterns.
for i := 0; i < b.N; i++ { - err := store.Set("benchmark_key", []byte("benchmark_value"), 0) + key := fmt.Sprintf("benchmark_key_%d", i) + err := store.Set(key, []byte("benchmark_value"), 0) require.NoError(b, err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
surrealdb/surrealdb_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/datasources/database/memory_db_test.go:0-0
Timestamp: 2024-11-25T19:36:45.661Z
Learning: In the `clean-code/app/datasources/database/memory_db.go`, the in-memory database implementation is not intended for production use. Therefore, adding extensive tests for concurrency and error handling in `memory_db_test.go` is unnecessary.
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
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.
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Learnt from: ReneWerner87
PR: gofiber/storage#0
File: :0-0
Timestamp: 2025-02-12T11:24:31.153Z
Learning: The storage package in gofiber/storage repository can be used independently of the Fiber web framework.
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.
Learnt from: juls0730
PR: gofiber/recipes#2710
File: tableflip/main.go:61-62
Timestamp: 2024-12-01T01:15:48.126Z
Learning: In the GoFiber `tableflip` recipe (`tableflip/main.go`), the implementation matches the upstream reference implementation. Future code suggestions should consider maintaining this alignment to ensure consistency.
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.
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
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`.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Learnt from: CR
PR: gofiber/fiber#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T15:13:10.652Z
Learning: Run `make test` to run the test suite when modifying code
📚 Learning: in go test files, prefer using the `require` methods from the `testify` package for assertions inste...
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:
surrealdb/surrealdb_test.go
📚 Learning: the project uses the testify/assert package for assertions in unit tests....
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
surrealdb/surrealdb_test.go
📚 Learning: unit tests in this project use testify require....
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:
surrealdb/surrealdb_test.go
📚 Learning: parallel tests for `session.save` already exist in the `middleware/session/session_test.go` file, sp...
Learnt from: sixcolors
PR: gofiber/fiber#3051
File: middleware/session/session.go:215-216
Timestamp: 2024-06-30T00:38:06.580Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.
Applied to files:
surrealdb/surrealdb_test.go
📚 Learning: the `test_reset` function in `clickhouse/clickhouse_test.go` already includes a verification step to...
Learnt from: luk3skyw4lker
PR: gofiber/storage#1342
File: clickhouse/clickhouse_test.go:138-160
Timestamp: 2024-10-08T19:06:06.583Z
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:
surrealdb/surrealdb_test.go
📚 Learning: in the `clean-code/app/datasources/database/memory_db.go`, the in-memory database implementation is ...
Learnt from: norri
PR: gofiber/recipes#2701
File: clean-code/app/datasources/database/memory_db_test.go:0-0
Timestamp: 2024-11-25T19:36:45.661Z
Learning: In the `clean-code/app/datasources/database/memory_db.go`, the in-memory database implementation is not intended for production use. Therefore, adding extensive tests for concurrency and error handling in `memory_db_test.go` is unnecessary.
Applied to files:
surrealdb/surrealdb_test.go
📚 Learning: when testing session `idletimeout` expiration, it's acceptable to use `time.sleep` to simulate the p...
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.
Applied to files:
surrealdb/surrealdb_test.go
🔇 Additional comments (9)
surrealdb/surrealdb_test.go (9)
3-15: LGTM! Imports are well-organized and appropriate.The addition of
fmtfor benchmark key generation and the testify/suite packages aligns well with the test suite refactoring pattern.
17-25: Good naming improvements for constants.The renamed constants (
defaultSurrealDBImage,imageEnvVar,testUser,testPass) are more descriptive and follow Go naming conventions better than the previous names.
27-54: Well-structured test suite with proper lifecycle management.The suite pattern with SetupSuite/TearDownSuite for container lifecycle and SetupTest for test isolation is excellent. This reduces test execution time by reusing the container across tests.
Minor note: The nil checks in
TearDownSuite(lines 43 and 46) are defensive but technically unnecessary sinceSetupSuiteensures these are always initialized. IfSetupSuitefails, the tests won't run.
60-78: Test methods are well-structured with clear assertions.The use of helper methods (
assertKeyHasValue,assertKeyNotExists) improves readability and maintains consistency across tests.
80-117: Comprehensive tests for Reset and List functionality.Both tests properly verify their respective operations with appropriate setup and assertions. The TestList method correctly validates the JSON output structure.
119-128: Good use of Eventually for expiration testing.Using
Eventuallywith polling is the right approach for testing TTL expiration, avoiding flaky tests while minimizing wait time.
164-195: Well-structured benchmark functions with proper cleanup.The use of
createBenchmarkStorehelper and defer cleanup ensures consistent setup/teardown. BenchmarkSetAndDelete correctly uses unique keys per iteration.
197-241: Excellent helper functions that promote code reuse.The helper functions provide clean abstractions:
createSurrealDBContainerproperly handles environment variable override for the imagecreateBenchmarkStorewith its cleanup function ensures proper resource managementcreateStorecentralizes configuration
243-262: Clean helper methods that enhance test readability.The helper methods (
setValue,getValue,assertKeyHasValue,assertKeyNotExists) effectively encapsulate common operations and maintain consistency in error handling and assertions throughout the test suite.
| func (s *SurrealDBTestSuite) TestListSkipsExpiredKeys() { | ||
| s.setValue("permanent", []byte("perm_value"), 0) | ||
| s.setValue("temporary", []byte("temp_value"), 500*time.Millisecond) | ||
|
|
||
| _ = testStore.Set("valid", []byte("123"), 0) | ||
|
|
||
| _ = testStore.Set("expired", []byte("456"), 1*time.Second) | ||
| time.Sleep(2 * time.Second) | ||
| time.Sleep(3 * time.Second) | ||
|
|
||
| data, err := testStore.List() | ||
| require.NoError(t, err) | ||
| data, err := s.store.List() | ||
| s.Require().NoError(err) | ||
|
|
||
| var result map[string][]byte | ||
| err = json.Unmarshal(data, &result) | ||
| require.NoError(t, err) | ||
| s.Require().NoError(err) | ||
|
|
||
| require.Contains(t, result, "valid") | ||
| require.NotContains(t, result, "expired") | ||
| s.Assert().Contains(result, "permanent") | ||
| s.Assert().NotContains(result, "temporary") | ||
| } |
There was a problem hiding this comment.
Excessive sleep duration in TestListSkipsExpiredKeys.
The test uses time.Sleep(3 * time.Second) when the TTL is only 500ms. This unnecessarily slows down the test suite.
Consider using Eventually pattern or reducing the sleep to a more appropriate duration:
- time.Sleep(3 * time.Second)
+ // Wait just enough for the key to expire with a small buffer
+ time.Sleep(700 * time.Millisecond)Or better yet, use the Eventually pattern like in TestGetExpired:
- time.Sleep(3 * time.Second)
+ s.Eventually(func() bool {
+ data, err := s.store.List()
+ if err != nil {
+ return false
+ }
+ var result map[string][]byte
+ err = json.Unmarshal(data, &result)
+ if err != nil {
+ return false
+ }
+ _, exists := result["temporary"]
+ return !exists
+ }, 2*time.Second, 100*time.Millisecond, "Temporary key should expire")🤖 Prompt for AI Agents
In surrealdb/surrealdb_test.go around lines 130 to 145, the test uses a 3-second
sleep to wait for a 500ms TTL to expire, which slows down the test
unnecessarily. Replace the fixed sleep with a shorter duration slightly longer
than 500ms or implement an Eventually pattern that repeatedly checks if the
"temporary" key has expired within a timeout, similar to the approach used in
TestGetExpired, to make the test faster and more reliable.
|
@mdelapenya WDYT? |
|
Hi, just wanted to kindly follow up on this PR. If the test suite pattern for the SurrealDB tests isn’t the preferred direction, I’d be glad to hear alternative suggestions. |
|
I'm currently on PTO, will go back to this PR in a week or so 🙏 |
|
No worries, enjoy your PTO |
mdelapenya
left a comment
mdelapenya
left a comment
There was a problem hiding this comment.
@muhammedkucukaslan thanks for this PR, it gives me a great idea for a bigger refactor abstracting the suite for all the stores (in a follow-up, not here 😅).
I left just few comments, hopefully we can discuss about them.
Cheers, and thanks for your contribution!
| if s.container != nil { | ||
| testcontainers.CleanupContainer(s.T(), s.container) | ||
| } |
There was a problem hiding this comment.
suggestion: CleanupContainer already handles nils
| require.NotNil(t, val) | ||
| data, err := s.store.List() | ||
| s.Require().NoError(err) | ||
| s.Assert().NotEmpty(data) |
There was a problem hiding this comment.
question: why do we need to change from require to assert? This would apply to all usages of assert Vs require. IMO I'd stand with require to avoid adding noise to test failure messages
There was a problem hiding this comment.
you are on point. It escaped me.
| return val | ||
| } | ||
|
|
||
| func (s *SurrealDBTestSuite) assertKeyHasValue(key string, expectedValue []byte) { |
There was a problem hiding this comment.
suggestion: I'd stand with require, instead of assert. More in the rest of the PR
| if s.store != nil { | ||
| s.store.Close() | ||
| } | ||
| if s.container != nil { |
There was a problem hiding this comment.
suggestion: this will destroy the container after the test suite, but we cannot be not sure if the data created in the tests is properly cleaned up. I'd start with a container per test method, so I'd leverage the BeforeTest and AfterTest hooks in the suite.
There was a problem hiding this comment.
but what is the point of using suite? I wanted to escape creating a container for each test by opening this PR. However, you're right to say that we can not be sure that the data created is cleaned up perfectly.
So, i think, creating a namespace for each test is our answer. In this way, we will keep using one container and have izolated environment for each test.
func (s *SurrealDBTestSuite) SetupTest() {
testID := time.Now().UnixNano()
uniqueNamespace := fmt.Sprintf("test_%d", testID)
err := s.env.store.db.Use(uniqueNamespace, testDatabase)
s.Require().NoError(err)
}There was a problem hiding this comment.
but what is the point of using suite? I wanted to escape creating a container for each test by opening this PR. However, you're right to say that we can not be sure that the data created is cleaned up perfectly.
So, i think, creating a namespace for each test is our answer. In this way, we will keep using one container and have izolated environment for each test.
func (s *SurrealDBTestSuite) SetupTest() {
testID := time.Now().UnixNano()
uniqueNamespace := fmt.Sprintf("test_%d", testID)
err := s.env.store.db.Use(uniqueNamespace, testDatabase)
s.Require().NoError(err)
}|
|
||
| err := testStore.Set("test", []byte("test12345"), 0) | ||
| require.NoError(t, err) | ||
| container, url := createSurrealDBContainer(s.ctx, s.T()) |
There was a problem hiding this comment.
suggestion: I'd return the container and the store in this method. The URL is an implementation detail of the underlying container type (mysql, postgres, scylladb, etc calculate the URL with different APIs). Here it's used to just create the store. We can probably have an intermediate type having the store and the container, and populate an instance of this type per-test or per-suite (see my other comment regarding the suite lifecycle)
There was a problem hiding this comment.
nice idea, so i named our intermediate type as env and it is like:
type env struct {
store *Storage
container testcontainers.Container
} so here became like
| container, url := createSurrealDBContainer(s.ctx, s.T()) | |
| env := createSurrealDBContainer(s.ctx, s.T()) | |
| s.env = env |
|
|
||
| require.NoError(b, err) | ||
| // Helper methods | ||
| func createSurrealDBContainer(ctx context.Context, t require.TestingT) (testcontainers.Container, string) { |
There was a problem hiding this comment.
| func createSurrealDBContainer(ctx context.Context, t require.TestingT) (testcontainers.Container, string) { | |
| func createSurrealDBContainer(ctx context.Context, t testing.T) (testcontainers.Container, string) { |
muhammedkucukaslan
force-pushed
the
main
branch
from
e6be994 to
7c6c81b
Compare
3 participants
✨ What’s Changed?
This PR introduces a new SurrealDBTestSuite based on testify/suite and refactors the existing test file to improve readability, maintainability, and reduce code duplication.
1. Introduced testify/suite
Replaced scattered
Test_Surrealdb_*functions with a centralized SurrealDBTestSuite using testify/suite.2. Removed repetitive boilerplate
Previously, almost every test included:
This has been abstracted into the suite lifecycle. Also, common test logic (e.g., set/get/assert helpers) is now reused via helper methods (setValue, getValue, assertKeyHasValue, etc.).
3. Improved global variable naming
Updated confusing or ambiguous variable names like:
4. Removed redundant tests
Removed
Test_Surrealdb_Get_WithNoErr: This test was already covered byTestCreateAndGetwith more complete assertions.Removed
Test_Surrealdb_GarbageCollector_RemovesExpiredKeys: This logic is doing same thing withTestGetExpired.5. Refactor Benchmarks
All benchmarks have been updated to use a cleaner test container setup with
createBenchmarkStore.Summary by CodeRabbit