Skip to content

test(driver): select matrix cells with vitest filters#4906

Closed
NathanFlurry wants to merge 1 commit intodriver-test-complaints/restore-serverful-hibernating-requestsfrom
driver-test-complaints/use-vitest-matrix-filter
Closed

test(driver): select matrix cells with vitest filters#4906
NathanFlurry wants to merge 1 commit intodriver-test-complaints/restore-serverful-hibernating-requestsfrom
driver-test-complaints/use-vitest-matrix-filter

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 May 3, 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
Contributor

claude Bot commented May 3, 2026

PR Review: test(driver): select matrix cells with vitest filters

Overview

This PR removes environment-variable-based matrix selection from the driver test harness and replaces it with always-on Vitest -t filters. The key changes are:

  1. Removes env vars (RIVETKIT_DRIVER_TEST_RUNTIME, RIVETKIT_DRIVER_TEST_SQLITE, RIVETKIT_DRIVER_TEST_ENCODING) entirely from the matrix selection path.
  2. Changes defaults in getDriverMatrixCells: old defaults were runtimes: ["native"] / sqliteBackends: ["local"] (minimal); new defaults are runtimes: ["native", "wasm"] / sqliteBackends: ["local", "remote"] (full matrix).
  3. Always uses the full describe block name runtime (X) / sqlite (Y) / encoding (Z) removes the conditional shorter form encoding (X).
  4. Updates SKILL.md to reflect the new commands (no env vars, simplified -t patterns).

This is a clean simplification that makes the describe block name stable and predictable, which is the right foundation for automated -t selection.


Code Quality

Positive:

  • Removing the env var save/restore boilerplate (restoreMatrixEnv, afterEach) is a clear improvement. Env mutation in tests is fragile.
  • The single code path for describe block naming eliminates a latent foot-gun where forgetting to set env vars would silently run tests under a different name format.
  • SKILL.md updates are thorough and consistent with the code changes.
  • envList helper and hasEnvMatrixOverride are correctly deleted with no dead code left behind.

Potential Issues

1. Default matrix expansion is a silent behavior change for all callers

Old defaults: native/local only (1 cell per encoding). New defaults: native/local, native/remote, wasm/remote (3 cells per encoding).

Every describeDriverMatrix("Suite", callback) call without explicit runtimes/sqliteBackends options now runs 3x more cells. All ~20 test files with bare describeDriverMatrix calls are affected. This is presumably intentional. The old minimal default was compensated by env vars in the skill. But the PR description is blank so the intent is undocumented.

2. native/remote is a newly-default matrix cell

This combination (native runtime + remote SQLite) was not tested by default before (only reachable via env overrides or explicit SQLITE_DRIVER_MATRIX_OPTIONS). It is now included by default for all suites. Worth confirming all test fixtures handle remote SQLite with the native runtime before merging.

3. wasm + local now silently skips instead of failing loudly

Previously, explicitly forcing wasm + local via env vars threw a clear error. Now it silently produces no cells. Since env vars are removed this is much harder to hit accidentally, but a short comment on the skip branch would preserve the intent.

4. Simplified -t pattern drops outer-describe anchoring (no issue, just noting)

The runtime/sqlite/encoding tuple is specific enough within a single file. The removed anchors are fine to drop.


Test Coverage

The replacement test "defaults to both runnable runtime pairs" correctly validates the new default matrix output. The removed "fails fast when env explicitly selects wasm with local SQLite" test was for a code path that no longer exists. Its removal is appropriate.


Summary

The refactor is sound. Main items to verify before merging:

  • The new native/remote default cell passes (or is expected to skip) across the ~20 suites that now include it by default.
  • Add a brief comment on the wasm + local skip branch (optional but helpful).
  • Fill in the PR description / checklist.

@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/use-vitest-matrix-filter branch from 68ad03b to bf25094 Compare May 3, 2026 21:03
@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/wait-wasm-db-sleep-recovery branch from 1899baf to 009afe0 Compare May 3, 2026 21:03
@NathanFlurry NathanFlurry marked this pull request as ready for review May 3, 2026 23:38
@NathanFlurry NathanFlurry changed the base branch from driver-test-complaints/wait-wasm-db-sleep-recovery to graphite-base/4906 May 3, 2026 23:40
@NathanFlurry NathanFlurry force-pushed the graphite-base/4906 branch from 009afe0 to e146d85 Compare May 3, 2026 23:40
@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/use-vitest-matrix-filter branch from bf25094 to 123cbb9 Compare May 3, 2026 23:40
@NathanFlurry NathanFlurry changed the base branch from graphite-base/4906 to driver-test-complaints/restore-serverful-hibernating-requests May 3, 2026 23:40
@NathanFlurry NathanFlurry force-pushed the driver-test-complaints/use-vitest-matrix-filter branch from 123cbb9 to bf25094 Compare May 4, 2026 00:32
@NathanFlurry
Copy link
Copy Markdown
Member Author

Landed in main via stack-merge fast-forward push. Commits are in main; closing to match.

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