Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common crate, expose more tests in wasm #1366

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Dec 3, 2024

This PR adds a common crate & some utilities for testing WebAssembly. It mostly moves things around and should not effect functionality other than a few minor bug fixes for tests which previously did not run in web assembly

the bug fixes:

  • fixed is_ready in register_identity to toggle once the identity signature has been verified with backend, fixing SequenceId errors
  • fixed some webassembly tests hanging because of sleep implementation, replaced with yield
  • fixed Notify/Delivery test utilities panicking streams in webassembly environment because of std::time, replaced with xmtp_common::time

Common Crate:

  • creates common crate that houses:
    • the rand_xx fns that were duplicated across a few crates
    • web assembly time:: shims, that are now accessible via xmtp_common::time. It should be a drop-in replacement for std::time, and preferable to use instead of std::time
    • xmtp_common::yield_ which delegates to tokio or the browsers executor depending on environment. Should be preferred over tokio::task::yield, since a WebAssembly future may not necessarily be running in a tokio executor
    • retry moved from xmtp_mls to common. Retry is general enough that this makes sense, and allows it to be used without including all of xmtp_mls.
    • traced_test moved from xmtp_mls to common. Also general enough to be used w/o mls, and useful elsewhere
    • xmtp_common includes a common logger initializer toggling on arch, xmtp_common::logger
  • assert_x and optify macros moved to common
  • new test utilities in store/mod.rs create triggers to track intent metadata (created/published/deleted) and exposed for tests on DbConnection
  • replaced some uses of cfg_attr/cfg_attr pair, with the more succinct wasm_bindgen_test(unsupported = tokio::test) macro

Copy link
Contributor Author

insipx commented Dec 3, 2024

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

@insipx insipx force-pushed the 11-30-common_crate_for_wasm_shims branch from c33b5ba to cf10919 Compare December 4, 2024 19:41
@insipx insipx force-pushed the 11-30-common_crate_for_wasm_shims branch from cf10919 to f8d41b5 Compare December 6, 2024 19:45
@insipx insipx force-pushed the 11-30-common_crate_for_wasm_shims branch 10 times, most recently from 3df89c2 to 919f767 Compare December 11, 2024 16:25
@insipx insipx force-pushed the 11-30-common_crate_for_wasm_shims branch 5 times, most recently from 718950b to 9b9da95 Compare December 11, 2024 23:12
storage::consent_record::{ConsentState, ConsentType},
utils::test::wait_for_min_intents,
};
use xmtp_cryptography::utils::generate_local_wallet;
use xmtp_id::InboxOwner;

#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
#[wasm_bindgen_test(unsupported = tokio::test(flavor = "multi_thread", worker_threads = 1))]
#[cfg_attr(target_family = "wasm", ignore)]
Copy link
Contributor Author

@insipx insipx Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests (and all of device sync on web) is blocked on xmtp/xmtp-message-history-server#6, then the ignore can be removed

@insipx insipx force-pushed the 11-30-common_crate_for_wasm_shims branch from 9b9da95 to 0b8d1fe Compare December 11, 2024 23:19
@insipx insipx force-pushed the 11-30-common_crate_for_wasm_shims branch from 0b8d1fe to fd98665 Compare December 11, 2024 23:31
@insipx insipx changed the title common crate for wasm shims common crate for wasm, expose more tests in wasm Dec 11, 2024
@insipx insipx changed the title common crate for wasm, expose more tests in wasm common crate, expose more tests in wasm Dec 11, 2024
@insipx insipx marked this pull request as ready for review December 11, 2024 23:40
@insipx insipx requested a review from a team as a code owner December 11, 2024 23:40
use super::*;
use diesel::{connection::LoadConnection, deserialize::FromSqlRow, sql_query, RunQueryDsl};
impl DbConnection {
/// Create a new table and register triggers for tracking column updates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a cool idea, nice 😎

Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome problem solving on the browser executor for streaming. and clean up in this pr is fantastic thanks! 🥇

Comment on lines +31 to +35
pub fn rand_array<const N: usize>() -> [u8; N] {
let mut buffer = [0u8; N];
crypto_utils::rng().fill_bytes(&mut buffer);
buffer
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's cool. I hadn't thought about using a generic like that before.

Fut: Future<Output = T>,
T: std::fmt::Debug + PartialEq,
{
let result = crate::time::timeout(crate::time::Duration::from_secs(60), async {
Copy link
Contributor

@codabrink codabrink Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we reduce the default timeout to something like 2-3 seconds? Otherwise tests will take a long time to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can reduce it a bit, but reducing too much causes tests in webassembly to flake out. I figure the common case of test success it will still finish quickly, but we need to give liberal time for wasm. I'll lower it to ~20sec?

Copy link
Contributor

@codabrink codabrink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making this. Huge QoL improvement.

@insipx insipx merged commit 600a8e2 into main Dec 12, 2024
16 checks passed
@insipx insipx deleted the 11-30-common_crate_for_wasm_shims branch December 12, 2024 16:38
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