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

Add unit test for saving wallet with pending blobs #2679

Merged

Conversation

ndr-ds
Copy link
Contributor

@ndr-ds ndr-ds commented Oct 22, 2024

Motivation

CI was failing on PR #2646 with a JSON serialization failure. That was fixed by PR #2668 but we realized none of our tests covered this case.

Proposal

Create a test that tries to save a wallet with pending blobs in it. Verified that before #2668 this test fails, and now it succeeds.

Test Plan

CI

Release Plan

  • Nothing to do / These changes follow the usual release cycle.

Copy link
Contributor Author

ndr-ds commented Oct 22, 2024

@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from 6851b18 to 626545a Compare October 22, 2024 19:02
@ndr-ds ndr-ds requested review from afck, jvff, ma2bd and Twey October 22, 2024 19:07
@ndr-ds ndr-ds force-pushed the 10-22-stop_using_clientoptions_in_clientcontext_directly branch from 7d98ce6 to f727975 Compare October 22, 2024 20:13
@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from 626545a to cd36336 Compare October 22, 2024 20:13
@ndr-ds ndr-ds force-pushed the 10-22-stop_using_clientoptions_in_clientcontext_directly branch from f727975 to b71383e Compare October 23, 2024 00:14
@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from cd36336 to 072e716 Compare October 23, 2024 00:14
@ndr-ds ndr-ds changed the base branch from 10-22-stop_using_clientoptions_in_clientcontext_directly to graphite-base/2679 October 23, 2024 02:27
@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from 072e716 to d985546 Compare October 23, 2024 02:28
@ndr-ds ndr-ds changed the base branch from graphite-base/2679 to main October 23, 2024 02:28
@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from d985546 to 648f7be Compare October 23, 2024 02:28
Copy link
Contributor

@afck afck left a comment

Choose a reason for hiding this comment

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

Thanks!

A much more lightweight alternative would be to just serialize and deserialize a Wallet with blobs in it. (Without actually saving to disk.) But I'm fine with either! 👍

Comment on lines 4 to 6
#[cfg(test)]
mod util;

#[cfg(test)]
mod chain_listener;

#[cfg(test)]
mod wallet;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the whole module already cfg(test)?

Suggested change
#[cfg(test)]
mod util;
#[cfg(test)]
mod chain_listener;
#[cfg(test)]
mod wallet;
mod util;
mod chain_listener;
mod wallet;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 😅 Will fix

Copy link
Contributor Author

ndr-ds commented Oct 23, 2024

Thanks!

A much more lightweight alternative would be to just serialize and deserialize a Wallet with blobs in it. (Without actually saving to disk.) But I'm fine with either! 👍

That's a great point 🤔 I went with it like this because I thought that we might as well test the full flow of saving a wallet with pending blobs, as that wasn't triggered by any other test it seems

@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from 648f7be to f060ff1 Compare October 23, 2024 12:05
@@ -185,6 +185,41 @@ where
}
}

#[cfg(with_testing)]
pub fn new_test(storage: S, wallet: W) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

{create,make,new}_test_client_context ?

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 thought the client_context suffix was redundant, since we're calling it from a ClientContext struct 🤔 but I can change it

@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from f060ff1 to c5e6324 Compare October 23, 2024 12:34
Copy link
Contributor Author

ndr-ds commented Oct 23, 2024

Merge activity

  • Oct 23, 10:34 AM EDT: A user started a stack merge that includes this pull request via Graphite.
  • Oct 23, 10:35 AM EDT: Graphite rebased this pull request as part of a merge.
  • Oct 23, 10:36 AM EDT: A user merged this pull request with Graphite.

@ndr-ds ndr-ds force-pushed the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch from c5e6324 to 3d43078 Compare October 23, 2024 14:35
@ndr-ds ndr-ds merged commit 610985d into main Oct 23, 2024
5 checks passed
@ndr-ds ndr-ds deleted the 10-22-add_unit_test_for_saving_wallet_with_pending_blobs branch October 23, 2024 14:36
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