chore(testing): use require in tests #1672
Conversation
mdelapenya
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThis set of changes refactors test helper functions across multiple storage backend test files. The main alteration is the removal of error returns from test setup helpers like Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Helper as newTestStore(t)
participant Container as Test Container
participant Storage as *Storage
Test->>Helper: Call newTestStore(t)
Helper->>Container: Start container (if needed)
Helper->>Helper: require.NoError on setup
Helper->>Storage: Initialize Storage instance
Helper->>Test: Return *Storage
Test->>Storage: Use storage in test
Test->>Storage: Defer Close()
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clickhouse/clickhouse_test.go (1)
244-254: Inconsistent benchmark timing in Set_And_Delete benchmark.Unlike other benchmarks, this one has
b.ReportAllocs()andb.ResetTimer()calls before client creation, which means the benchmark includes the time to create the client in its measurements. Consider moving these calls after client creation for consistency with other benchmarks.-func Benchmark_Clickhouse_Set_And_Delete(b *testing.B) { - b.ReportAllocs() - b.ResetTimer() - - client := newTestStore(b, Config{ +func Benchmark_Clickhouse_Set_And_Delete(b *testing.B) { + client := newTestStore(b, Config{ Engine: Memory, Table: "test_table", Clean: true, }) defer client.Close() + + b.ReportAllocs() + b.ResetTimer()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
aerospike/aerospike_test.go(2 hunks)azureblob/azureblob_test.go(11 hunks)clickhouse/clickhouse_test.go(9 hunks)couchbase/couchbase_test.go(9 hunks)dynamodb/dynamodb_test.go(12 hunks)minio/minio_test.go(17 hunks)mongodb/mongodb_test.go(13 hunks)mysql/mysql_test.go(15 hunks)nats/nats_test.go(14 hunks)neo4j/neo4j_test.go(13 hunks)postgres/postgres_test.go(17 hunks)s3/init_test.go(4 hunks)s3/s3_methods_test.go(3 hunks)s3/s3_test.go(9 hunks)scylladb/scylladb_test.go(13 hunks)
🧰 Additional context used
🧠 Learnings (3)
s3/init_test.go (2)
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: 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.
clickhouse/clickhouse_test.go (2)
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.
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.
couchbase/couchbase_test.go (2)
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.
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.
🧬 Code Graph Analysis (1)
aerospike/aerospike_test.go (1)
aerospike/aerospike.go (1)
Storage(11-18)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: compare (postgres)
- GitHub Check: lint (minio)
- GitHub Check: lint (postgres)
- GitHub Check: lint (dynamodb)
- GitHub Check: lint (mongodb)
- GitHub Check: lint (nats)
- GitHub Check: lint (clickhouse)
- GitHub Check: lint (neo4j)
- GitHub Check: lint (aerospike)
- GitHub Check: lint (mysql)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.24.x)
- GitHub Check: Tests (1.23.x)
- GitHub Check: Tests (1.23.x)
🔇 Additional comments (43)
nats/nats_test.go (4)
78-144: LGTM - Good refactoring to internal error handlingThe function signature has been simplified to return only
*Storageinstead of a tuple with error. All error handling now happens internally usingrequire.NoError(t, err), which will fail the test immediately if any error occurs during test store creation.
153-154: LGTM - Properly updated test initializationTest initialization has been correctly updated to use the new helper function signature, eliminating the need for error variables and explicit error checking after store creation. The deferred close statements remain unchanged which ensures proper resource cleanup.
Also applies to: 171-172, 194-195, 216-217, 240-241, 267-268, 285-286, 308-309, 340-341, 345-346
354-355: LGTM - Benchmark initialization updated correctlyBenchmark initialization has been updated to match the new helper function signature. The benchmark logic remains unchanged.
Also applies to: 369-370, 386-387
360-361: LGTM - Added error declaration before loop to avoid redeclaration issuesMoving the error variable declaration outside the loop is a good practice to avoid variable redeclaration issues in Go, while still allowing the error result to be checked after the loop completes.
Also applies to: 392-393
azureblob/azureblob_test.go (3)
20-46: LGTM - Good refactoring with internal error handlingThe helper function has been updated to return only
*Storageinstead of(*Storage, error), with errors now handled internally viarequire.NoError(t, err). This follows the standardization pattern applied across the codebase.
55-56: LGTM - Test and benchmark initialization updated correctlyAll test and benchmark functions now correctly use the new helper function signature, which simplifies test code by removing error checking after store creation while ensuring that setup failures still fail the test immediately.
Also applies to: 72-73, 85-86, 110-111, 121-122, 137-138, 169-170, 176-177, 181-182, 196-197, 213-214
187-188: LGTM - Proper error variable declaration for benchmarksError variable declarations have been moved before the loop to avoid redeclaration issues in Go, while still allowing for error checking after the loop completes.
Also applies to: 219-220
s3/init_test.go (2)
27-72: LGTM - Good implementation of per-test store initializationThis new helper function replaces global test state with per-test initialization, which improves test isolation. The implementation correctly uses
require.NoErrorfor error assertions during bucket creation. The comment about not needing to delete test buckets is helpful for future maintainers.
9-9: LGTM - Added required importAdded
requireimport which is needed for the assertions in the new helper function.s3/s3_methods_test.go (1)
14-16: LGTM - Properly updated tests to use per-test storesTests now correctly instantiate a fresh store instance per test with proper cleanup via deferred Close calls. This change aligns with the PR objective of running separate containers per test for better isolation.
Also applies to: 27-29, 70-72
dynamodb/dynamodb_test.go (2)
19-48: Good refactoring ofnewTestStoreto use require assertionsThe function now handles errors internally using
require.NoErrorinstead of returning them, which will fail the test immediately upon error. This is a cleaner approach that centralizes error handling in the setup code.
56-57: Clean test function simplificationAll test and benchmark functions have been updated to use the simplified
newTestStoresignature without error handling. This removes boilerplate error checking code from all tests, making them more concise and easier to read while maintaining the same functionality.Also applies to: 69-70, 85-86, 97-98, 111-112, 128-129, 150-151, 155-156, 162-164, 177-180, 194-196
mysql/mysql_test.go (3)
25-39: Improved test store helper with built-in assertionsThe
newTestStorefunction now usesrequire.NoErrorinternally instead of returning errors, centralizing error handling at the setup level. This is a cleaner pattern that will fail tests immediately if setup fails.
65-67: Test closure pattern improvementThe test now correctly creates a new store instance and immediately defers its closure, ensuring proper resource cleanup even if the test fails. This pattern is consistently applied across all tests in the file.
256-258: Better error reporting for Close testThe
Test_MYSQL_Closenow usesrequire.NoErrorfor checking the close operation result instead of the previous implementation. This provides better error messages if the close operation fails.scylladb/scylladb_test.go (3)
21-49: Improved error handling in test setupThe
newTestStorefunction has been refactored to handle errors internally withrequire.NoErrorassertions. This ensures immediate test failure on setup errors instead of returning them to the caller, consistent with the standardization across the codebase.
181-183: Consistent test closure patternThe test now correctly asserts the error from
Close()usingrequire.NoErrorrather than returning it, maintaining the consistency of error handling throughout the test suite.
231-232: Clean variable declaration in benchmarkThe benchmark now uses a separate variable declaration for error handling, avoiding redeclaration issues and maintaining consistent error handling patterns in benchmarks.
s3/s3_test.go (3)
15-17: Improved test isolation with per-test containersEach test now creates its own test store instance and defers its closure, matching the PR's goal of running separate containers per test function instead of a shared singleton. This enhances test isolation and prevents test interference, though it may increase test execution time.
Also applies to: 28-30, 44-46, 56-58, 70-72, 87-89, 114-116
109-111: Better error assertion for store closingThe
Test_S3_Closenow creates a new test store and usesrequire.NoErrorto assert the close operation succeeds, providing better error reporting than the previous implementation.
120-122: Consistent resource management in benchmarksBenchmarks now follow the same pattern as tests - creating a dedicated store instance and deferring closure. This ensures proper resource cleanup even during benchmarking.
Also applies to: 135-137, 152-154
couchbase/couchbase_test.go (3)
24-54: Good refactor to remove error returns and use immediate assertions.The function now correctly uses
require.NoErrorto assert errors immediately, improving test reliability by failing fast on setup errors. This change aligns with the PR's goal of standardizing error handling in test helpers.
57-58: Tests properly updated to use simplified store creation.All test functions have been correctly updated to remove error handling after store creation, making the tests cleaner and more focused on their intended logic.
Also applies to: 65-66, 75-76, 88-89, 103-104, 117-118, 131-132, 136-137, 143-144, 158-159, 175-176
149-150: Appropriate variable declaration adjustments for benchmarks.The benchmark functions now correctly declare the error variable before the loop to match the new error handling pattern.
Also applies to: 181-182
minio/minio_test.go (3)
26-62: Good refactor to remove error returns and use immediate assertions.The function now correctly uses
require.NoErrorto assert errors immediately, improving test reliability by failing fast on setup errors. This change aligns with the PR's goal of standardizing error handling in test helpers.
70-71: Tests properly updated to use simplified store creation.All test functions have been correctly updated to remove error handling after store creation, making the tests cleaner and more focused on their intended logic.
Also applies to: 90-91, 103-104, 116-117, 134-135, 147-148, 162-163, 179-180, 195-196, 208-209, 224-225, 242-243, 247-248, 262-263, 279-280
253-254: Appropriate variable declaration adjustments for benchmarks.The benchmark functions now correctly declare the error variable before the loop to match the new error handling pattern.
Also applies to: 285-286
mongodb/mongodb_test.go (4)
25-54: Good refactor to remove error returns and use immediate assertions.The function now correctly uses
require.NoErrorto assert errors immediately, improving test reliability by failing fast on setup errors. This change aligns with the PR's goal of standardizing error handling in test helpers.
62-63: Tests properly updated to use simplified store creation.All test functions have been correctly updated to remove error handling after store creation, making the tests cleaner and more focused on their intended logic.
Also applies to: 75-76, 91-92, 109-110, 125-126, 134-135, 148-149, 165-166, 192-193, 199-200, 214-215, 231-232
187-188: Improved error assertion in Close test.Changed from
require.Niltorequire.NoError, which is more semantically correct for error checks. This improves code clarity and aligns with Go best practices for error handling.
205-206: Appropriate variable declaration adjustments for benchmarks.The benchmark functions now correctly declare the error variable before the loop to match the new error handling pattern.
Also applies to: 237-238
clickhouse/clickhouse_test.go (3)
30-75: Function renamed and refactored to align with project conventions.The function has been renamed from
getTestConnectiontonewTestStorefor consistency with other modules, and now correctly usesrequire.NoErrorto assert errors immediately. This change aligns with the PR's goals of standardizing naming conventions and error handling in test helpers.
78-83: Tests properly updated to use simplified store creation.All test functions have been correctly updated to use the renamed function and remove error handling after store creation, making the tests cleaner and more focused on their intended logic.
Also applies to: 87-92, 99-104, 111-116, 129-133, 154-159, 175-180, 196-201, 205-210, 224-229, 248-253
212-215: Appropriate benchmark setup adjustments.The benchmark functions now correctly declare the error variable before the loop and properly position the
ReportAllocsandResetTimercalls after store creation.Also applies to: 231-235, 255-256
neo4j/neo4j_test.go (3)
20-49: Improved test isolation through per-test container management.The new
newTestStorefunction properly implements the container-per-test pattern, creating an isolated Neo4j container for each test function and handling cleanup appropriately. The use ofrequire.NoErrorfor immediate test failure on setup errors improves test reliability.
35-36: Correctly using CleanupContainer for automated test cleanup.Using
testcontainers.CleanupContainer(t, neo4jContainer)ensures the container is properly terminated when the test completes, even if it fails. This is a best practice that prevents resource leaks.
57-59: Test refactoring consistently applies the new store-per-test pattern.All test and benchmark functions now correctly create a fresh Storage instance and defer its closure, improving test isolation and preventing state leakage between tests.
Also applies to: 70-72, 86-88, 104-106, 120-122, 129-131, 143-145, 160-162, 184-186, 196-197, 201-203, 207-209, 222-224, 239-241
aerospike/aerospike_test.go (3)
28-30: Improved test helper function signature.Adding
t.Helper()properly marks this as a helper function, which improves test failure reporting by showing the correct location in test code.
54-62: Enhanced container management with proper cleanup.The function now correctly registers container cleanup with
testcontainers.CleanupContainer(t, ctr)and usesrequire.NoErrorto fail tests immediately on setup errors instead of propagating them.
68-76: Simplified error handling in newTestStore.The function now correctly uses
require.NoErrorfor error handling, eliminating error propagation and simplifying calling code.postgres/postgres_test.go (3)
26-53: Streamlined test configuration with proper cleanup.The
newTestConfigfunction now correctly registers container cleanup withtestcontainers.CleanupContainerand usesrequire.NoErrorfor immediate test failure on errors. This simplifies test code by eliminating error handling in test functions.
55-65: Simplified test store creation pattern.The
newTestStoreandnewTestStoreWithConfigfunctions have been refactored to return only the store instance without error handling, making test code cleaner and more readable.
235-240: Consistently applied simplified error handling pattern across all tests.All test and benchmark functions have been updated to use the simplified store creation pattern, removing error checking after store creation and improving test readability.
Also applies to: 248-256, 264-273, 282-293, 298-304, 307-313, 321-333, 338-357, 362-382, 387-396, 407-411, 414-416, 419-431, 434-448, 451-464
|
I'm not sure about the failures:
Is that expected?
Thoughts? |
The S3 module does exactly the same initialisation for the config
|
Interesting: for And I updated the test config for minio to avoid calling reset on startup, as in the S3 module, which was indeed the root cause: because Reset was set to true, the Reset method was called, trying to remove the config bucket, logging the error without capturing it. BTW I'm happy to split this PR in two and fix the minio bug in a separate PR |
There was a problem hiding this comment.
Pull Request Overview
This PR standardizes test setup and error handling across multiple storage backends. It replaces manual error checks with testify’s require assertions in test helper functions and aligns naming conventions for test store functions.
- Simplified test helper functions by removing error return values.
- Each test now creates its own isolated container instance for improved test isolation.
- Updated naming of test store functions for consistency.
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| scylladb/scylladb_test.go | Updated newTestStore to return *Storage; replaced error checks with require assertions. |
| s3/s3_test.go | Simplified test setup by removing error returns from newTestStore. |
| s3/s3_methods_test.go | Updated newTestStore usage for consistency in S3 methods tests. |
| s3/init_test.go | Modified test store initialization; removed bucket deletion since container disposal is sufficient. |
| postgres/postgres_test.go | Revised newTestConfig and newTestStore functions to remove unused error returns. |
| neo4j/neo4j_test.go | Adjusted container termination handling and replaced require.Nil with require.NoError. |
| nats/nats_test.go | Standardized test store initialization across NATS tests. |
| mysql/mysql_test.go | Updated test store functions to use require assertions instead of manual error handling. |
| mongodb/mongodb_test.go | Simplified newTestStore and updated tests to reflect the new signature. |
| minio/minio_test.go | Revised newTestStore and updated test cases to remove error returns. |
| minio/minio.go | Modified Reset() to accumulate deletion errors using errors.Join. |
| dynamodb/dynamodb_test.go | Aligned newTestStore and test cases to use require assertions. |
| couchbase/couchbase_test.go | Standardized test store initialization and error verification. |
| clickhouse/clickhouse_test.go | Updated newTestStore and test cases for consistent test container usage. |
| azureblob/azureblob_test.go | Updated newTestStore functions and tests to remove error returns. |
| aerospike/aerospike_test.go | Revised test container startup to use require assertions; adjusted test store creation. |
Comments suppressed due to low confidence (1)
aerospike/aerospike_test.go:2
- The require package is used in the test code (e.g., require.NoError) but it is not imported. Please add 'github.com/stretchr/testify/require' to the import block to ensure the tests compile.
package aerospike
What does this PR do?
It standarises the creation of the store in the tests using testcontainers-go. Now, the function does not return an error and instead uses testify's require to check errors in the creational function.
Also, I noticed some stores did not name the function with
newTestStore, so I aligned the functions for that, so developers looking for testing patterns are less prone to commit a mistake.Finally, I changed the creation of containers in the neo4j and s3 modules, as they created a singleton container. Now each test function uses its own container. If we see that this is not performant, we can go back to it with ease.
Why is it important?
Consistency across the test suites.
Summary by CodeRabbit