Skip to content

test(rivetkit): remove wasm sqlite-only fixtures and coverage#4642

Open
NathanFlurry wants to merge 1 commit intochore/remove-sqlite-vfs-packagesfrom
test/remove-wasm-sqlite-fixtures
Open

test(rivetkit): remove wasm sqlite-only fixtures and coverage#4642
NathanFlurry wants to merge 1 commit intochore/remove-sqlite-vfs-packagesfrom
test/remove-wasm-sqlite-fixtures

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented Apr 13, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link
Copy Markdown

claude bot commented Apr 13, 2026

PR #4642 Review: test(rivetkit): remove wasm sqlite-only fixtures and coverage

Overview

This PR removes WASM SQLite-specific test fixtures and coverage from the driver test suite. The core change is a strategic architectural decision: the RivetKit SQLite runtime is now native-only (via @rivetkit/rivetkit-native), so all WASM VFS test infrastructure is being cleaned up.

Net change: +18 / −1,210 lines — a substantial reduction in test surface that previously covered WASM SQLite paths.


What's Removed

Artifact Why Removed
cross-backend-vfs.ts test suite Tested WASM↔native VFS data compatibility; no longer relevant
actor-db-kv-stats.ts test suite Instrumented WASM KV-VFS operations (getBatch/putBatch stats)
db-kv-stats.ts fixture actor Custom WASM KV-VFS instrumentation provider
sleepWsRawDbAfterClose fixture Simulated poisoned KV transport via raw IDatabase handle
KV channel reconnect stress tests Used /.test/kv-channel/force-disconnect endpoint
disconnectKvChannelForCurrentConfig cleanup Was wiring native SQLite KV channel per test runtime
setNativeSqliteConfig driver call Configured native KV channel endpoint per test
Max SQLite preload size limit doc entry WASM-era preload limit no longer applies

Feedback

Positive:

  • The cleanups are consistent and complete. All three layers (fixtures, test suite, driver orchestration) are updated together, avoiding orphaned dead code.
  • CLAUDE.md, rivetkit-typescript/CLAUDE.md, and ACTOR_KV_STRUCTURE.md are all updated in the same change to reflect the new native-only stance — good documentation hygiene.
  • Removing the disconnectKvChannelForCurrentConfig cleanup path from createTestRuntime is correct now that there is no KV channel to manage per test instance.
  • The comment update in actor-db-stress.ts (KV channel resiliencenative database behavior) accurately reflects what the remaining tests actually cover.

Concerns / Questions:

  1. Test coverage gap for native SQLite behavior. The removed actor-db-kv-stats.ts tests verified precise write patterns (e.g., "warm UPDATE uses BATCH_ATOMIC: exactly 1 putBatch", "no WAL or SHM operations"). These were valuable invariant checks for the storage layer. If @rivetkit/rivetkit-native surfaces analogous metrics or inspection APIs, equivalent native-layer tests should be added. Is that planned, or are these guarantees now verified at a lower level (Rust unit tests, etc.)?

  2. sleepWsRawDbAfterClose removal without a native equivalent. The "poisoned KV produces disk I/O error on commit" test was specifically checking that the error propagated correctly when the underlying transport failed mid-transaction. This is a real production failure mode. Has the failure path been validated against the native runtime, or is it implicitly covered elsewhere?

  3. Cross-backend VFS removal. The cross-backend-vfs.ts tests verified that data written by one backend (WASM or native) was readable by the other. Since the PR's stated position is that WASM SQLite will not be reintroduced, these tests are indeed obsolete — but it's worth confirming there are no migration scenarios (e.g., existing actors previously persisted via WASM VFS that need to be readable by the native runtime on upgrade).

  4. Minor: utils.ts reformats a createTestInlineClientDriver call (splits args across lines) with no semantic change. This is a cosmetic diff that slightly obscures the meaningful changes — consider reverting or noting it's intentional.

  5. Max SQLite preload size doc removal — the limit row is dropped from limits.mdx. If the native runtime still has a configurable per-actor SQLite preload limit (even if the name changes), it should be re-documented under the new name. If the limit is gone entirely, this removal is correct as-is.

No correctness issues found. The changes are internally consistent and all references to removed symbols have been cleaned up.


Reviewed by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant