chore: make bridge configurable in tests#1147
Conversation
This commit introduces new test files and utilities for testing the ERC-20 bridge functionality. Key additions include: - `erc20_bridge_simple_test.go`: A minimal integration test to validate the initialization of the ERC-20 bridge and the balance retrieval functionality. - `simple_mock.sql`: A seed script for testing the ERC-20 bridge configuration. - New helper functions in `testutils` for simulating ERC-20 deposit and withdrawal events, enhancing the testing framework for the bridge. - Refactored existing test utilities to support the new ERC-20 bridge testing capabilities. These changes aim to improve the test coverage and reliability of the ERC-20 bridge functionality within the project.
WalkthroughAdds kwiltest-gated ERC‑20 bridge integration tests and helpers, embeds test-only SQL seeds, centralizes tn_cache test helpers (SimpleCache), adds cache/erc20 test utilities and mocks, updates the test runner to initialize cache/erc20 extensions for schema tests, and modifies CI to run Go tests with the Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test (kwiltest)
participant H as ERC20 Helpers
participant P as Platform.Engine
participant OS as Ordered-Sync
participant DB as Test DB
T->>H: ForTestingSeedAndActivateInstance(chain, escrow, token, ...)
H->>DB: Ensure alias, metadata, topics, reward state
H-->>T: Instance ready (synced/enabled)
T->>P: Call erc20.info (EngineCtx)
P-->>T: Info row (chain, escrow, decimals, synced)
T->>H: InjectERC20Transfer(from,to,value,point)
H->>OS: StoreLogs(topic, serializedLogs, point, prev)
H->>OS: Resolve(BlockContext{height=point})
OS-->>H: Resolved
T->>P: Call erc20.balance(user)
P-->>T: Balance rows
sequenceDiagram
autonumber
participant T as Test
participant C as cache.SetupCacheTest
participant E as tn_cache Extension
participant TH as tn_cache TestHelper
participant DB as Test DB
T->>C: SetupCacheTest(ctx, platform, SimpleCache)
C->>E: Parse config and SetupCacheExtension
E-->>C: Extension started (scheduler, sync checker)
C-->>T: CacheTestHelper returned
T->>TH: RefreshAllStreamsSync(ctx)
TH-->>T: Refreshed count
T->>C: Cleanup()
C->>E: Stop scheduler/sync checker, Close extension
C->>DB: Clear test DB injection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (5 passed)✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ 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. Comment |
…resolution This commit introduces significant improvements to the ERC-20 bridge testing framework, including: - Updated `TestERC20BridgeSimpleBalance` to compute the absolute path for the seed script dynamically, ensuring compatibility across environments. - Added `TestERC20BridgeMockListener` to validate the ERC-20 bridge testing infrastructure using a mock listener, enhancing test coverage without requiring the actual `erc20_bridge` extension. - Improved comments to clarify the purpose and expected behavior of the tests, including a note on the current unavailability of the `erc20_bridge` extension in the test environment. These changes aim to strengthen the reliability and clarity of the ERC-20 bridge tests, facilitating easier maintenance and future enhancements.
…files This commit introduces improvements to the migration system by embedding SQL files from a new `test_only` directory, allowing for better organization of test-specific seed scripts. The `GetSeedScriptPaths` function is updated to include paths from this directory, ensuring that all relevant SQL files are accessible during testing. Additionally, a new SQL file, `bootstrap_erc20.sql`, is added to the `test_only` directory, defining necessary tables for testing the ERC-20 bridge functionality. This enhancement aims to streamline the testing process and improve the overall structure of the migration files.
… injected transfers This commit introduces two new test files for the ERC-20 bridge functionality: - `erc20_bridge_epoch_test.go`: Implements `TestERC20BridgeEpochFlow`, validating the lock-and-issue, finalize, and confirm processes using shims. It ensures that rewards are correctly handled during epoch transitions. - `erc20_bridge_injection_test.go`: Implements `TestERC20BridgeInjectedTransferAffectsBalance`, which tests the impact of injected transfers on user balances, ensuring that the system accurately reflects these changes. Additionally, the existing `erc20_bridge_simple_test.go` file is updated to ensure proper instance synchronization and balance checks, enhancing the overall test coverage and reliability of the ERC-20 bridge functionality.
…ner dependencies This commit refactors the ERC-20 bridge test suite by removing the mock listener and simplifying the test setup. Key changes include: - Updated `erc20_bridge_simple_test.go` to utilize transaction-based isolation for better test reliability and consistency. - Introduced `WithERC20TestSetup` and `WithERC20TestSetupTx` helpers to streamline the setup process for ERC-20 bridge tests. - Removed complex listener tests and focused on essential functionality, ensuring that tests are easier to maintain and understand. - Enhanced existing tests to ensure proper instance synchronization and balance checks, improving overall test coverage. These changes aim to enhance the clarity and maintainability of the ERC-20 bridge testing framework.
…onalities This commit introduces multiple new test files for the ERC-20 bridge functionality, enhancing the test coverage and reliability of the system. Key additions include: - `erc20_bridge_actions_smoke_test.go`: A minimal smoke test to verify basic ERC-20 actions and expected values. - `erc20_bridge_admin_authz_test.go`: A test to validate SYSTEM-only method access control, ensuring proper authorization handling. - `erc20_bridge_end_to_end_test.go`: An end-to-end test to confirm the complete flow from deposit to reward listing. - `erc20_bridge_epoch_test.go`: A test for validating the lock-and-issue, finalize, and confirm processes during epoch transitions. - `erc20_bridge_injection_test.go`: A test to ensure injected transfers affect user balances correctly. - `erc20_bridge_multi_instance_test.go`: A test to verify isolation of balances across multiple ERC-20 instances. - `erc20_bridge_transfer_test.go`: A test to validate the transfer functionality between users. These changes aim to improve the clarity, maintainability, and robustness of the ERC-20 bridge testing framework.
…ses and cleanup improvements This commit introduces a new test file, `erc20_bridge_consecutive_test.go`, which ensures that running multiple ERC-20 bridge tests consecutively does not result in "already active" errors due to improper cleanup. Key changes include: - Implementation of `TestERC20BridgeConsecutiveTests` to validate the proper handling of multiple tests in sequence. - Refactoring of existing test functions to utilize the new `ForTestingActivateAndInitialize` helper for better instance management. - Improvements to cleanup mechanisms in the test setup to ensure a clean slate before each test run. These changes aim to improve the reliability and robustness of the ERC-20 bridge testing framework, enhancing overall test coverage and maintainability.
…pt dependencies This commit refactors the ERC-20 bridge test suite to eliminate reliance on seed scripts, enhancing test reliability and maintainability. Key changes include: - Updated test functions to utilize `ForTestingSeedAndActivateInstance` for instance management, ensuring proper setup and teardown. - Removed the `simple_mock.sql` seed script, consolidating instance initialization within the test functions. - Enhanced cleanup mechanisms to ensure a clean state before each test run, preventing "already active" errors. These changes aim to improve the clarity and robustness of the ERC-20 bridge testing framework, facilitating easier maintenance and future enhancements.
…mproved test targeting This commit modifies the CI workflow configuration to include the 'kwiltest' build tag in the Go test command. This change aims to enhance the testing process by allowing for more specific test execution, ensuring that tests relevant to the 'kwiltest' context are run effectively. The adjustment is part of ongoing efforts to improve the reliability and maintainability of the testing framework.
…test clarity This commit modifies the `CallLockAdmin` function in the ERC-20 helper to include an `extensionAlias` parameter, enhancing its flexibility for different bridge instances. Additionally, the `erc20_bridge_simple_test.go` and `erc20_bridge_epoch_test.go` files are updated to reflect this change, ensuring that the correct alias is used during admin lock operations. These adjustments aim to improve the clarity and maintainability of the test suite.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Nitpick comments (63)
tests/streams/utils/service/service.go (4)
11-25: Guard against nil bridgeConfig (and justify returning error).Dereferencing bridgeConfig at Line 14 will panic if callers pass nil. Either drop the error return and keep this panic risk explicit, or keep the error and validate input. Suggest adding a nil check and returning an error.
Apply:
func CreateBridgeService(bridgeConfig *config.ERC20BridgeConfig) (*common.Service, error) { - // Create full config + // Validate input + if bridgeConfig == nil { + return nil, errors.New("bridgeConfig is nil") + } + // Create full config fullConfig := &config.Config{ Erc20Bridge: *bridgeConfig, } // Create service service := &common.Service{ Logger: log.NewStdoutLogger(), LocalConfig: fullConfig, Identity: []byte("test-erc20-node"), } return service, nil }
4-8: Add the required import for the new error path.If you adopt the nil-check above, import errors.
import ( + "errors" "github.com/trufnetwork/kwil-db/common" "github.com/trufnetwork/kwil-db/config" "github.com/trufnetwork/kwil-db/core/log" )
13-16: Avoid cross-test mutations from shared map references.Assigning by value copies the struct, but inner maps/slices/functions remain shared; tests that mutate bridgeConfig can bleed into others. Consider cloning maps (and nested structs) before embedding.
Helper (place in this file):
// cloneBridgeConfig makes a defensive copy of maps inside ERC20BridgeConfig. func cloneBridgeConfig(src *config.ERC20BridgeConfig) config.ERC20BridgeConfig { dst := *src if src.RPC != nil { dst.RPC = make(map[string]string, len(src.RPC)) for k, v := range src.RPC { dst.RPC[k] = v } } if src.Signers != nil { dst.Signers = make(map[string]string, len(src.Signers)) for k, v := range src.Signers { dst.Signers[k] = v } } if src.Chains != nil { dst.Chains = make(map[string]*config.ERC20ChainConfig, len(src.Chains)) for k, v := range src.Chains { if v == nil { dst.Chains[k] = nil; continue } cp := *v dst.Chains[k] = &cp } } if src.MockListeners != nil { dst.MockListeners = make(map[string]listeners.ListenFunc, len(src.MockListeners)) for k, v := range src.MockListeners { dst.MockListeners[k] = v } } return dst }Then:
- fullConfig := &config.Config{ - Erc20Bridge: *bridgeConfig, - } + fullConfig := &config.Config{ + Erc20Bridge: cloneBridgeConfig(bridgeConfig), + }
27-34: Handle nil localConfig defensively.Avoid surprises if callers pass nil.
func CreateServiceWithLocalConfig(localConfig *config.Config) *common.Service { + if localConfig == nil { + localConfig = config.DefaultConfig() + } return &common.Service{ Logger: log.NewStdoutLogger(), LocalConfig: localConfig, Identity: []byte("test-node"), } }.github/workflows/ci.yaml (1)
92-92: Parametrize test tags and allow opt-in race detector via env.Keeps
kwiltestby default while enabling local overrides and-racewithout editing the workflow.- if go test -short -p 1 -timeout=15m -count=1 -tags=kwiltest ./...; then + if go test -short -p 1 -timeout=15m -count=1 ${ENABLE_RACE:+-race} -tags="${TEST_TAGS:-kwiltest}" ./...; thentests/extensions/erc20/README.md (1)
1-63: Add an explicit go test example and a production caveat.
- Include a one-liner so devs can copy/paste.
- Call out that
kwiltestmust never be used in prod builds.### Quick start @@ - Enable the `kwiltest` tag (above) so the test helpers are available. +- Run tests explicitly with the tag: + ``` + go test -tags=kwiltest ./tests/extensions/erc20 -run . -v + ``` @@ ### TL;DR @@ Use the provided `ForTesting*` helpers instead of plain SQL/`USE` to mirror production wiring that SQL alone cannot trigger, ensuring deterministic, isolated ERC‑20 extension tests. + +Note: The `kwiltest` tag is for tests only. Do not build production binaries with this tag enabled.tests/streams/aggregation/aggr05_no_duplicate_child_streams_test.go (1)
29-29: LGTM: migrated to SimpleCache.Optional: prefer matching SQLSTATE (e.g., 23505) or a sentinel error over raw message text for portability across DBs.
-assert.Contains(t, err.Error(), "violates unique constraint") +// Example if errors expose SQLSTATE: +// assert.True(t, errors.Is(err, db.ErrUniqueViolation)) +// or match `23505`: +// assert.Contains(t, err.Error(), "23505")tests/streams/aggregation/aggr08_weight_change_test.go (2)
40-40: SimpleCache switch looks good; confirm wildcard semantics.If
WithStreamtreatsstreamID="*"as a wildcard, this is fine. Otherwise, consider scoping to the specific stream to avoid over-broad cache setup.
154-168: Unify result formatting to avoid per-test transformers.These tests mix integer expected values plus a
ColumnTransformersappender here, and fully-expanded decimals elsewhere. Prefer one style (recommend: explicit 18-decimal strings) and drop the transformer for consistency.Example change for query 1:
- query1expected := ` - | event_time | value | - |------------|-------| - | 1 | 73 | - | 5 | 37 | - | 10 | 74 | - ` - table.AssertResultRowsEqualMarkdownTable(t, table.AssertResultRowsEqualMarkdownTableInput{ - Actual: query1result.Rows, - Expected: query1expected, - ColumnTransformers: map[string]func(string) string{ - "value": addDecimalZeros(18), - }, - }) + query1expected := ` + | event_time | value | + |------------|-------| + | 1 | 73.000000000000000000 | + | 5 | 37.000000000000000000 | + | 10 | 74.000000000000000000 | + ` + table.AssertResultRowsEqualMarkdownTable(t, table.AssertResultRowsEqualMarkdownTableInput{ + Actual: query1result.Rows, + Expected: query1expected, + })Repeat similarly for the day-6 check.
Also applies to: 193-205
tests/streams/aggregation/aggr03_taxonomy_validity_periods_test.go (2)
36-36: SimpleCache migration is fine; validate wildcard stream filter.Confirm
SimpleCache("…0123", "*")is intended and won’t pull unrelated streams; if not, pass the concrete streamID(s).
181-183: Assert cache miss in no-cache mode for stronger signal.When
useCache == false, also assert!result.CacheHitto catch accidental cache usage.- if useCache { - assert.True(t, result.CacheHit, "Expected cache hit") - } + if useCache { + assert.True(t, result.CacheHit, "Expected cache hit") + } else { + assert.False(t, result.CacheHit, "Expected cache miss") + }tests/streams/aggregation/aggr09_duplicate_values_test.go (2)
42-42: SimpleCache switch LGTM; double-check wildcard behavior.Same note as other files: verify
"*"is an intentional selector for this test harness.
112-114: Also assert cache miss when cache is disabled.Mirror the positive assertion with a negative one when
useCacheis false.- if useCache { - assert.True(t, result.CacheHit, "Expected cache hit") - } + if useCache { + assert.True(t, result.CacheHit, "Expected cache hit") + } else { + assert.False(t, result.CacheHit, "Expected cache miss") + }tests/streams/aggregation/aggr04_missing_data_handling_test.go (3)
35-35: SimpleCache rename looks correct; confirm"*"scope.Ensure the wildcard doesn’t introduce cross-test coupling.
107-109: Symmetric assertion for no-cache path.Add an assertion for
!result.CacheHitwhenuseCacheis false.- if useCache { - assert.True(t, result.CacheHit, "Expected cache hit") - } + if useCache { + assert.True(t, result.CacheHit, "Expected cache hit") + } else { + assert.False(t, result.CacheHit, "Expected cache miss") + }
21-31: Clarify wording to match expected behavior.The sentence “doesn't have data … (including last available data)” is ambiguous vs. later comments using last-known values. Consider:
- AGGR04: If a child stream doesn't have data for the given date (including last available data), the composed stream will not count it's weight for that date. + AGGR04: If a child stream has no value at a given date and also has no last-known value, its weight is not counted that day. If a last-known value exists, it is used.tests/extensions/erc20/erc20_bridge_admin_authz_test.go (3)
29-33: Seed/activate idempotency check.Confirm
ForTestingSeedAndActivateInstanceis safe to call across retries/parallel runs and thatTestChainis the intended extension alias.
44-55: Assert the failure mode, not just failure.Also assert the error contains a stable substring (e.g., “system-only” or equivalent) to ensure we’re failing for authz reasons.
- require.Error(t, err, "action lock_admin is system-only") + require.ErrorContains(t, err, "system-only")
61-76: Optionally validate locked accounting.If a helper exists (e.g.,
GetLockedBalance), assert locked increased byTestAmount1to complement the available-balance check.internal/migrations/migration.go (2)
10-11: Avoid embedding test-only SQL into production binariesIncluding
test_only/*.sqlin the main embed set bloats production artifacts and risks unintended seeds being available at runtime. Prefer gating test-only assets behind a build tag or a separate embed in akwiltest-tagged file.Example approach (new file: internal/migrations/migration_kwiltest.go, build-tagged):
+//go:build kwiltest +package migrations + +import "embed" + +//go:embed test_only/*.sql +var seedFilesTestOnly embed.FSThen, in
GetSeedScriptPaths, read fromseedFilesTestOnlyonly when built withkwiltest.
21-32: Runtime path dependency: returning OS paths defeats embed portabilityYou list embedded files via
embed.FSbut return absolute disk paths fromruntime.Caller. This breaks if the source tree isn’t present (e.g., in containers). Consider returning logical names and letting the migration runner read from the embedded FS, or materialize to temp files when needed.Confirm the migration runner expects OS file paths; if not, we can switch it to consume
embed.FS.tests/extensions/tn_cache/cache_height_tracking_test.go (3)
38-39: Prefer one helper surface; avoid mixing package helper and direct tn_cache helperYou set up via
cache.SetupCacheTest(...)but refresh viatn_cache.GetTestHelper(). IfCacheTestHelperexposesRefreshAllStreamsSync, prefer using it for consistency and easier future changes.Also applies to: 65-66
69-71: Remove arbitrary sleep; refresh is already synchronous
RefreshAllStreamsSyncsuggests commit has completed. Sleeping 100ms is brittle on slow CI; poll for a condition or drop the sleep if unnecessary.Proposed change:
- // Wait a moment to ensure the database has processed the cache update - time.Sleep(100 * time.Millisecond) + // No-op: refresh is synchronous. If needed, poll a lightweight readiness check instead.
96-118: Assert on structured fields rather than substring matching logsParsing logs with
strings.Containscan be flaky if formatting changes. If logs are JSON, unmarshal and assert on fieldscache_hit,cache_height,cache_refreshed_at_timestamp.tests/extensions/erc20/erc20_bridge_epoch_test.go (1)
40-51: Avoid narrowing to int for row counts
count(*)may exceedinton 32-bit. Keep asint64and compare accordingly.- var preRows int + var preRows int64 err := platform.Engine.ExecuteWithoutEngineCtx(ctx, platform.DB, preQ, nil, func(row *common.Row) error { if len(row.Values) != 1 { return nil } - preRows = int(row.Values[0].(int64)) + preRows = row.Values[0].(int64) return nil }) ``` <!-- review_comment_end --> </blockquote></details> <details> <summary>tests/streams/complex_composed_test.go (2)</summary><blockquote> `610-613`: **Assertion message contradicts the check.** Message says “should differ” but using `assert.Equal`. Fix wording. ```diff - assert.Equal(t, valueFromLatestOnlyPath, valueFromLatestRangedPath, - "Values for latest event_time (%d) should differ. 'Latest only' path gave '%s', 'ranged' path gave '%s'. This demonstrates the inconsistency.", + assert.Equal(t, valueFromLatestOnlyPath, valueFromLatestRangedPath, + "Values for latest event_time (%d) should match. 'Latest only' path gave '%s', 'ranged' path gave '%s'.", latestEventTime, valueFromLatestOnlyPath, valueFromLatestRangedPath)
647-647: Fix expectation message.You assert
12rows but the message says “Expected 13 rows”.- assert.Equal(t, 12, len(result), "Expected 13 rows") + assert.Equal(t, 12, len(result), "Expected 12 rows")tests/extensions/erc20/erc20_bridge_injection_test.go (1)
22-26: Avoid hardcoding the chain name.Use the shared
TestChainconstant to prevent drift.- chain := "sepolia" + chain := TestChaintests/extensions/erc20/erc20_bridge_actions_smoke_test.go (5)
5-15: Add fmt import for safer parsing below.Subsequent suggestions use fmt for formatting/type-safe conversions.
Apply:
import ( "context" "testing" "github.com/stretchr/testify/require" "github.com/trufnetwork/kwil-db/common" "github.com/trufnetwork/kwil-db/core/types" kwilTesting "github.com/trufnetwork/kwil-db/testing" + "fmt" erc20shim "github.com/trufnetwork/kwil-db/node/exts/erc20-bridge/erc20" )
62-66: Fail fast on unexpected decimals() row shape.Return an error instead of silently ignoring wrong shapes.
Apply:
- if len(row.Values) != 1 { - return nil - } + if len(row.Values) != 1 { + return fmt.Errorf("erc20.decimals: expected 1 column, got %d", len(row.Values)) + }
84-90: Assert non-nil scale_up result before dereference.Avoid nil pointer deref on unexpected handler output.
Apply:
require.NoError(t, err) if r != nil && r.Error != nil { return r.Error } - require.Equal(t, TestAmount1, scaledUpResult.String(), "scale_up(1) should return 1e18") + require.NotNil(t, scaledUpResult, "scale_up should return a decimal") + require.Equal(t, TestAmount1, scaledUpResult.String(), "scale_up(1) should return 1e18")
98-104: Avoid string type assertion for scale_down result.Be tolerant to non-string driver types by formatting.
Apply:
- if len(row.Values) != 1 { - return nil - } - scaledDownResult = row.Values[0].(string) + if len(row.Values) != 1 { + return fmt.Errorf("erc20.scale_down: expected 1 column, got %d", len(row.Values)) + } + scaledDownResult = fmt.Sprintf("%v", row.Values[0]) return nil
22-27: Update step comment to match implementation.Comment mentions ForceSync/Initialize but code uses SeedAndActivate.
Apply:
-// 1) Initialize ERC20 instance via test shims (ForTestingForceSyncInstance + ForTestingInitializeExtension) +// 1) Initialize ERC20 instance via test shims (ForTestingSeedAndActivateInstance)tests/extensions/tn_cache/cache_integration_test.go (1)
322-335: Deduplicate consecutive RefreshAllStreamsSync calls.After the first refresh returns 9 records (parent+children), the two subsequent refresh-all calls are redundant.
Apply:
- // Refresh cache - should cache parent and auto-resolve children - recordsCached, err := tn_cache.GetTestHelper().RefreshAllStreamsSync(ctx) + // Refresh cache - should cache parent and auto-resolve children + recordsCached, err := tn_cache.GetTestHelper().RefreshAllStreamsSync(ctx) require.NoError(t, err) assert.Equal(t, 9, recordsCached, "should cache 9 records (3 parent + 3 child1 + 3 child2)") // Verify child streams were automatically added to cache verifyChildStreamsInCache(t, ctx, platform, deployer.Address(), childComposed1Id.String(), childComposed2Id.String()) - // Refresh cache for child streams to populate their data - _, err = tn_cache.GetTestHelper().RefreshAllStreamsSync(ctx) - require.NoError(t, err, "Failed to refresh child composed 1 cache") - - _, err = tn_cache.GetTestHelper().RefreshAllStreamsSync(ctx) - require.NoError(t, err, "Failed to refresh child composed 2 cache") + // One refresh is sufficient; children are included in the same run.tests/extensions/erc20/erc20_bridge_simple_test.go (2)
30-41: Thread the test context instead of context.Background.Use the seedAndRun ctx for engine calls to support cancellation/timeouts and consistent tracing.
Apply:
-func testSimpleBalanceTx(t *testing.T, txPlatform *kwilTesting.Platform) { +func testSimpleBalanceTx(t *testing.T, ctx context.Context, txPlatform *kwilTesting.Platform) { @@ - balance, err := testerc20.GetUserBalance(context.Background(), txPlatform, TestChain, testWallet) + balance, err := testerc20.GetUserBalance(ctx, txPlatform, TestChain, testWallet)And update the caller:
- testSimpleBalanceTx(t, platform) + testSimpleBalanceTx(t, ctx, platform)
71-96: Also pass ctx in admin-lock test helpers.Mirror the same context threading in testAdminLockTx for consistency.
Apply:
-func testAdminLockTx(t *testing.T, txPlatform *kwilTesting.Platform) { +func testAdminLockTx(t *testing.T, ctx context.Context, txPlatform *kwilTesting.Platform) { @@ - err := testerc20.CreditUserBalance(context.Background(), txPlatform, TestChain, TestEscrowA, testUser, testAmount) + err := testerc20.CreditUserBalance(ctx, txPlatform, TestChain, TestEscrowA, testUser, testAmount) @@ - err = testerc20.CallLockAdmin(context.Background(), txPlatform, TestChain, testUser, testAmount) + err = testerc20.CallLockAdmin(ctx, txPlatform, TestChain, testUser, testAmount) @@ - finalBalance, err := testerc20.GetUserBalance(context.Background(), txPlatform, TestChain, testUser) + finalBalance, err := testerc20.GetUserBalance(ctx, txPlatform, TestChain, testUser)And update the caller:
- testAdminLockTx(t, platform) + testAdminLockTx(t, ctx, platform)internal/migrations/test_only/bootstrap_erc20.sql (1)
15-21: Add an index to speed ordered-sync lookups.Even in tests, resolving large batches benefits from indexing by (topic_id, point).
Apply:
CREATE TABLE IF NOT EXISTS pending_data ( point int8, topic_id UUID REFERENCES topics(id) ON UPDATE CASCADE ON DELETE CASCADE, previous_point int8, data bytea not null, PRIMARY KEY (point, topic_id) ); + +-- Helpful for range scans during resolution +CREATE INDEX IF NOT EXISTS pending_data_topic_point_idx + ON pending_data (topic_id, point);tests/extensions/tn_cache/resolution_transaction_test.go (1)
41-44: Ensure helper methods use the transaction DB.You SetDB(txPlatform.DB) on helper, but call tn_cache.GetTestHelper() directly afterward. If GetTestHelper returns a fresh struct not bound to the helper’s DB, resolution/refresh might run against the non-transaction DB.
Consider invoking the helper’s bound test helper (or add thin wrappers):
-err = tn_cache.GetTestHelper().TriggerResolution(ctx) +err = helper.TriggerResolution(ctx) @@ -_, err = tn_cache.GetTestHelper().RefreshAllStreamsSync(ctx) +_, err = helper.RefreshAllStreamsSync(ctx)If these methods don’t exist, expose them on CacheTestHelper to forward to the internally stored testHelper after SetDB.
Also applies to: 61-63, 87-89
tests/extensions/erc20/erc20_bridge_consecutive_test.go (1)
29-41: Guard type assertions to avoid panics on unexpected schema.Row value layout can drift; add an assertion before casting.
Apply:
- if len(row.Values) >= 7 { - syncedResult = row.Values[6].(bool) - } + if len(row.Values) >= 7 { + if v, ok := row.Values[6].(bool); ok { + syncedResult = v + } + }tests/extensions/erc20/erc20_bridge_end_to_end_test.go (2)
24-30: Align comments with actual helpers used.You’re using ForTestingFinalizeAndConfirmCurrentEpoch; update the flow comments accordingly to avoid confusion for future readers.
Apply:
-// 4) Finalize current epoch and create next (ForTestingFinalizeCurrentEpoch). -// 5) Confirm finalized epochs (ForTestingConfirmAllFinalizedEpochs). +// 4-5) Finalize and confirm the current epoch (ForTestingFinalizeAndConfirmCurrentEpoch).
114-121: “confirmed only” flag vs. argument value looks inverted.The comment says “confirmed only”, but the second argument passed is false. If false means “include unconfirmed,” adjust the comment; if it’s truly “confirmed only,” pass true.
Possible fix (if API expects confirmedOnly):
- r, err = platform.Engine.Call(engineCtx, platform.DB, TestChain, "list_wallet_rewards", []any{TestUserA, false}, func(row *common.Row) error { + r, err = platform.Engine.Call(engineCtx, platform.DB, TestChain, "list_wallet_rewards", []any{TestUserA, true}, func(row *common.Row) error {tests/streams/utils/utils.go (2)
20-26: Avoid panic on duplicate precompile registration; guard with sync.Once and “already registered” checkDuplicate registration across test packages will panic and flake CI. Wrap in sync.Once and tolerate “already registered” errors.
@@ -import ( - "time" +import ( + "strings" + "sync" + "time" @@ -// init registers tn_cache precompiles globally for tests -func init() { - err := precompiles.RegisterInitializer(tn_cache.ExtensionName, tn_cache.InitializeCachePrecompile) - if err != nil { - panic("failed to register tn_cache precompiles: " + err.Error()) - } -} +// init registers tn_cache precompiles globally for tests +var registerCacheOnce sync.Once + +func init() { + registerCacheOnce.Do(func() { + if err := precompiles.RegisterInitializer(tn_cache.ExtensionName, tn_cache.InitializeCachePrecompile); err != nil { + // Allow duplicate registration in multi-package test setups + if !strings.Contains(strings.ToLower(err.Error()), "already") { + panic("failed to register tn_cache precompiles: " + err.Error()) + } + } + }) +}
77-83: Cron “Feb 31” may be rejected; prefer a supported “never” strategyMany cron parsers will error on “0 0 31 2 *”. Consider a no-op flag in CacheOptions (e.g., WithNoRefresh/WithDisabledSchedules) or a safe, very-rare schedule (e.g., “@Yearly”) to avoid init failures.
Would you like me to add a WithNoRefresh() knob to tests/streams/utils/cache/options.go and wire it here?
tests/streams/utils/cache/options.go (3)
136-138: Return a copy to avoid external mutation of internal sliceDefensive copy prevents surprising mutations between builder and consumer code in concurrent tests.
-func (c *CacheOptions) GetStreams() []StreamConfig { - return c.streams -} +func (c *CacheOptions) GetStreams() []StreamConfig { + out := make([]StreamConfig, len(c.streams)) + copy(out, c.streams) + return out +}
52-56: Clarify semantics for negative durationsIf negatives like “-1s” are intended to disable sync checks, document it and optionally normalize to a sentinel (e.g., “disabled”) to make intent explicit for the extension.
I can add normalization in Build() (map “max_block_age” to “disabled” when duration < 0) if tn_cache recognizes it.
100-128: Prefer error return over panic in Build() for marshaling failures (tests still fail fast)Returning (map[string]string, error) makes failures explicit and easier to assert; tests can call t.Fatalf on error.
-func (c *CacheOptions) Build() map[string]string { +func (c *CacheOptions) Build() (map[string]string, error) { metadata := make(map[string]string) @@ - streamsJSON, err := json.Marshal(c.streams) - if err != nil { - panic(fmt.Sprintf("failed to marshal cache streams: %v", err)) - } + streamsJSON, err := json.Marshal(c.streams) + if err != nil { + return nil, fmt.Errorf("failed to marshal cache streams: %w", err) + } metadata["streams_inline"] = string(streamsJSON) } } - return metadata + return metadata, nil }Note: Call sites (e.g., helper.Setup) need a trivial adaptation.
tests/streams/utils/erc20/helper.go (2)
44-51: Engine context height may lag injected block; consider matching the injection heightInjected logs use point=10. Using Height:1 could cause flakiness if height-gated reads exist. Prefer using the injection height (or higher).
- txCtx := &common.TxContext{ + txCtx := &common.TxContext{ Ctx: ctx, - BlockContext: &common.BlockContext{Height: 1}, + BlockContext: &common.BlockContext{Height: 10},Or pass the height as a parameter from the caller.
37-40: Hard-coded block point risks collisions across multiple injectionsUsing a constant point (10) can conflict in multi-step tests. Make it monotonic or accept a point argument.
-func CreditUserBalance(ctx context.Context, platform *kwilTesting.Platform, extensionAlias, escrowAddr, userAddr, amount string) error { +func CreditUserBalance(ctx context.Context, platform *kwilTesting.Platform, extensionAlias, escrowAddr, userAddr, amount string) error { @@ - ctx, platform, extensionAlias, escrowAddr, "0x2222222222222222222222222222222222222222", userAddr, escrowAddr, amount, 10, nil) + // Use a unique, increasing point to avoid collisions (example: time-based) + ctx, platform, extensionAlias, escrowAddr, "0x2222222222222222222222222222222222222222", userAddr, escrowAddr, amount, time.Now().Unix(), nil)(Note: add time import.)
tests/extensions/erc20/common_test.go (2)
23-23: Duplicate address for TestEscrowB and TestERC20 — likely unintendedTestEscrowB and TestERC20 both use 0x2222... Use distinct addresses to avoid false positives and cross-entity confusion.
Apply:
- TestERC20 = "0x2222222222222222222222222222222222222222" + TestERC20 = "0x3333333333333333333333333333333333333333"
80-85: Avoid duplicate TestingT definitions across packagesThis TestingT (with Cleanup) differs from testutils.TestingT (no Cleanup). Consider centralizing under tests/streams/utils to prevent drift.
tests/streams/utils/erc20/mocks.go (3)
13-27: Prefer json.Marshal over hand-built JSONManual fmt.Sprintf risks escaping bugs; json.Marshal is safer and clearer.
Apply:
@@ -import ( - "context" - "fmt" - - "github.com/trufnetwork/kwil-db/common" - "github.com/trufnetwork/kwil-db/extensions/listeners" -) +import ( + "context" + "encoding/json" + "fmt" + + "github.com/trufnetwork/kwil-db/common" + "github.com/trufnetwork/kwil-db/extensions/listeners" +) @@ func CreateMockERC20Transfer(from, to, amount string) []byte { - // This would create a properly formatted Ethereum log event - // For now, return a simple JSON representation - return []byte(fmt.Sprintf(`{"from":"%s","to":"%s","amount":"%s"}`, from, to, amount)) + payload, _ := json.Marshal(map[string]string{ + "from": from, + "to": to, + "amount": amount, + }) + return payload } @@ func CreateMockERC20Deposit(user, token, amount string) []byte { - return []byte(fmt.Sprintf(`{"user":"%s","token":"%s","amount":"%s","type":"deposit"}`, user, token, amount)) + payload, _ := json.Marshal(map[string]string{ + "user": user, + "token": token, + "amount": amount, + "type": "deposit", + }) + return payload } @@ func CreateMockERC20Withdrawal(user, token, amount string) []byte { - return []byte(fmt.Sprintf(`{"user":"%s","token":"%s","amount":"%s","type":"withdrawal"}`, user, token, amount)) + payload, _ := json.Marshal(map[string]string{ + "user": user, + "token": token, + "amount": amount, + "type": "withdrawal", + }) + return payload }
59-61: Avoid magic strings for event typesDefine constants for "erc20_deposit"/"erc20_withdrawal" and reuse to reduce typos.
Apply:
@@ package erc20 +const ( + EventDeposit = "erc20_deposit" + EventWithdrawal = "erc20_withdrawal" +) @@ - if err := eventstore.Broadcast(ctx, "erc20_deposit", mockData); err != nil { + if err := eventstore.Broadcast(ctx, EventDeposit, mockData); err != nil { @@ - if err := eventstore.Broadcast(ctx, "erc20_withdrawal", mockData); err != nil { + if err := eventstore.Broadcast(ctx, EventWithdrawal, mockData); err != nil {Also applies to: 78-80
45-47: Listeners block until context cancellation — ensure caller cancelsThese functions wait on <-ctx.Done(). If the test harness doesn’t cancel the context, tests can hang.
Option: add a “run-once” variant that returns after broadcasting.
Also applies to: 65-66, 84-85
tests/streams/utils/runner.go (4)
54-57: Use explicit disable value for MaxBlockAgeSyncChecker treats 0 as disabled; negative durations may parse inconsistently. Prefer 0 for clarity.
Apply:
- WithMaxBlockAge(-1 * time.Second). // Disable sync check + WithMaxBlockAge(0). // Disable sync check
87-93: erc20Config parameter is unusedIf ERC-20 setup is handled in tests, drop the parameter to avoid confusion or add a TODO explaining why it remains.
-func wrapWithExtensionsSetup(ctx context.Context, originalFuncs []kwilTesting.TestFunc, cacheConfig *cache.CacheOptions, erc20Config *erc20.ERC20BridgeConfig, opts *kwilTesting.Options) []kwilTesting.TestFunc { +func wrapWithExtensionsSetup(ctx context.Context, originalFuncs []kwilTesting.TestFunc, cacheConfig *cache.CacheOptions, _ *erc20.ERC20BridgeConfig, opts *kwilTesting.Options) []kwilTesting.TestFunc { @@ - if cacheConfig != nil || erc20Config != nil { + if cacheConfig != nil { wrappedTests = wrapWithExtensionsSetup(context.Background(), s.FunctionTests, cacheConfig, erc20Config, kwilOpts) }Or remove ERC-20 from Options entirely in a follow-up.
127-165: Duplicate cache setup logic — prefer single helperThis reimplements tests/streams/utils/cache/helper.go’s setup/teardown. Consolidate to one implementation returning a cleanup to avoid drift.
I can refactor helper.go to return (cleanup func, err) and update this call-site if you want.
33-35: *Type assertion to testing.T is brittleIf you always require *testing.T, accept it directly and drop the interface here. Otherwise, guard the assertion and return a descriptive error.
-func RunSchemaTest(t TestingT, s kwilTesting.SchemaTest, options *Options) { - // Convert to kwilTesting.Options - use type assertion since TestingT is an alias for testing.T - testT := t.(*testing.T) +func RunSchemaTest(testT *testing.T, s kwilTesting.SchemaTest, options *Options) {Also applies to: 171-174
tests/streams/utils/cache/helper.go (5)
26-27: Unused ctx parameterSetupCacheTest receives ctx but doesn’t use it. Either use it (e.g., for future DB/setup calls) or remove it.
40-45: Avoid redundant string conversions (if Build() already returns map[string]string)If CacheOptions.Build() returns map[string]string, you can pass it directly.
For example:
- configMap := make(map[string]string) - for k, v := range cacheConfig.Build() { - configMap[k] = fmt.Sprintf("%v", v) - } - tn_cache.SetTestConfiguration(configMap) + tn_cache.SetTestConfiguration(cacheConfig.Build())
18-23: Remove or use the unused testHelper fieldtestHelper is assigned but never read. Drop the field (and assignment) or expose it via an accessor if it’s needed by callers.
type CacheTestHelper struct { cacheConfig *CacheOptions platform *kwilTesting.Platform - testHelper *tn_cache.TestHelper } @@ - helper.testHelper = tn_cache.GetTestHelper()Also applies to: 50-51
144-151: ParseConfig is using the global test override, not LocalConfigBecause SetupCacheTest() set a test override via SetTestConfiguration, tn_cache.ParseConfig() will ignore LocalConfig by design. If you intend to test the LocalConfig path here, clear the override before parsing.
// Parse and setup extension + // Ensure ParseConfig uses LocalConfig rather than test override + tn_cache.SetTestConfiguration(nil) processedConfig, err := tn_cache.ParseConfig(mockService)
95-113: Thread a context through transactionsWithTx uses context.Background(). Consider accepting a ctx so callers can bound/ cancel DB work.
-func WithTx(platform *kwilTesting.Platform, testFunc func(TestingT, *kwilTesting.Platform)) func(TestingT) { - return func(t TestingT) { - tx, err := platform.DB.BeginTx(context.Background()) +func WithTxCtx(ctx context.Context, platform *kwilTesting.Platform, testFunc func(TestingT, *kwilTesting.Platform)) func(TestingT) { + return func(t TestingT) { + tx, err := platform.DB.BeginTx(ctx) if err != nil { t.Fatalf("begin tx: %v", err) return } - defer tx.Rollback(context.Background()) + defer tx.Rollback(ctx)Then have RunInTx use WithTxCtx(ctx, ...).
tests/streams/utils/erc20/config.go (1)
80-96: Unmapped fields (Timeout, AutoStart, MockListeners)BuildERC20BridgeConfig doesn’t propagate Timeout/AutoStart/MockListeners. If config.ERC20BridgeConfig supports them, map them; otherwise add a brief comment clarifying they are test-only knobs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
.github/workflows/ci.yaml(1 hunks)internal/migrations/migration.go(2 hunks)internal/migrations/test_only/bootstrap_erc20.sql(1 hunks)tests/extensions/erc20/README.md(1 hunks)tests/extensions/erc20/common_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_actions_smoke_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_admin_authz_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_consecutive_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_end_to_end_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_epoch_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_injection_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_multi_instance_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_simple_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_transfer_test.go(1 hunks)tests/extensions/tn_cache/cache_height_tracking_test.go(5 hunks)tests/extensions/tn_cache/cache_integration_test.go(6 hunks)tests/extensions/tn_cache/cache_observability_test.go(3 hunks)tests/extensions/tn_cache/resolution_transaction_test.go(4 hunks)tests/streams/aggregation/aggr01_basic_aggregation_test.go(1 hunks)tests/streams/aggregation/aggr02_weighted_contributions_test.go(1 hunks)tests/streams/aggregation/aggr03_taxonomy_validity_periods_test.go(1 hunks)tests/streams/aggregation/aggr04_missing_data_handling_test.go(2 hunks)tests/streams/aggregation/aggr05_no_duplicate_child_streams_test.go(1 hunks)tests/streams/aggregation/aggr06_single_active_taxonomy_test.go(1 hunks)tests/streams/aggregation/aggr07_inexistent_streams_test.go(1 hunks)tests/streams/aggregation/aggr08_weight_change_test.go(1 hunks)tests/streams/aggregation/aggr09_duplicate_values_test.go(1 hunks)tests/streams/complex_composed_test.go(5 hunks)tests/streams/utils/cache/helper.go(1 hunks)tests/streams/utils/cache/options.go(1 hunks)tests/streams/utils/erc20/config.go(1 hunks)tests/streams/utils/erc20/helper.go(1 hunks)tests/streams/utils/erc20/inject.go(1 hunks)tests/streams/utils/erc20/mocks.go(1 hunks)tests/streams/utils/eventstore/mock.go(1 hunks)tests/streams/utils/runner.go(1 hunks)tests/streams/utils/service/service.go(1 hunks)tests/streams/utils/utils.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (30)
tests/streams/aggregation/aggr01_basic_aggregation_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/extensions/erc20/erc20_bridge_end_to_end_test.go (3)
tests/extensions/erc20/common_test.go (5)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)TestUserA(24-24)TestAmount1(26-26)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(43-68)
tests/streams/utils/service/service.go (1)
tests/streams/utils/erc20/config.go (1)
ERC20BridgeConfig(16-23)
tests/streams/aggregation/aggr08_weight_change_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (1)
tests/extensions/erc20/common_test.go (4)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)TestAmount1(26-26)
tests/extensions/erc20/erc20_bridge_transfer_test.go (3)
tests/extensions/erc20/common_test.go (7)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)TestUserA(24-24)TestAmount2(27-27)TestUserB(25-25)TestAmount1(26-26)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(43-68)
tests/extensions/erc20/erc20_bridge_epoch_test.go (2)
tests/extensions/erc20/common_test.go (1)
TestChain(20-20)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)
tests/streams/aggregation/aggr06_single_active_taxonomy_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/extensions/erc20/erc20_bridge_multi_instance_test.go (2)
tests/extensions/erc20/common_test.go (4)
TestChain(20-20)TestERC20(23-23)TestUserA(24-24)TestAmount1(26-26)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)
tests/streams/aggregation/aggr03_taxonomy_validity_periods_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/extensions/erc20/common_test.go (3)
tests/streams/utils/runner.go (3)
TestingT(171-174)RunSchemaTest(32-83)Options(22-28)internal/migrations/migration.go (1)
GetSeedScriptPaths(13-50)tests/streams/utils/utils.go (1)
GetTestOptions(56-60)
tests/streams/utils/cache/options.go (1)
tests/streams/utils/utils.go (3)
StreamConfig(34-34)NewCacheOptions(44-44)CacheOptions(33-33)
tests/streams/aggregation/aggr05_no_duplicate_child_streams_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/extensions/erc20/erc20_bridge_admin_authz_test.go (3)
tests/extensions/erc20/common_test.go (6)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)TestUserA(24-24)TestAmount2(27-27)TestAmount1(26-26)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(43-68)
tests/streams/utils/erc20/helper.go (1)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)
tests/streams/aggregation/aggr04_missing_data_handling_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/extensions/erc20/erc20_bridge_injection_test.go (2)
tests/extensions/erc20/common_test.go (1)
TestChain(20-20)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(22-77)
tests/extensions/tn_cache/cache_height_tracking_test.go (4)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/runner.go (1)
RunSchemaTest(32-83)tests/streams/utils/cache/helper.go (1)
SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
tests/streams/aggregation/aggr09_duplicate_values_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/streams/utils/runner.go (7)
tests/streams/utils/utils.go (2)
CacheOptions(33-33)NewCacheOptions(44-44)tests/streams/utils/cache/options.go (2)
CacheOptions(11-16)NewCacheOptions(28-32)tests/extensions/erc20/common_test.go (1)
TestingT(81-85)tests/streams/utils/cache/helper.go (3)
TestingT(116-119)SetupCacheTest(26-52)SetupCacheExtension(122-172)tests/streams/utils/service/service.go (1)
CreateDefaultService(37-43)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)extensions/tn_cache/syncschecker/sync_checker.go (1)
SyncChecker(24-37)
tests/extensions/tn_cache/cache_integration_test.go (3)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/cache/helper.go (1)
SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
tests/streams/utils/cache/helper.go (4)
tests/streams/utils/cache/options.go (1)
CacheOptions(11-16)extensions/tn_cache/test_init.go (3)
SetTestDBConfiguration(24-26)SetTestDB(30-32)SetTestConfiguration(19-21)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)
tests/extensions/tn_cache/cache_observability_test.go (3)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/cache/helper.go (1)
SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
tests/extensions/erc20/erc20_bridge_simple_test.go (2)
tests/extensions/erc20/common_test.go (3)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)tests/streams/utils/erc20/helper.go (3)
GetUserBalance(43-68)CreditUserBalance(36-40)CallLockAdmin(72-100)
tests/extensions/tn_cache/resolution_transaction_test.go (3)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/cache/helper.go (1)
SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
tests/streams/complex_composed_test.go (4)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/runner.go (1)
TestingT(171-174)tests/streams/utils/cache/helper.go (2)
TestingT(116-119)SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
tests/streams/aggregation/aggr07_inexistent_streams_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/streams/aggregation/aggr02_weighted_contributions_test.go (1)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)
tests/streams/utils/utils.go (3)
tests/streams/utils/cache/options.go (3)
CacheOptions(11-16)StreamConfig(19-25)NewCacheOptions(28-32)tests/streams/utils/cache/helper.go (1)
CacheTestHelper(19-23)tests/streams/utils/runner.go (1)
Options(22-28)
tests/extensions/erc20/erc20_bridge_consecutive_test.go (1)
tests/extensions/erc20/common_test.go (3)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)
🪛 GitHub Check: lint
tests/streams/utils/erc20/inject.go
[failure] 72-72:
undefined: orderedsync.ForTestingResolve (typecheck)
[failure] 67-67:
undefined: orderedsync.ForTestingStoreLogs
[failure] 47-47:
undefined: types.BigIntToHash32
[failure] 30-30:
undefined: erc20bridge.ForTestingTransferListenerTopic
[failure] 24-24:
undefined: erc20bridge.ForTestingForceSyncInstance
tests/streams/utils/erc20/helper.go
[failure] 30-30:
undefined: erc20shim.ForTestingSeedAndActivateInstance
[failure] 27-27:
undefined: erc20shim.ForTestingResetSingleton
[failure] 26-26:
undefined: erc20shim.ForTestingDisableInstance
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (24)
tests/streams/utils/service/service.go (3)
36-43: LGTM for the default helper.DefaultConfig with a basic identity is appropriate for tests.
19-21: Replace Stdout logger with a no-op or injectable loggerStdout logging in test helpers floods CI. Use a no-op logger if available (e.g., log.NewNopLogger()), or refactor the helper to accept a log.Logger parameter and default to Stdout only when nil. No matches for NewNopLogger/NopLogger were found in the repo — verify availability or add a Nop logger.
Locations: tests/streams/utils/service/service.go — lines 19-21, 30-31, 39-41
13-16: Confirm config.Config.Erc20Bridge field type matches config.ERC20BridgeConfig.Found a test-local ERC20BridgeConfig at tests/streams/utils/erc20/config.go and the assignment in tests/streams/utils/service/service.go (Erc20Bridge: *bridgeConfig). No repo-wide definition of config.Config or config.ERC20BridgeConfig was located in the provided results — confirm that the imported config.Config has an Erc20Bridge field and that its type is config.ERC20BridgeConfig (not tests/streams/utils/erc20.ERC20BridgeConfig).
tests/streams/utils/eventstore/mock.go (1)
99-100: Alias looks good.Exported alias enables type assertions in tests without leaking internals. No change needed.
tests/streams/aggregation/aggr06_single_active_taxonomy_test.go (1)
32-32: LGTM: migrated to SimpleCache.Consistent with the suite-wide cache helper change.
tests/streams/aggregation/aggr02_weighted_contributions_test.go (1)
33-33: LGTM: migrated to SimpleCache.tests/streams/aggregation/aggr07_inexistent_streams_test.go (1)
28-28: LGTM: migrated to SimpleCache.tests/streams/aggregation/aggr01_basic_aggregation_test.go (1)
33-33: LGTM: migrated to SimpleCache.tests/extensions/erc20/erc20_bridge_admin_authz_test.go (2)
1-1: Kwiltest build tag is appropriate.Scoped to kwiltest runs—good isolation.
29-31: Verified — helpers present and kwiltest-gated
seedAndRun and engCtx are defined in tests/extensions/erc20/common_test.go and the file begins with//go:build kwiltest(seedAndRun ~line 31, engCtx ~line 67).tests/extensions/tn_cache/cache_height_tracking_test.go (1)
31-32: LGTM on switching to centralized SimpleCache helperUsing
testutils.SimpleCacheimproves consistency across cache tests.tests/extensions/erc20/erc20_bridge_multi_instance_test.go (1)
35-40: Good: two isolated instances with distinct escrows and aliasesClear separation via
escrowA/BandaliasA/Bmakes the isolation check robust.tests/streams/complex_composed_test.go (1)
116-123: Possible double-initialization of tn_cache in test harness.You pass
GetTestOptionsWithCache(cacheConfig)toRunSchemaTestand also callcache.SetupCacheTesthere. Verify there are no side effects (e.g., reconfiguring the same DB/extension twice).Would you like me to scan the repo to see where
GetTestOptionsWithCacheinitializes the cache and whether it already handles cleanup?tests/extensions/tn_cache/cache_observability_test.go (1)
30-31: Cache test setup pattern LGTM; confirm no redundant setup.Switch to
SimpleCacheandtn_cache.GetTestHelper().RefreshAllStreamsSynclooks correct. SinceRunSchemaTest(..., GetTestOptionsWithCache(cacheConfig))likely initializes cache already, validate that callingcache.SetupCacheTestagain (only to gainCleanup) doesn’t reconfigure state unnecessarily. If it does, consider exposing a lightweight helper to fetch the test helper or aNewCleanupOnlyutility.Also applies to: 38-41, 103-106
tests/extensions/erc20/erc20_bridge_transfer_test.go (1)
27-72: Solid minimal E2E transfer test.Good use of seed/activate + injected transfer + balance assertions; flow is clear and deterministic.
tests/extensions/tn_cache/cache_integration_test.go (2)
34-35: Using SimpleCache config is appropriate here.Good choice to disable auto-refresh/resolve deterministically for tests.
49-51: Double test-setup may reconfigure the extension redundantly. Verify idempotency.RunSchemaTest + GetTestOptionsWithCache already configures the cache; SetupCacheTest does it again. Confirm tn_cache.SetTestConfiguration/SetTestDB are idempotent in this path to avoid flaky state.
If needed, I can help replace SetupCacheTest with a lightweight “attach helper only” utility that skips reconfiguration.
Also applies to: 247-249
tests/extensions/erc20/erc20_bridge_simple_test.go (1)
21-22: Good activation flow.Enabling and activating the instance inline with ForTestingSeedAndActivateInstance keeps tests hermetic and fast.
tests/extensions/tn_cache/resolution_transaction_test.go (1)
24-25: Nice switch to SimpleCache.The never-run schedules and disabled sync checks make the test deterministic.
tests/extensions/erc20/erc20_bridge_consecutive_test.go (1)
48-66: Failure path captures diagnostics then still fails fast — OK.The DB probe is helpful; retaining require.NoError ensures we don’t accidentally pass on “already active” regressions.
tests/streams/utils/erc20/helper.go (1)
12-16: Do not change the import — current path matches existing testsinject.go and helper.go both import "github.com/trufnetwork/kwil-db/node/exts/erc20-bridge/erc20" (aliases erc20bridge / erc20shim) and call the ForTesting* helpers across the test suite; switching to "github.com/trufnetwork/node/extensions/erc20bridge" is incorrect.
Likely an incorrect or invalid review comment.
tests/extensions/erc20/common_test.go (1)
42-47: Intentional no-commit? Verify tests don’t require persisted stateThe transaction is always rolled back via defer; if any test assumes committed state across function boundaries, this will mask issues or cause flakes.
Suggest adding a short comment clarifying rollback-only isolation policy.
tests/streams/utils/cache/helper.go (1)
75-80: Global overrides are not parallel-safeSetTestDB / SetTestConfiguration mutate package-level state. Document that tests using WithCache must not call t.Parallel(), or add a guard to serialize such tests.
tests/streams/utils/erc20/config.go (1)
82-86: Field-name verification: BlockSyncChuckSize vs BlockSyncChunkSize; Signer vs SignersThese identifiers look misspelled/mismatched. Confirm config.ERC20BridgeConfig’s field names. If upstream uses BlockSyncChunkSize and Signers, update accordingly.
erc20Config := &config.ERC20BridgeConfig{ RPC: c.RPC, - BlockSyncChuckSize: make(map[string]string), - Signer: c.Signers, + BlockSyncChunkSize: make(map[string]string), + Signers: c.Signers, } @@ - if chainConfig.BlockSyncChunkSize != "" { - erc20Config.BlockSyncChuckSize[chain] = chainConfig.BlockSyncChunkSize + if chainConfig.BlockSyncChunkSize != "" { + erc20Config.BlockSyncChunkSize[chain] = chainConfig.BlockSyncChunkSize }If upstream indeed defines the “Chuck”/“Signer” fields, disregard this change.
Also applies to: 89-93
Time Submission Status
|
…d go.sum This commit updates the versions of `kwil-db` and `kwil-db/core` in the `go.mod` and `go.sum` files to the latest revisions. Additionally, it ensures that the `go-ethereum` dependency is correctly listed without the indirect comment. These changes aim to keep the project dependencies up to date and maintain compatibility with the latest features and fixes.
This commit adds a comment to the `bootstrap_erc20.sql` file, clarifying its purpose in manually enabling the ERC20 bootstrap process for testing. This enhancement aims to improve the understanding of the file's role within the test harness.
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 (1)
tests/streams/complex_composed_test.go (1)
610-613: Assertion message contradicts the check (consistency vs. difference).The code asserts equality but the message says “should differ”. Pick one; for a “consistency” test, make the message say “should match.”
- assert.Equal(t, valueFromLatestOnlyPath, valueFromLatestRangedPath, - "Values for latest event_time (%d) should differ. 'Latest only' path gave '%s', 'ranged' path gave '%s'. This demonstrates the inconsistency.", + assert.Equal(t, valueFromLatestOnlyPath, valueFromLatestRangedPath, + "Values for latest event_time (%d) should match. 'Latest only' path gave '%s', 'ranged' path gave '%s'.", latestEventTime, valueFromLatestOnlyPath, valueFromLatestRangedPath)
♻️ Duplicate comments (6)
tests/extensions/erc20/erc20_bridge_transfer_test.go (1)
54-60: Resolved: now checking VM execution error (r.Error).Nice fix capturing r and handling r.Error. To align with the test suite’s fail-fast preference (panic on schema surprises), assert inline instead of returning the error.
- r, err := platform.Engine.Call(engineCtx, platform.DB, TestChain, "transfer", []any{TestUserB, halfDec}, func(row *common.Row) error { + r, err := platform.Engine.Call(engineCtx, platform.DB, TestChain, "transfer", []any{TestUserB, halfDec}, func(row *common.Row) error { return nil }) require.NoError(t, err) - if r != nil && r.Error != nil { - return r.Error - } + if r != nil { + require.NoError(t, r.Error) + }tests/extensions/erc20/erc20_bridge_multi_instance_test.go (1)
87-91: Nice: guaranteed cleanup via t.Cleanup.Ensures isolation even on early failures.
tests/streams/complex_composed_test.go (1)
55-60: *Removed unsafe t.(testing.T) assertion in RunInTx — good.Passing the outer *testing.T eliminates a possible panic.
Also applies to: 63-68
tests/streams/utils/runner.go (2)
94-101: Also recover from panics to report them as testErrCleanups now run via defer (nice), but a panic in the test leaves testErr nil. Recover and convert to an error so the harness reports failures consistently.
- defer func() { + defer func() { + if r := recover(); r != nil { + testErr = fmt.Errorf("test panicked: %v", r) + } for i := len(cleanups) - 1; i >= 0; i-- { if cleanups[i] != nil { cleanups[i]() } } }()
94-101: Deferred cleanup addition looks goodYou added a single deferred block to run cleanups in reverse order, which addresses the earlier review. Nice.
tests/streams/utils/erc20/config.go (1)
129-132: Cross-platform dev-null check: LGTMUsing os.DevNull instead of "/dev/null" is correct and portable.
🧹 Nitpick comments (6)
tests/extensions/erc20/erc20_bridge_transfer_test.go (1)
30-31: Avoid magic numbers for decimals and reorg window.Name these to prevent drift and make intent obvious (especially if other tests/constants change).
- err := erc20shim.ForTestingSeedAndActivateInstance(ctx, platform, TestChain, TestEscrowA, TestERC20, 18, 60, TestChain) + const decimals = 18 + const reorgWindow = 60 + err := erc20shim.ForTestingSeedAndActivateInstance(ctx, platform, TestChain, TestEscrowA, TestERC20, decimals, reorgWindow, TestChain)Alternatively, define shared test constants in tests/extensions/erc20/common_test.go (preferred) to keep everything in one place:
// in tests/extensions/erc20/common_test.go const TestDecimals = 18 const TestReorgWindow = 60Then:
- err := erc20shim.ForTestingSeedAndActivateInstance(ctx, platform, TestChain, TestEscrowA, TestERC20, 18, 60, TestChain) + err := erc20shim.ForTestingSeedAndActivateInstance(ctx, platform, TestChain, TestEscrowA, TestERC20, TestDecimals, TestReorgWindow, TestChain)tests/extensions/erc20/erc20_bridge_multi_instance_test.go (1)
49-67: Balance read: style deviates from preferred fail-fast.Given your fail-fast preference, consider direct type assertions instead of ok-guards to panic on schema drift. Current approach is fine if you intentionally softened it here.
tests/streams/complex_composed_test.go (1)
646-648: Fix misleading assertion message (row count).Message says 13 but asserting 12.
- assert.Equal(t, 12, len(result), "Expected 13 rows") + assert.Equal(t, 12, len(result), "Expected 12 rows")tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (1)
21-27: Update test-flow comment to match implementation.You’re using SeedAndActivate, not ForceSync+Initialize; keep docs accurate.
-// 1) Initialize ERC20 instance via test shims (ForTestingForceSyncInstance + ForTestingInitializeExtension) +// 1) Initialize ERC20 instance via test shims (ForTestingSeedAndActivateInstance)tests/streams/utils/runner.go (2)
30-33: Fix doc to reflect current behavior (ERC-20 setup is handled in tests)The wrapper no longer initializes ERC-20; update the comment to avoid confusion.
-// RunSchemaTest is a wrapper around kwilTesting.RunSchemaTest that automatically -// handles both cache and ERC-20 bridge setup. +// RunSchemaTest wraps kwilTesting.RunSchemaTest and initializes the cache extension when requested. +// ERC-20 bridge setup is handled directly in tests via helpers.
33-40: *Change RunSchemaTest to accept testing.T (preferred) or add a defensive guard)Verified callers: repository search shows all current call sites pass the test's *testing.T; one caller already casts explicitly (tests/extensions/erc20/common_test.go:59). Prefer changing the signature to avoid a runtime panic; if you must keep the interface, add a clear guard that fails the test.
Preferred (minimal):
-func RunSchemaTest(t TestingT, s kwilTesting.SchemaTest, options *Options) { - // Convert to kwilTesting.Options - use type assertion since TestingT is an alias for testing.T - testT := t.(*testing.T) +func RunSchemaTest(testT *testing.T, s kwilTesting.SchemaTest, options *Options) {Alternative (keep interface, defensive):
- testT := t.(*testing.T) + testT, ok := any(t).(*testing.T) + if !ok { + t.Fatalf("RunSchemaTest expects *testing.T; got %T", t) + return + }tests/streams/utils/runner.go:32-40; see tests/extensions/erc20/common_test.go:59 for an existing cast.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
go.mod(2 hunks)tests/extensions/erc20/erc20_bridge_actions_smoke_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_injection_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_multi_instance_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_transfer_test.go(1 hunks)tests/streams/complex_composed_test.go(5 hunks)tests/streams/utils/cache/helper.go(1 hunks)tests/streams/utils/erc20/config.go(1 hunks)tests/streams/utils/erc20/helper.go(1 hunks)tests/streams/utils/erc20/inject.go(1 hunks)tests/streams/utils/eventstore/mock.go(1 hunks)tests/streams/utils/runner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/streams/utils/eventstore/mock.go
- tests/streams/utils/erc20/inject.go
- tests/extensions/erc20/erc20_bridge_injection_test.go
- tests/streams/utils/cache/helper.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T01:19:04.515Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47
Timestamp: 2025-09-12T01:19:04.515Z
Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
Applied to files:
tests/extensions/erc20/erc20_bridge_multi_instance_test.gotests/extensions/erc20/erc20_bridge_actions_smoke_test.gotests/extensions/erc20/erc20_bridge_transfer_test.gotests/streams/utils/erc20/helper.go
📚 Learning: 2025-09-12T01:16:55.153Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_epoch_test.go:31-32
Timestamp: 2025-09-12T01:16:55.153Z
Learning: In the ERC20 bridge tests, the test environment is reset after each test via seedAndRun function and cleanup mechanisms, so using shared aliases like TestChain is acceptable and doesn't cause collisions between tests.
Applied to files:
tests/extensions/erc20/erc20_bridge_multi_instance_test.gotests/streams/utils/erc20/helper.go
📚 Learning: 2025-08-29T13:02:14.526Z
Learnt from: outerlook
PR: trufnetwork/node#1126
File: tests/streams/utils/setup/primitive.go:324-0
Timestamp: 2025-08-29T13:02:14.526Z
Learning: outerlook prefers fail-fast behavior in test code using unsafe constructors (like util.Unsafe_NewEthereumAddressFromString) rather than error handling, as panicking on invalid input immediately highlights programming errors in controlled test environments.
Applied to files:
tests/extensions/erc20/erc20_bridge_multi_instance_test.gotests/extensions/erc20/erc20_bridge_actions_smoke_test.go
📚 Learning: 2025-09-12T01:16:28.145Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: internal/migrations/migration.go:34-43
Timestamp: 2025-09-12T01:16:28.145Z
Learning: In internal/migrations/migration.go, the maintainer (outerlook) prefers fail-fast behavior when the test_only directory is missing. The code should panic rather than gracefully handle missing test directories, as this indicates a build/configuration problem that should be caught immediately.
Applied to files:
tests/extensions/erc20/erc20_bridge_multi_instance_test.gotests/extensions/erc20/erc20_bridge_actions_smoke_test.go
🧬 Code graph analysis (6)
tests/extensions/erc20/erc20_bridge_multi_instance_test.go (2)
tests/extensions/erc20/common_test.go (4)
TestChain(20-20)TestERC20(23-23)TestUserA(24-24)TestAmount1(26-26)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)
tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (1)
tests/extensions/erc20/common_test.go (4)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)TestAmount1(26-26)
tests/extensions/erc20/erc20_bridge_transfer_test.go (3)
tests/extensions/erc20/common_test.go (7)
TestChain(20-20)TestEscrowA(21-21)TestERC20(23-23)TestUserA(24-24)TestAmount2(27-27)TestUserB(25-25)TestAmount1(26-26)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(44-69)
tests/streams/utils/erc20/helper.go (1)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)
tests/streams/complex_composed_test.go (3)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/cache/helper.go (2)
TestingT(119-122)SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
tests/streams/utils/runner.go (6)
tests/streams/utils/utils.go (2)
CacheOptions(33-33)NewCacheOptions(44-44)tests/streams/utils/cache/options.go (2)
CacheOptions(11-16)NewCacheOptions(28-32)tests/streams/utils/cache/helper.go (2)
SetupCacheTest(26-52)SetupCacheExtension(125-175)tests/streams/utils/service/service.go (1)
CreateDefaultService(37-43)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)extensions/tn_cache/syncschecker/sync_checker.go (1)
SyncChecker(24-37)
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (16)
tests/extensions/erc20/erc20_bridge_transfer_test.go (5)
1-1: KWIL test gating looks good.The kwiltest build tag correctly isolates this test for CI profiles using -tags=kwiltest.
34-41: Deposit injection + balance assertion reads clean.Good end-to-end coverage on the credit path; uses the real ordered-sync resolve and helper to fetch balance.
43-46: Zero-balance precondition on userB is a solid sanity check.This guards against leftover state across runs.
62-71: Post-transfer balance assertions are correct and minimal.Good black-box validation of remaining and received balances without re-testing internals.
48-52: Ensure block-height monotonicity vs injected resolve point.engCtx(...) sets TxContext.BlockContext.Height (tests/extensions/erc20/common_test.go:66–74) while the injector resolves with orderedsync.ForTestingResolve(... &common.BlockContext{Height: point}) (tests/streams/utils/erc20/inject.go:74). The test currently calls engCtx(..., 2, ...) (tests/extensions/erc20/erc20_bridge_transfer_test.go:48–52) — use engCtx height ≥ point+1 (e.g., if point == 10 use ≥11) or confirm the engine path ignores BlockContext.Height for ordering/TTL/authz.
go.mod (3)
5-5: Toolchain directive: ensure runners allow auto-download.go toolchain go1.24.4 may auto-fetch; if CI blocks downloads, preinstall the toolchain or pin setup-go to 1.24.4.
85-85: Confirm geth as a direct dep or revert to indirect.
go.mod (line 85) lists github.com/ethereum/go-ethereum v1.14.13 — sandbox rg/go mod tidy was inconclusive; verify whether any package imports github.com/ethereum/go-ethereum. If none, rungo mod tidyto demote/remove the direct require.
22-23: kwil-db pseudo-version bumps OK — no replace pins requiredgo.mod contains both github.com/trufnetwork/kwil-db v0.10.3-0.20250911225741-d6cb2b2747ff and github.com/trufnetwork/kwil-db/core v0.4.3-0.20250911225741-d6cb2b2747ff (lines 22–23) with matching timestamp/hash; ran go mod tidy -diff (no changes) and no replace directives referencing kwil-db were found.
tests/extensions/erc20/erc20_bridge_multi_instance_test.go (1)
28-45: LGTM: instance seeding and isolated inject are clear.Good use of ForTestingSeedAndActivateInstance and a single directed inject to escrowA.
tests/streams/complex_composed_test.go (1)
121-128: Cache refresh precondition is solid.Verifying recordsCached > 0 prevents false positives.
tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (2)
35-57: Info action: intentional fail-fast is consistent.Direct indexing/type assertions align with your schema-drift stance.
74-89: Scale-up/down checks are clear and minimal.Accepting both “1” and “1.000…” is pragmatic; keeps the test robust to formatting.
tests/streams/utils/erc20/helper.go (1)
71-92: Big-int amount handling: good choice.Avoids int64 overflow and sets precision/scale explicitly.
tests/streams/utils/runner.go (2)
50-58: Default-enabling cache may surprise existing testsToday, if options are nil, cache gets enabled by default. That changes baseline behavior. Consider defaulting to disabled unless explicitly opted-in, or confirm this is intentional.
56-57: Use 0 (not -1s) to disable sync check per SyncChecker contractSyncChecker’s comment says 0 = disabled. Passing -1s may parse oddly depending on the loader.
- WithMaxBlockAge(-1 * time.Second). // Disable sync check + WithMaxBlockAge(0). // Disable sync check (0 = disabled)To be safe, verify how CacheOptions.Build() encodes this and how tn_cache parses it.
tests/streams/utils/erc20/config.go (1)
98-121: URL validation looks solidScheme and host checks on ws/wss endpoints are appropriate for WebSocket RPCs.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/streams/complex_composed_test.go (1)
610-613: Fix assertion: you state “should differ” but assert equality.Use NotEqual to actually detect the inconsistency.
- assert.Equal(t, valueFromLatestOnlyPath, valueFromLatestRangedPath, - "Values for latest event_time (%d) should differ. 'Latest only' path gave '%s', 'ranged' path gave '%s'. This demonstrates the inconsistency.", + assert.NotEqual(t, valueFromLatestOnlyPath, valueFromLatestRangedPath, + "Values for latest event_time (%d) should differ. 'Latest only' path gave '%s', 'ranged' path gave '%s'.", latestEventTime, valueFromLatestOnlyPath, valueFromLatestRangedPath)
♻️ Duplicate comments (7)
tests/streams/utils/erc20/config.go (2)
129-133: DevNull check looks good and is cross-platform.This addresses the earlier portability nit around “/dev/null”.
147-152: Correct numeric validation (replaces URL parsing).Good switch to strconv for chunk size validation.
tests/streams/utils/erc20/helper.go (3)
1-4: Good: kwiltest build tag added.Prevents type-check issues for test-only APIs.
82-91: Good: big.Int-based amount parsing avoids overflow.Matches ERC-20 magnitude expectations.
13-17: Use erc20bridge import alias and update calls to match test helperstests/streams/utils/erc20/helper.go imports the erc20 package as erc20shim but test helpers elsewhere call erc20bridge; change the alias and calls to avoid undefined symbols. Location: tests/streams/utils/erc20/helper.go (lines 13-17 and 27-31).
- erc20shim "github.com/trufnetwork/kwil-db/node/exts/erc20-bridge/erc20" + erc20bridge "github.com/trufnetwork/kwil-db/node/exts/erc20-bridge/erc20" @@ - erc20shim.ForTestingDisableInstance(context.Background(), txPlatform, alias, escrowAddr, alias) - erc20shim.ForTestingResetSingleton() + erc20bridge.ForTestingDisableInstance(context.Background(), txPlatform, alias, escrowAddr, alias) + erc20bridge.ForTestingResetSingleton() @@ - erc20shim.ForTestingSeedAndActivateInstance(context.Background(), txPlatform, alias, escrowAddr, "0x2222222222222222222222222222222222222222", 18, 60, alias) + erc20bridge.ForTestingSeedAndActivateInstance(context.Background(), txPlatform, alias, escrowAddr, "0x2222222222222222222222222222222222222222", 18, 60, alias)tests/streams/complex_composed_test.go (1)
55-56: *Good: removed unsafe t.(testing.T) casts in RunInTx closures.Eliminates potential panics from interface downcasts.
Also applies to: 63-65
tests/streams/utils/cache/helper.go (1)
54-60: Good: Cleanup clears all tn_cache test overrides.Prevents cross-test leakage of config/DB.
🧹 Nitpick comments (6)
go.mod (1)
85-85: go-ethereum is now a direct dependency — confirm it’s truly needed at build/test scope.If it’s only referenced by kwiltest-gated tests, ensure CI always runs with -tags=kwiltest; otherwise,
go mod tidymay flip this back to indirect. If production code doesn’t import it, consider isolating usages in test helpers to keep the dependency test-scoped.tests/streams/utils/erc20/config.go (1)
68-72: Builder exposes fields that never flow into Build() (AutoStart, Timeout, MockListeners).If external config supports these, wire them; otherwise remove to avoid dead config.
Also applies to: 80-96
tests/streams/utils/erc20/helper.go (2)
19-21: Docstring mismatch with function name.Comment says WithERC20TestSetupTx; function is WithERC20TestSetup.
-// WithERC20TestSetupTx is a transaction-aware version that works with WithTx for perfect isolation +// WithERC20TestSetup is a transaction-aware version that works with WithTx for perfect isolation
54-61: Prefer fail-fast row handling per maintainer preference.Directly index row.Values[0] and panic on mismatch to catch schema drift sooner.
Context: Maintainer prefers panic-on-schema-change in ERC20 tests (retrieved learnings).
tests/streams/utils/runner.go (2)
33-40: **Avoid risky type assertion to testing.T; accept testing.T explicitly.Signature already used only from tests. Removing the cast eliminates panic risk.
-func RunSchemaTest(t TestingT, s kwilTesting.SchemaTest, options *Options) { - // Convert to kwilTesting.Options - use type assertion since TestingT is an alias for testing.T - testT := t.(*testing.T) +func RunSchemaTest(testT *testing.T, s kwilTesting.SchemaTest, options *Options) {
92-101: Ensure cleanups run and surface panics as test errors.Add recover in the deferred cleanup to convert panics into testErr while still running cleanups.
defer func() { + if r := recover(); r != nil { + testErr = fmt.Errorf("panic: %v", r) + } for i := len(cleanups) - 1; i >= 0; i-- { if cleanups[i] != nil { cleanups[i]() } } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
go.mod(2 hunks)internal/migrations/test_only/bootstrap_erc20.sql(1 hunks)tests/extensions/erc20/erc20_bridge_actions_smoke_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_injection_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_multi_instance_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_transfer_test.go(1 hunks)tests/streams/complex_composed_test.go(5 hunks)tests/streams/utils/cache/helper.go(1 hunks)tests/streams/utils/erc20/config.go(1 hunks)tests/streams/utils/erc20/helper.go(1 hunks)tests/streams/utils/erc20/inject.go(1 hunks)tests/streams/utils/eventstore/mock.go(1 hunks)tests/streams/utils/runner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/streams/utils/erc20/inject.go
- tests/extensions/erc20/erc20_bridge_transfer_test.go
- tests/extensions/erc20/erc20_bridge_actions_smoke_test.go
- tests/streams/utils/eventstore/mock.go
- tests/extensions/erc20/erc20_bridge_multi_instance_test.go
- internal/migrations/test_only/bootstrap_erc20.sql
- tests/extensions/erc20/erc20_bridge_injection_test.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-12T01:19:04.515Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47
Timestamp: 2025-09-12T01:19:04.515Z
Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
Applied to files:
tests/streams/utils/erc20/helper.go
📚 Learning: 2025-09-12T01:16:55.153Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_epoch_test.go:31-32
Timestamp: 2025-09-12T01:16:55.153Z
Learning: In the ERC20 bridge tests, the test environment is reset after each test via seedAndRun function and cleanup mechanisms, so using shared aliases like TestChain is acceptable and doesn't cause collisions between tests.
Applied to files:
tests/streams/utils/erc20/helper.go
🧬 Code graph analysis (4)
tests/streams/utils/cache/helper.go (6)
tests/streams/utils/utils.go (2)
CacheOptions(33-33)CacheTestHelper(37-37)tests/streams/utils/cache/options.go (1)
CacheOptions(11-16)extensions/tn_cache/test_init.go (3)
SetTestDBConfiguration(24-26)SetTestDB(30-32)SetTestConfiguration(19-21)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)extensions/tn_cache/syncschecker/sync_checker.go (1)
SyncChecker(24-37)
tests/streams/utils/runner.go (5)
tests/streams/utils/utils.go (2)
CacheOptions(33-33)NewCacheOptions(44-44)tests/streams/utils/cache/options.go (2)
CacheOptions(11-16)NewCacheOptions(28-32)tests/streams/utils/cache/helper.go (2)
SetupCacheTest(26-52)SetupCacheExtension(125-175)tests/streams/utils/service/service.go (1)
CreateDefaultService(37-43)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)
tests/streams/utils/erc20/helper.go (1)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)
tests/streams/complex_composed_test.go (3)
tests/streams/utils/utils.go (1)
SimpleCache(77-83)tests/streams/utils/cache/helper.go (2)
TestingT(119-122)SetupCacheTest(26-52)extensions/tn_cache/test_helpers.go (1)
GetTestHelper(82-84)
🪛 GitHub Check: lint
tests/streams/utils/runner.go
[failure] 75-75:
undefined: orderedsync.ForTestingReset (typecheck)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (5)
go.mod (1)
22-23: kwil-db(core) bumped to the same snapshot — verify API compat and lock reproducibility.Good that both modules point to the same pseudo-version commit. Please:
- Run the full test matrix (including -tags=kwiltest) to ensure no subtle API shifts.
- Commit the updated go.sum (and vendor/ if you vendor) to keep builds reproducible across CI/dev.
tests/streams/complex_composed_test.go (2)
646-648: Row count expectation/message mismatch.Asserting 12 with message “Expected 13 rows.” Confirm intended count and fix either value or message.
- assert.Equal(t, 12, len(result), "Expected 13 rows") + assert.Equal(t, 13, len(result), "Expected 13 rows")
121-129: Good: centralized refresh via tn_cache test helper before assertions.Ensures deterministic cache state.
Also applies to: 701-705
tests/streams/utils/runner.go (1)
74-76: Undefined symbol: orderedsync.ForTestingReset (per lint).Confirm the helper name in ordered-sync; update or remove the call to fix type-check failures.
Potential fixes:
- Rename to the actual exported test reset (e.g., ResetForTesting/ForTestingClear).
- Or drop the reset if WithTx/DB reinit already guarantees isolation.
tests/streams/utils/cache/helper.go (1)
97-116: Transaction wrapper looks correct.Begins tx, defers rollback, and swaps DB in helper for isolation.
…tension alias This commit refactors multiple ERC-20 bridge test files to replace direct references to the `TestChain` constant with the new `TestExtensionAlias`. Key changes include: - Updated instance activation calls in various test files to ensure consistent use of the extension alias. - Enhanced clarity and maintainability of the test suite by standardizing the way instances are activated across tests. These adjustments aim to improve the robustness of the ERC-20 bridge testing framework and facilitate easier future enhancements.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
tests/streams/utils/runner.go (4)
56-57: Use 0s to disable sync-check per tn_cache contractsync_checker documents “0 = disabled.” Using -1s is ambiguous.
Apply:
- WithMaxBlockAge(-1 * time.Second). // Disable sync check + WithMaxBlockAge(0 * time.Second). // Disable sync check (0 = disabled)
68-72: Drop unused ERC-20 wrapper plumbingwrapWithExtensionsSetup no longer uses erc20Config. Remove the param to reduce confusion (and its import).
Apply:
- if cacheConfig != nil || erc20Config != nil { - wrappedTests = wrapWithExtensionsSetup(context.Background(), s.FunctionTests, cacheConfig, erc20Config, kwilOpts) + if cacheConfig != nil { + wrappedTests = wrapWithExtensionsSetup(context.Background(), s.FunctionTests, cacheConfig, kwilOpts) }-func wrapWithExtensionsSetup(ctx context.Context, originalFuncs []kwilTesting.TestFunc, cacheConfig *cache.CacheOptions, erc20Config *erc20.ERC20BridgeConfig, opts *kwilTesting.Options) []kwilTesting.TestFunc { +func wrapWithExtensionsSetup(ctx context.Context, originalFuncs []kwilTesting.TestFunc, cacheConfig *cache.CacheOptions, opts *kwilTesting.Options) []kwilTesting.TestFunc {Also applies to: 85-87
33-35: Avoid panicking type assertion on tThis will panic if a different TestingT is passed. Either accept *testing.T explicitly or guard the assertion.
Example:
-func RunSchemaTest(t TestingT, s kwilTesting.SchemaTest, options *Options) { - // Convert to kwilTesting.Options - use type assertion since TestingT is an alias for testing.T - testT := t.(*testing.T) +func RunSchemaTest(testT *testing.T, s kwilTesting.SchemaTest, options *Options) {
94-101: Also recover to convert panics into test errors (cleanups already deferred)Optional: recover to set testErr so a single test panic doesn’t abort the whole run.
Apply:
- defer func() { + defer func() { + if r := recover(); r != nil { + testErr = fmt.Errorf("panic: %v", r) + } for i := len(cleanups) - 1; i >= 0; i-- { if cleanups[i] != nil { cleanups[i]() } } }()tests/streams/utils/erc20/config.go (1)
128-136: Optional: make scheme check case-insensitiveMinor robustness: accept WS/WSS in any case.
Apply:
- if u.Scheme != "ws" && u.Scheme != "wss" { + s := strings.ToLower(u.Scheme) + if s != "ws" && s != "wss" { return fmt.Errorf("RPC URL for chain %s must use WebSocket protocol (ws:// or wss://), got %s", chain, u.Scheme) }And add:
-import ( +import ( "fmt" "net/url" "os" "strconv" "time" + "strings" )tests/extensions/erc20/erc20_bridge_epoch_test.go (1)
39-51: Tighten the pre-finalize assertion to this test’s recipientCurrently counts all rows in epoch_rewards; filter by recipient to avoid future false positives if isolation semantics change.
Apply:
- preQ := ` - {kwil_erc20_meta}SELECT count(*) FROM epoch_rewards` + preQ := ` + {kwil_erc20_meta}SELECT count(*) FROM epoch_rewards WHERE recipient = $recipient` + ubytes := ethcommon.HexToAddress(user).Bytes() var preRows int - err := platform.Engine.ExecuteWithoutEngineCtx(ctx, platform.DB, preQ, nil, func(row *common.Row) error { + err := platform.Engine.ExecuteWithoutEngineCtx(ctx, platform.DB, preQ, map[string]any{"recipient": ubytes}, func(row *common.Row) error { if len(row.Values) != 1 { return nil } preRows = int(row.Values[0].(int64)) return nil })And later reuse the existing variable:
- ubytes := ethcommon.HexToAddress(user).Bytes() + // ubytes already computed aboveAlso applies to: 64-66
tests/extensions/erc20/erc20_bridge_end_to_end_test.go (1)
39-45: Fail fast if info() shape changesAvoid silently ignoring schema drift; panic on mismatch to surface the issue immediately.
Apply:
- resInfo, errInfo := platform.Engine.Call(engineCtx, platform.DB, TestExtensionAlias, "info", []any{}, func(row *common.Row) error { - if len(row.Values) < 9 { - return nil - } + resInfo, errInfo := platform.Engine.Call(engineCtx, platform.DB, TestExtensionAlias, "info", []any{}, func(row *common.Row) error { syncedResult = row.Values[6].(bool) enabledResult = row.Values[8].(bool) return nil })tests/extensions/erc20/erc20_bridge_simple_test.go (1)
79-96: Assert pre-lock credited balance to tighten the test.Verify that the credit landed before locking to avoid false positives if lock_admin no-ops.
Apply:
err := testerc20.CreditUserBalance(context.Background(), txPlatform, TestChain, TestEscrowA, testUser, testAmount) require.NoError(t, err) + // Sanity-check the credited balance before locking + preBalance, err := testerc20.GetUserBalance(context.Background(), txPlatform, TestExtensionAlias, testUser) + require.NoError(t, err) + require.Equal(t, testAmount, preBalance, "credited balance mismatch before admin lock") + // Step 2: Execute admin lock operation // This should reduce user's balance by the locked amount err = testerc20.CallLockAdmin(context.Background(), txPlatform, TestExtensionAlias, testUser, testAmount)tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (1)
61-66: Remove len checks to keep fail-fast behavior consistent across handlers.For decimals(), scale_up(), and scale_down(), the len guards mute schema/shape regressions. Prefer direct indexing to panic on drift, as used in info().
Apply:
r, err = platform.Engine.Call(engineCtx, platform.DB, TestExtensionAlias, "decimals", []any{}, func(row *common.Row) error { - if len(row.Values) != 1 { - return nil - } decimalsResult = row.Values[0].(int64) return nil }) @@ r, err = platform.Engine.Call(engineCtx, platform.DB, TestExtensionAlias, "scale_up", []any{"1"}, func(row *common.Row) error { - if len(row.Values) != 1 { - return nil - } scaledUpResult = row.Values[0].(*types.Decimal) return nil }) @@ r, err = platform.Engine.Call(engineCtx, platform.DB, TestExtensionAlias, "scale_down", []any{amountDecimal}, func(row *common.Row) error { - if len(row.Values) != 1 { - return nil - } scaledDownResult = row.Values[0].(string) return nil })Also applies to: 77-82, 98-103
tests/streams/utils/erc20/helper.go (1)
37-41: Parameter naming: make the intent explicit (this arg is the chain).The first argument is used as the chain when calling InjectERC20Transfer. Rename for clarity to prevent misuse.
Apply:
-func CreditUserBalance(ctx context.Context, platform *kwilTesting.Platform, extensionAlias, escrowAddr, userAddr, amount string) error { +func CreditUserBalance(ctx context.Context, platform *kwilTesting.Platform, chain, escrowAddr, userAddr, amount string) error { // Use the platform's DB and Engine (could be transaction-scoped) return InjectERC20Transfer( - ctx, platform, extensionAlias, escrowAddr, "0x2222222222222222222222222222222222222222", userAddr, escrowAddr, amount, 10, nil) + ctx, platform, chain, escrowAddr, "0x2222222222222222222222222222222222222222", userAddr, escrowAddr, amount, 10, nil) }Call sites (e.g., simple_test.go) already pass TestChain, so no further changes needed.
tests/extensions/erc20/common_test.go (1)
37-41: Use a non-cancelled context in cleanup.Cleanup runs after the wrapped function returns; ctx may be cancelled. Use Background to ensure teardown completes.
Apply:
- t.Cleanup(func() { - erc20shim.ForTestingClearAllInstances(ctx, platform) - erc20shim.ForTestingResetSingleton() - }) + t.Cleanup(func() { + erc20shim.ForTestingClearAllInstances(context.Background(), platform) + erc20shim.ForTestingResetSingleton() + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
tests/extensions/erc20/common_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_actions_smoke_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_admin_authz_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_consecutive_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_end_to_end_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_epoch_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_injection_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_simple_test.go(1 hunks)tests/extensions/erc20/erc20_bridge_transfer_test.go(1 hunks)tests/streams/utils/erc20/config.go(1 hunks)tests/streams/utils/erc20/helper.go(1 hunks)tests/streams/utils/runner.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/extensions/erc20/erc20_bridge_injection_test.go
- tests/extensions/erc20/erc20_bridge_consecutive_test.go
- tests/extensions/erc20/erc20_bridge_admin_authz_test.go
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-12T01:19:04.515Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_actions_smoke_test.go:40-47
Timestamp: 2025-09-12T01:19:04.515Z
Learning: In ERC20 bridge tests, the maintainer (outerlook) prefers fail-fast behavior through panics when schema changes occur. Direct indexing and type assertions in test row handlers should be maintained to immediately catch unexpected schema changes, rather than using graceful error handling.
Applied to files:
tests/extensions/erc20/erc20_bridge_simple_test.gotests/extensions/erc20/erc20_bridge_actions_smoke_test.gotests/streams/utils/erc20/helper.gotests/extensions/erc20/erc20_bridge_epoch_test.gotests/extensions/erc20/erc20_bridge_transfer_test.gotests/extensions/erc20/erc20_bridge_end_to_end_test.go
📚 Learning: 2025-09-12T12:03:13.768Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/streams/utils/erc20/helper.go:37-41
Timestamp: 2025-09-12T12:03:13.768Z
Learning: In the ERC20 bridge test helpers at tests/streams/utils/erc20/helper.go, the extensionAlias parameter serves dual purpose as both the extension identifier and the chain name. This is a deliberate design choice in their test setup where extensionAlias and chain are the same value.
Applied to files:
tests/extensions/erc20/erc20_bridge_simple_test.gotests/streams/utils/erc20/helper.gotests/extensions/erc20/erc20_bridge_epoch_test.gotests/extensions/erc20/common_test.go
📚 Learning: 2025-09-12T01:16:28.145Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: internal/migrations/migration.go:34-43
Timestamp: 2025-09-12T01:16:28.145Z
Learning: In internal/migrations/migration.go, the maintainer (outerlook) prefers fail-fast behavior when the test_only directory is missing. The code should panic rather than gracefully handle missing test directories, as this indicates a build/configuration problem that should be caught immediately.
Applied to files:
tests/extensions/erc20/erc20_bridge_actions_smoke_test.gotests/extensions/erc20/erc20_bridge_epoch_test.go
📚 Learning: 2025-08-29T13:02:14.526Z
Learnt from: outerlook
PR: trufnetwork/node#1126
File: tests/streams/utils/setup/primitive.go:324-0
Timestamp: 2025-08-29T13:02:14.526Z
Learning: outerlook prefers fail-fast behavior in test code using unsafe constructors (like util.Unsafe_NewEthereumAddressFromString) rather than error handling, as panicking on invalid input immediately highlights programming errors in controlled test environments.
Applied to files:
tests/extensions/erc20/erc20_bridge_actions_smoke_test.gotests/extensions/erc20/erc20_bridge_epoch_test.go
📚 Learning: 2025-09-12T01:16:55.153Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/extensions/erc20/erc20_bridge_epoch_test.go:31-32
Timestamp: 2025-09-12T01:16:55.153Z
Learning: In the ERC20 bridge tests, the test environment is reset after each test via seedAndRun function and cleanup mechanisms, so using shared aliases like TestChain is acceptable and doesn't cause collisions between tests.
Applied to files:
tests/streams/utils/erc20/helper.gotests/extensions/erc20/erc20_bridge_epoch_test.gotests/extensions/erc20/common_test.go
📚 Learning: 2025-09-12T12:01:30.882Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/streams/utils/erc20/config.go:82-86
Timestamp: 2025-09-12T12:01:30.882Z
Learning: The config.ERC20BridgeConfig struct from github.com/trufnetwork/kwil-db/config uses field names "BlockSyncChuckSize" (with "Chuck") and "Signer" (singular), not the more conventional "BlockSyncChunkSize" and "Signers" spellings. When building external config structs, always verify the actual field names in the external dependency rather than assuming conventional naming.
Applied to files:
tests/streams/utils/erc20/config.go
📚 Learning: 2025-09-12T12:01:30.882Z
Learnt from: outerlook
PR: trufnetwork/node#1147
File: tests/streams/utils/erc20/config.go:82-86
Timestamp: 2025-09-12T12:01:30.882Z
Learning: The config.ERC20BridgeConfig struct from github.com/trufnetwork/kwil-db/config uses field names "BlockSyncChuckSize" (with "Chuck") and "Signer" (singular), not the more conventional "BlockSyncChunkSize" and "Signers" spellings.
Applied to files:
tests/streams/utils/erc20/config.go
🧬 Code graph analysis (8)
tests/extensions/erc20/erc20_bridge_simple_test.go (2)
tests/extensions/erc20/common_test.go (4)
TestChain(20-20)TestEscrowA(22-22)TestERC20(24-24)TestExtensionAlias(21-21)tests/streams/utils/erc20/helper.go (3)
GetUserBalance(44-69)CreditUserBalance(37-41)CallLockAdmin(73-100)
tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (1)
tests/extensions/erc20/common_test.go (5)
TestChain(20-20)TestEscrowA(22-22)TestERC20(24-24)TestExtensionAlias(21-21)TestAmount1(27-27)
tests/streams/utils/erc20/helper.go (1)
tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)
tests/extensions/erc20/erc20_bridge_epoch_test.go (2)
tests/extensions/erc20/common_test.go (1)
TestExtensionAlias(21-21)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)
tests/streams/utils/runner.go (6)
tests/streams/utils/utils.go (2)
CacheOptions(33-33)NewCacheOptions(44-44)tests/streams/utils/cache/options.go (2)
CacheOptions(11-16)NewCacheOptions(28-32)tests/streams/utils/cache/helper.go (2)
SetupCacheTest(26-52)SetupCacheExtension(125-175)tests/streams/utils/service/service.go (1)
CreateDefaultService(37-43)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)extensions/tn_cache/syncschecker/sync_checker.go (1)
SyncChecker(24-37)
tests/extensions/erc20/erc20_bridge_transfer_test.go (3)
tests/extensions/erc20/common_test.go (8)
TestChain(20-20)TestEscrowA(22-22)TestERC20(24-24)TestExtensionAlias(21-21)TestUserA(25-25)TestAmount2(28-28)TestUserB(26-26)TestAmount1(27-27)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(44-69)
tests/extensions/erc20/erc20_bridge_end_to_end_test.go (3)
tests/extensions/erc20/common_test.go (6)
TestChain(20-20)TestEscrowA(22-22)TestERC20(24-24)TestExtensionAlias(21-21)TestUserA(25-25)TestAmount1(27-27)tests/streams/utils/erc20/inject.go (1)
InjectERC20Transfer(24-79)tests/streams/utils/erc20/helper.go (1)
GetUserBalance(44-69)
tests/extensions/erc20/common_test.go (3)
tests/streams/utils/runner.go (3)
TestingT(173-176)RunSchemaTest(32-83)Options(22-28)internal/migrations/migration.go (1)
GetSeedScriptPaths(13-50)tests/streams/utils/utils.go (1)
GetTestOptions(56-60)
⏰ 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). (2)
- GitHub Check: lint
- GitHub Check: acceptance-test
🔇 Additional comments (9)
tests/streams/utils/runner.go (1)
134-145: Good: no leaks on early returns and correct teardown orderhelper.Cleanup() on errors prevents leaks; in cleanup you stop scheduler/checker, close, clear SetExtension(nil), then cleanup helper. Solid.
Also applies to: 150-164
tests/streams/utils/erc20/config.go (3)
95-112: Field mapping aligns with external kwil-db configPropagating RPC, BlockSyncChuckSize, and Signer only (others test-only) matches the current external API. The inline note is clear.
140-154: Cross-platform dev-null check is correctUsing os.DevNull avoids hardcoding “/dev/null”. Nice.
161-172: Strictly positive chunk size validation looks goodParsing uint and rejecting 0 enforces the intended “positive integer” contract.
tests/extensions/erc20/erc20_bridge_transfer_test.go (1)
54-61: Good: capture and assert VM-level errors from Engine.CallChecking r.Error prevents false positives when transport err is nil but the VM fails.
tests/extensions/erc20/erc20_bridge_simple_test.go (2)
21-22: Seeding + activation flow looks correct.Single-step seed+activate with explicit alias/chain/escrow/erc20/decimals/period is clear and deterministic.
36-41: Clear zero-balance assertion.Good direct check of the initial state via the extension entrypoint.
tests/extensions/erc20/erc20_bridge_actions_smoke_test.go (1)
41-46: Fail-fast row handling aligns with project preference.Direct indexing/type assertions for info() match the fail-fast approach for schema drift.
tests/streams/utils/erc20/helper.go (1)
1-5: Correct kwiltest gating.Build tag ensures these helpers only compile under test-only APIs.
…k call This commit enhances the error handling in the ERC-20 test setup by adding a check for the success of the `ForTestingSeedAndActivateInstance` function, ensuring that any activation failures are reported clearly. Additionally, it improves the `CallLockAdmin` function to handle potential execution errors from the engine call, providing more informative error messages. These changes aim to increase the robustness and reliability of the ERC-20 bridge testing framework.
…SchemaTest for cache handling This commit updates various test files to replace instances of `kwilTesting.RunSchemaTest` with `testutils.RunSchemaTest`, ensuring that all tests utilize the new caching mechanism. This change enhances test reliability by ensuring proper cache management and isolation during test execution. Key updates include modifications across multiple test files, improving consistency and maintainability in the testing framework.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/streams/composed_test.go (1)
99-105: Undefined identifier: composedStreamLocator used but not declared.Both testComposedLastAvailable and testCOMPOSED01SetTaxonomyWithValidData reference composedStreamLocator without a local or package-level declaration, causing a compile error.
Apply this diff to define a shared locator alongside the existing vars:
var ( composedStreamName = "composed_stream" composedStreamId = util.GenerateStreamId(composedStreamName) composedDeployer = util.Unsafe_NewEthereumAddressFromString("0x0000000000000000000000000000000000000123") + composedStreamLocator = types.StreamLocator{ + StreamId: composedStreamId, + DataProvider: composedDeployer, + } )Also applies to: 140-145
tests/streams/multi_level_composed_test.go (1)
135-146: DataProvider mismatch breaks lookups.Streams are created with deployerMultiLevel, but queries use deployerSharedComplex. This will fail to locate streams by locator.
Apply this diff to use the correct deployer:
- multiLevelLocator := types.StreamLocator{StreamId: parentComposed, DataProvider: deployerSharedComplex} + multiLevelLocator := types.StreamLocator{StreamId: parentComposed, DataProvider: deployerMultiLevel} ... - StreamLocator: types.StreamLocator{StreamId: composed1, DataProvider: deployerSharedComplex}, + StreamLocator: types.StreamLocator{StreamId: composed1, DataProvider: deployerMultiLevel}, ... - StreamLocator: types.StreamLocator{StreamId: composed2, DataProvider: deployerSharedComplex}, + StreamLocator: types.StreamLocator{StreamId: composed2, DataProvider: deployerMultiLevel}, ... - StreamLocator: multiLevelLocator, + StreamLocator: multiLevelLocator,Also applies to: 140-147, 163-169, 185-191
♻️ Duplicate comments (3)
tests/streams/utils/runner.go (3)
109-117: Cleanup now runs on panic/early return — niceDefer-based reverse cleanup ensures no leaks even if the test body fails.
149-160: Early-return leak fixedCalling helper.Cleanup() on ParseConfig/SetupCacheExtension errors prevents resource leaks.
165-179: Correct cleanup orderingStopping scheduler/checker, closing the extension, then clearing registration before helper cleanup is the right order.
🧹 Nitpick comments (6)
tests/streams/other/other_test.go (4)
31-50: Register the correct data provider or confirm auto-create on role grant.Inside ValidAddress subtest you grant network_writer to validAddress, but CreateDataProvider is called for defaultStreamLocator.DataProvider, not validAddress. If AddMemberToRoleBypass does not auto-create the provider, stream creation can fail or be flaky. Prefer registering the actual owner or use GrantRoles to trigger auto-create.
Apply one of the following:
- // Test with valid Ethereum address - err = setup.CreateDataProvider(ctx, platform, defaultStreamLocator.DataProvider.Address()) + // Test with valid Ethereum address + err = setup.CreateDataProvider(ctx, platform, validAddress.Address())or, to keep ops within the tx subtest:
- err = setup.CreateDataProvider(ctx, platform, validAddress.Address()) + err = setup.CreateDataProvider(ctx, txPlatform, validAddress.Address())
37-50: Use txPlatform consistently inside WithTx closures.Within the WithTx subtests, some calls use platform and others txPlatform. Prefer txPlatform for DB-mutating setup to keep all changes within the same transactional scope and reduce confusion.
206-209: Remove duplicate test invocation to save runtime.The same test function is added twice; each RunSchemaTest call isolates state, so duplication doesn’t add coverage.
FunctionTests: []kwilTesting.TestFunc{ - testAnyUserCanCreateStream(t), - testAnyUserCanCreateStream(t), + testAnyUserCanCreateStream(t), },
158-191: Make the uniqueness expectation explicit.NonDuplicateStreamID logs either branch (global unique vs unique-per-owner) without asserting. Given the requirement comment says “unique by each stream owner,” assert the expected behavior to avoid silent regressions.
tests/streams/roles/permission_gates_test.go (1)
19-29: Harness migration LGTM.Minor: consider centralizing expected error substrings as constants to reduce drift if messages change.
tests/streams/utils/runner.go (1)
66-76: Remove dead ERC-20 wiring from wrapper; tighten wrap conditionERC-20 setup is “handled directly in tests,” yet its local var gates wrapping and the wrapper takes unused params.
- // Handle ERC-20 bridge configuration - var erc20Config *erc20.ERC20BridgeConfig - if options != nil && options.ERC20Bridge != nil && !options.DisableERC20Bridge { - erc20Config = options.ERC20Bridge - } @@ - if cacheConfig != nil || erc20Config != nil { - wrappedTests = wrapWithExtensionsSetup(context.Background(), s.FunctionTests, cacheConfig, erc20Config, kwilOpts) + if cacheConfig != nil { + wrappedTests = wrapWithExtensionsSetup(context.Background(), s.FunctionTests, cacheConfig) } @@ -func wrapWithExtensionsSetup(ctx context.Context, originalFuncs []kwilTesting.TestFunc, cacheConfig *cache.CacheOptions, erc20Config *erc20.ERC20BridgeConfig, opts *kwilTesting.Options) []kwilTesting.TestFunc { +func wrapWithExtensionsSetup(ctx context.Context, originalFuncs []kwilTesting.TestFunc, cacheConfig *cache.CacheOptions) []kwilTesting.TestFunc {Also applies to: 102-105
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
extensions/tn_cache/internal/engine_ops_integration_test.go(2 hunks)extensions/tn_cache/internal/engine_ops_permissions_test.go(1 hunks)extensions/tn_digest/engine_ops_integration_test.go(1 hunks)internal/benchmark/digest/digest_benchmark_test.go(1 hunks)tests/streams/auth/auth_test.go(9 hunks)tests/streams/common_test.go(4 hunks)tests/streams/composed_test.go(2 hunks)tests/streams/comprehensive_shared_path_independence_test.go(1 hunks)tests/streams/database_size_test.go(1 hunks)tests/streams/digest/digest_actions_test.go(3 hunks)tests/streams/gamefi_index_test.go(1 hunks)tests/streams/index_change_test.go(1 hunks)tests/streams/multi_level_composed_test.go(1 hunks)tests/streams/other/other_test.go(4 hunks)tests/streams/other/stream_exists_batch_test.go(1 hunks)tests/streams/pending_prune_days_test.go(1 hunks)tests/streams/primitive_batch_insert_alignment_test.go(1 hunks)tests/streams/primitive_test.go(1 hunks)tests/streams/query/metadata_test.go(2 hunks)tests/streams/query/query_test.go(2 hunks)tests/streams/roles/permission_gates_test.go(2 hunks)tests/streams/roles/role_management_test.go(2 hunks)tests/streams/taxonomy_query_actions_test.go(2 hunks)tests/streams/truflation_composed_frozen_test.go(2 hunks)tests/streams/truflation_primitive_frozen_test.go(2 hunks)tests/streams/utils/runner.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- extensions/tn_cache/internal/engine_ops_integration_test.go
🧰 Additional context used
🧬 Code graph analysis (25)
tests/streams/primitive_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/multi_level_composed_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
extensions/tn_cache/internal/engine_ops_permissions_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/query/query_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/taxonomy_query_actions_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
extensions/tn_digest/engine_ops_integration_test.go (1)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)
tests/streams/primitive_batch_insert_alignment_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/pending_prune_days_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/comprehensive_shared_path_independence_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/truflation_primitive_frozen_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/other/stream_exists_batch_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/gamefi_index_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/composed_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/roles/role_management_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/database_size_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/query/metadata_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
internal/benchmark/digest/digest_benchmark_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/roles/permission_gates_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/other/other_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/truflation_composed_frozen_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/common_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/digest/digest_actions_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/index_change_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/auth/auth_test.go (2)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
tests/streams/utils/runner.go (7)
tests/streams/utils/utils.go (2)
CacheOptions(33-33)NewCacheOptions(44-44)tests/streams/utils/cache/options.go (2)
CacheOptions(11-16)NewCacheOptions(28-32)tests/streams/utils/erc20/config.go (1)
ERC20BridgeConfig(16-23)tests/extensions/erc20/common_test.go (1)
TestingT(82-86)tests/streams/utils/cache/helper.go (3)
TestingT(119-122)SetupCacheTest(26-52)SetupCacheExtension(125-175)tests/streams/utils/service/service.go (1)
CreateDefaultService(37-43)extensions/tn_cache/tn_cache.go (1)
ParseConfig(36-73)
🪛 GitHub Check: lint
extensions/tn_digest/engine_ops_integration_test.go
[failure] 31-31:
undefined: testutils (typecheck)
⏰ 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). (1)
- GitHub Check: acceptance-test
🔇 Additional comments (31)
tests/streams/gamefi_index_test.go (1)
101-101: Harness switch LGTM.Using testutils.RunSchemaTest with GetTestOptionsWithCache() aligns this test with the new configurable test runner. No further action.
Also applies to: 109-109
tests/streams/roles/role_management_test.go (1)
84-84: Harness migration looks good.The switch to testutils.RunSchemaTest/GetTestOptionsWithCache() is consistent with the suite-wide pattern.
Also applies to: 97-97
tests/streams/composed_test.go (1)
27-27: Harness switch LGTM.Consistent use of the new runner and cached options.
Also applies to: 38-38
tests/streams/multi_level_composed_test.go (1)
35-35: Harness migration approved.Matches the new test runner and options convention.
Also applies to: 41-41
tests/streams/pending_prune_days_test.go (1)
24-31: Runner/options update LGTM.Adopts the shared runner and cache-enabled options; no issues.
Also applies to: 35-42
tests/streams/query/query_test.go (1)
53-53: Update to testutils runner approved.Consistent with the rest of the suite.
Also applies to: 79-79
tests/streams/comprehensive_shared_path_independence_test.go (1)
35-35: Runner migration LGTM.Matches the project-wide change to testutils with cache-enabled options.
Also applies to: 41-41
tests/streams/primitive_test.go (1)
24-31: LGTM: migrated to testutils runner with cache optsChange aligns with the new runner contract and default cache enablement.
tests/streams/database_size_test.go (1)
26-33: LGTM: harness/options switch is correctRunner and options usage are consistent with tests/streams/utils.
tests/streams/index_change_test.go (1)
25-33: LGTM: moved to centralized runnerNo issues with the options handoff; defaults will enable cache as intended.
extensions/tn_cache/internal/engine_ops_permissions_test.go (1)
27-34: LGTM: runner migrationHarness switch and cached options look good for cache extension coverage.
tests/streams/taxonomy_query_actions_test.go (1)
34-51: LGTM: runner/options substitutionUsing testutils.RunSchemaTest with GetTestOptionsWithCache() matches the new API surface.
tests/streams/other/stream_exists_batch_test.go (1)
25-32: LGTM: centralized schema test harnessHarness migration and options look correct.
tests/streams/truflation_composed_frozen_test.go (1)
32-46: LGTM: migrated to testutils with cache enabledSwitch is consistent with the rest of the suite.
tests/streams/other/other_test.go (1)
31-32: Harness migration to testutils LGTM.Switching to testutils.RunSchemaTest and GetTestOptionsWithCache is consistent with the new runner and ensures meta store + cache are wired for these suites.
Also applies to: 85-85, 91-92, 196-197, 202-209, 253-260
tests/streams/primitive_batch_insert_alignment_test.go (1)
33-40: Harness migration LGTM.Running via testutils with cached options matches the new pattern; no functional changes observed in the test body.
tests/streams/query/metadata_test.go (1)
37-50: Harness migration LGTM.Adopting testutils.RunSchemaTest + GetTestOptionsWithCache here is consistent with the updated runner.
tests/streams/truflation_primitive_frozen_test.go (1)
27-43: Harness migration LGTM.Switch to testutils.RunSchemaTest with cached options aligns with the runner and per-function isolation behavior.
tests/streams/common_test.go (1)
44-52: Harness migration LGTM across all changed blocks.All calls now route through the new runner and cached options; good consistency.
Also applies to: 118-126, 178-186, 240-248
tests/streams/digest/digest_actions_test.go (2)
35-65: Harness migration LGTM (digest tests block 1).Options now pass through testutils, which wraps per-function isolation and extension setup.
69-134: Harness migration LGTM (digest tests block 2).Same pattern; no functional changes to assertions.
tests/streams/auth/auth_test.go (9)
45-52: Harness migration LGTM (AUTH01.ValidOwnershipTransfer).
57-63: Harness migration LGTM (AUTH01.InvalidAddressHandling).
178-185: Harness migration LGTM (AUTH02).
299-305: Harness migration LGTM (AUTH02_NestedReadPermissions).
459-467: Harness migration LGTM (AUTH03).
533-540: Harness migration LGTM (AUTH04).
630-636: Harness migration LGTM (AUTH04_NestedComposePermissions).
857-864: Harness migration LGTM (AUTH05_StreamDeletion).
924-931: Harness migration LGTM (FilterStreamsByExistence).tests/streams/utils/runner.go (1)
58-62: Validate “never” schedules and negative durations are accepted by the loaderConfirm that:
- WithMaxBlockAge(-1s) is treated as “disable sync check” (not rejected as invalid).
- Cron "0 0 31 2 *" parses without error and results in no runs.
If not guaranteed, prefer an explicit disable flag or an empty/disabled schedule knob in CacheOptions.Build().
… improved caching This commit modifies several test files to replace instances of `kwilTesting.RunSchemaTest` with `testutils.RunSchemaTest`, ensuring consistent use of the new caching mechanism. The changes enhance test reliability and maintainability by improving cache management during test execution. Key updates include adjustments in `engine_ops_integration_test.go`, `backward_compatibility_test.go`, and `digest_benchmark_test.go`, aligning with the ongoing efforts to streamline the testing framework.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/streams/backward_compatibility_test.go (1)
79-96: Turn prints into assertions to actually detect arg-compat regressionsThe test currently only prints and returns; it will pass even if an argument count mismatch occurs. Fail the subtest on any “argument/arity/count” mismatch and log other expected failures (permission/not found).
Apply:
- // This should not panic or fail due to argument count mismatch - result, err := platform.Engine.Call(engineContext, platform.DB, "", actionName, args, func(row *common.Row) error { - return nil - }) - - // We expect this to fail with a permission error, not an argument count error - if err != nil { - fmt.Printf("Expected error (permission/stream not found): %v\n", err) - return - } - - if result.Error != nil { - fmt.Printf("Expected result error (permission/stream not found): %v\n", result.Error) - return - } - - fmt.Printf("Call to %s with %d args succeeded\n", actionName, len(args)) + // This should not fail due to argument count mismatch + result, err := platform.Engine.Call(engineContext, platform.DB, "", actionName, args, func(row *common.Row) error { return nil }) + if err != nil { + if strings.Contains(err.Error(), "argument") || strings.Contains(err.Error(), "arity") || strings.Contains(err.Error(), "count") { + t.Fatalf("unexpected argument mismatch calling %s with %d args: %v", actionName, len(args), err) + } + t.Logf("Expected non-argument error (e.g., permission/stream not found): %v", err) + return + } + if result.Error != nil { + if strings.Contains(result.Error.Error(), "argument") || strings.Contains(result.Error.Error(), "arity") || strings.Contains(result.Error.Error(), "count") { + t.Fatalf("unexpected argument mismatch calling %s with %d args: %v", actionName, len(args), result.Error) + } + t.Logf("Expected non-argument result error (e.g., permission/stream not found): %v", result.Error) + return + } + t.Logf("Call to %s with %d args succeeded", actionName, len(args))Note: import strings and prefer t.Logf over fmt.Printf (see import fix below).
🧹 Nitpick comments (4)
tests/streams/backward_compatibility_test.go (4)
59-67: Propagate test context instead of context.Background()Use the ctx provided by the harness and mark the helper for better failure locations.
-func testBackwardCompatibilityCall(t *testing.T, platform *kwiltesting.Platform, actionName string, args []any) { +func testBackwardCompatibilityCall(t *testing.T, ctx context.Context, platform *kwiltesting.Platform, actionName string, args []any) { + t.Helper() deployer, err := util.NewEthereumAddressFromBytes(platform.Deployer) @@ txContext := &common.TxContext{ - Ctx: context.Background(), + Ctx: ctx, BlockContext: &common.BlockContext{ Height: 1, },
25-57: Pass ctx through to the call helperForward the harness context so cancellations/timeouts propagate.
- t.Run("get_record_with_5_args", func(t *testing.T) { - testBackwardCompatibilityCall(t, platform, "get_record", []any{ + t.Run("get_record_with_5_args", func(t *testing.T) { + testBackwardCompatibilityCall(t, ctx, platform, "get_record", []any{ defaultDeployer.Address(), "test_stream", nil, // from nil, // to nil, // frozen_at // useCache parameter is omitted - should default to false }) }) @@ - t.Run("get_record_with_6_args", func(t *testing.T) { - testBackwardCompatibilityCall(t, platform, "get_record", []any{ + t.Run("get_record_with_6_args", func(t *testing.T) { + testBackwardCompatibilityCall(t, ctx, platform, "get_record", []any{ defaultDeployer.Address(), "test_stream", nil, // from nil, // to nil, // frozen_at true, // useCache }) })
3-13: Imports: drop fmt, add stringsfmt becomes unused after switching to t.Logf; strings is needed for error checks.
import ( "context" - "fmt" + "strings" "testing" "github.com/trufnetwork/kwil-db/common" kwiltesting "github.com/trufnetwork/kwil-db/testing" "github.com/trufnetwork/node/internal/migrations" testutils "github.com/trufnetwork/node/tests/streams/utils" "github.com/trufnetwork/sdk-go/core/util" )
31-53: Optional: collapse the two t.Run cases into a table-driven loopReduces duplication and makes it easier to add more arg-shape scenarios later.
Example shape:
- name: "5 args (omit useCache)", args: [...5 args...]
- name: "6 args (useCache=true)", args: [...6 args...]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
extensions/tn_cache/internal/engine_ops_integration_test.go(3 hunks)extensions/tn_digest/engine_ops_integration_test.go(2 hunks)internal/benchmark/digest/digest_benchmark_test.go(2 hunks)tests/streams/backward_compatibility_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/benchmark/digest/digest_benchmark_test.go
- extensions/tn_digest/engine_ops_integration_test.go
- extensions/tn_cache/internal/engine_ops_integration_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/streams/backward_compatibility_test.go (3)
tests/streams/utils/runner.go (1)
RunSchemaTest(36-100)internal/migrations/migration.go (1)
GetSeedScriptPaths(13-50)tests/streams/utils/utils.go (1)
GetTestOptionsWithCache(62-75)
⏰ 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). (2)
- GitHub Check: acceptance-test
- GitHub Check: lint
🔇 Additional comments (1)
tests/streams/backward_compatibility_test.go (1)
16-22: Harness swap to cache-enabled runner looks goodSwitching to testutils.RunSchemaTest and GetTestOptionsWithCache() aligns with the new runner: it enforces SetupMetaStore, sets a sane InitialHeight default, resets ordered_sync between isolated function tests, and enables a non-intrusive default cache config. No concerns.
…tCases, and Owner This commit updates the `RunSchemaTest` function in the `runner.go` file to include additional fields: `SeedStatements`, `TestCases`, and `Owner` in the `kwilTesting.SchemaTest` struct. These enhancements aim to improve the flexibility and detail of schema tests, allowing for more comprehensive test configurations and better test management. This change aligns with ongoing efforts to refine the testing framework and improve overall test reliability.
This commit updates the `RunSchemaTest` function in the `runner.go` file to enforce a fatal error when test cases are provided, as they are not currently supported. This change clarifies the limitations of the function and sets the groundwork for potential future support of test cases. The adjustment aims to improve error handling and provide clearer feedback during test execution.
2 participants
Description
initfunction requires us to clean up after every test before seeding again. That's why so many files are touched, to be replaced by our own wrapperRelated Problem
How Has This Been Tested?
Summary by CodeRabbit