Skip to content

Conversation

@insipx
Copy link
Contributor

@insipx insipx commented Nov 21, 2025

this is mostly a QoL pr since i noticed the wasm ephemeral db builds completely synchronously. this makes using some test utils more flexible, like making it possible to use proptest for db queries (not used yet)

@insipx
Copy link
Contributor Author

insipx commented Nov 21, 2025

This PR is part of a stack of 1 bookmark:

  1. trunk()
  2. insipx/sync-conn ← this PR

Created with jj-stack

@claude
Copy link

claude bot commented Nov 21, 2025

Claude finished @insipx's task —— View job


Code Review Complete

Review Date: November 25, 2025, 21:04 UTC


Summary

This PR successfully converts the with_connection test harness from async to synchronous execution using FutureExt::now_or_never. The change is well-motivated and safe for the specific use case.

Positive Observations

  1. Correct Usage of now_or_never: The implementation is safe because:

    • For Wasm ephemeral databases, WasmDb::new(&StorageOption::Ephemeral) creates an in-memory connection synchronously (see xmtp_db/src/encrypted_store/database/wasm.rs:123)
    • No await points exist in the ephemeral path - it calls WasmDbConnection::new_ephemeral() which immediately constructs a connection
    • The now_or_never() usage in xmtp_db/src/test_utils.rs:119 is therefore guaranteed to resolve immediately
  2. Improved Test Ergonomics: The synchronous with_connection enables more flexible test patterns, including proptest usage as mentioned in the PR description

  3. Consistent Pattern: All 22 test files have been updated consistently to remove .await calls, making the tests simpler

  4. Preserved Async Path: The with_connection_async function is retained for tests that genuinely need async behavior

Concerns & Recommendations

1. Missing Test Coverage (82% patch coverage)

The codecov report shows 45 lines missing coverage, particularly in:

  • xmtp_proto/src/types/cursor_list.rs (17 lines - 0% coverage)
  • xmtp_db/src/encrypted_store/group_intent/types.rs (12 lines - 42.85% coverage)
  • xmtp_db/src/encrypted_store/group_intent/error.rs (5 lines - 0% coverage)

Recommendation: These appear to be new utility types/errors introduced as part of the refactor. Consider adding tests for the uncovered code paths or document why they're acceptable to leave untested.

2. Potential Footgun for Future Developers

The synchronous with_connection looks like it should work for any database type, but it's actually only safe for ephemeral databases. If someone tries to use it with persistent storage, they'll get a panic at runtime.

Recommendation: Add a comment to xmtp_db/src/test_utils.rs:111 documenting this constraint:

/// Test harness that loads an Ephemeral store.
/// IMPORTANT: Only works with ephemeral databases because persistent databases
/// require async initialization (OPFS setup). Use `with_connection_async` for
/// persistent database tests.
pub fn with_connection<F, R>(fun: F) -> R

3. Dependency Management

The futures crate is added as an optional test-utils dependency (Cargo.toml:47, 98), but it's not clear if this is only for now_or_never or if there are other uses.

Recommendation: Since futures::FutureExt::now_or_never is a small utility, consider whether pulling in the entire futures crate is necessary. However, given that futures is likely already in the dependency tree, this is minor.

Code Quality

  • ✅ Tests are correctly converted from async to sync
  • ✅ No unnecessary changes beyond the stated goal
  • ✅ Follows existing code patterns
  • ✅ The PR reduces complexity (net -270 lines)

Verdict

Approve with minor suggestions. The core change is sound and well-executed. The main concerns are documentation and test coverage, which are low-priority improvements.


@insipx insipx marked this pull request as draft November 21, 2025 00:15
@insipx insipx marked this pull request as ready for review November 21, 2025 00:16
@insipx insipx marked this pull request as draft November 21, 2025 00:18
@codecov
Copy link

codecov bot commented Nov 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.19%. Comparing base (0b5a6fb) to head (0c9c8d2).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2820      +/-   ##
==========================================
- Coverage   74.65%   74.19%   -0.47%     
==========================================
  Files         387      387              
  Lines       50048    49262     -786     
==========================================
- Hits        37362    36548     -814     
- Misses      12686    12714      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@insipx insipx changed the base branch from insipx/sql-queries to main November 24, 2025 21:02
@insipx insipx changed the base branch from main to insipx/dependency-query November 24, 2025 21:03
@insipx insipx marked this pull request as ready for review November 24, 2025 21:04
@macroscopeapp
Copy link
Contributor

macroscopeapp bot commented Nov 24, 2025

Make xmtp_db::test_utils::with_connection synchronous and align Wasm initialization with FutureExt::now_or_never() in test_utils.rs

Convert the test harness and all affected tests to call a synchronous with_connection, removing await usage across the suite and adding futures to enable WasmDb::new(...).now_or_never() in Wasm builds.

📍Where to Start

Start with the with_connection helpers in test_utils.rs, then scan call sites in test modules to confirm synchronous usage and removal of .await.


Macroscope summarized 0c9c8d2.

@insipx insipx force-pushed the insipx/dependency-query branch from a8c6d99 to 8fa0624 Compare November 24, 2025 21:17
@insipx insipx force-pushed the insipx/sync-conn branch 3 times, most recently from c54934e to 859ec84 Compare November 24, 2025 21:29
@insipx insipx force-pushed the insipx/dependency-query branch from 8fa0624 to 157f868 Compare November 24, 2025 21:29
@insipx insipx force-pushed the insipx/dependency-query branch from 157f868 to 03917e5 Compare November 25, 2025 16:38
@insipx insipx force-pushed the insipx/dependency-query branch from 03917e5 to 344089e Compare November 25, 2025 20:49
@insipx insipx changed the base branch from insipx/dependency-query to main November 25, 2025 20:50
@insipx insipx merged commit 1e1bf34 into main Nov 25, 2025
24 checks passed
@insipx insipx deleted the insipx/sync-conn branch November 25, 2025 23:54
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.

3 participants