0xPolygonMiden / miden-client

Client library that facilitates interaction with the Miden rollup

MIT License

39 stars 30 forks source link

Fix flaky test #391

Closed igamigo closed 4 months ago

igamigo commented 5 months ago

Looks like test_swap_offchain() is flaky right now. We should review and fix it.

mFragaBA commented 5 months ago

I noticed there are some other instances of flaky tests.

FLAKY 2/3 [  80.819s] miden-client::integration test_p2idr_transfer_consumed_by_target

edit: this might be an unrelated issue, the test was terminated because it took too long:

TRY 1 SLOW [>540.000s] miden-client::integration test_get_account_update
TRY 1 TRMNTG [>720.000s] miden-client::integration test_get_account_update
TRY 1 TMT [ 720.006s] miden-client::integration test_get_account_update

mFragaBA commented 5 months ago

Here are others:

   FLAKY 3/3 [ 110.606s] miden-client::integration onchain_tests::test_onchain_accounts
   FLAKY 2/3 [  89.965s] miden-client::integration onchain_tests::test_onchain_notes_flow
   FLAKY 3/3 [  14.544s] miden-client::integration onchain_tests::test_onchain_notes_sync_with_tag
   FLAKY 2/3 [  88.996s] miden-client::integration test_p2idr_transfer_consumed_by_target
cargo nextest run --release --test=integration --features "integration" --run-ignored ignored-only -- test_import_genesis_accounts_can_be_used_for_transactions
   FLAKY 2/3 [  78.649s] miden-client::integration test_sync_detail_values

NVM: this one was from when we changed back from cargo makefile to plain makefile (when we did the change, the make start-node was re-running the make_genesis from the note, taking time and causing the first run of some tests to timeout.

mFragaBA commented 5 months ago

Looks like test_swap_offchain() is flaky right now. We should review and fix it.

A hypothesis I have for this assertion failure is that the node has another account with the same account id prefix (either from the same test or other tests that are ran simultaneously). This could mean that doing the sync can bring notes that aren't actually directed to this account. If that's the case I see at least 2 approaches:

make sure that functions that create notes on integration tests return the created note ID and stop doing assertions on the get_input_notes count.
we can modify the sync_state function so that we only store public notes that are consumable by any of our accounts (don't know if this is desired behavior though).

the hardest part overall is making sure that is what causes the issue.

igamigo commented 4 months ago

410 was merged (fixing the hypothesis we discussed above) and so far we have not seen another occurrence of this issue. Closing this for now until something comes up.