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

test(wallet-integration): unit tests for pop up contract #374

Open
wants to merge 24 commits into
base: alex/feat-wallet-integration-refactor
Choose a base branch
from

Conversation

al3mart
Copy link
Contributor

@al3mart al3mart commented Dec 11, 2024

This PR covers the addition of unit tests for the wallet integration with pop up contracts.

fn wait_for_signature is tested as part of an integration test in pop-cli/tests/contracts.rs.
The integration test the https server handling the signature of payloads has been included in the test contract_lifecycle in pop-cli/tests/contracts.rs.

Retrieving the correct upload and instantiation call data is tested in pop-cli/src/commands/up/contract.rs in get_upload_call_data_works and get_instantiate_call_data_works tests.

[sc-1887]

@al3mart al3mart changed the base branch from peter/feat-wallet-integration-up-contract to alex/feat-wallet-integration-refactor December 12, 2024 14:31
@al3mart al3mart changed the title chore(wallet-integration): include unit tests for pop up contract test(wallet-integration): include unit tests for pop up contract Dec 12, 2024
@al3mart al3mart changed the title test(wallet-integration): include unit tests for pop up contract test(wallet-integration): unit tests for pop up contract Dec 12, 2024
@@ -620,6 +635,7 @@ mod tests {
}

#[tokio::test]
#[ignore]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was failing on CI and decided to ignore in order to sort the tests included in this PR.

Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 80.79470% with 29 lines in your changes missing coverage. Please review.

Please upload report for BASE (alex/feat-wallet-integration-refactor@a08b18d). Learn more about missing BASE report.

Files with missing lines Patch % Lines
crates/pop-cli/src/commands/up/contract.rs 80.34% 0 Missing and 23 partials ⚠️
crates/pop-contracts/src/up.rs 84.84% 0 Missing and 5 partials ⚠️
crates/pop-cli/src/common/wallet.rs 0.00% 1 Missing ⚠️
@@                           Coverage Diff                            @@
##             alex/feat-wallet-integration-refactor     #374   +/-   ##
========================================================================
  Coverage                                         ?   74.78%           
========================================================================
  Files                                            ?       61           
  Lines                                            ?    13541           
  Branches                                         ?    13541           
========================================================================
  Hits                                             ?    10126           
  Misses                                           ?     2075           
  Partials                                         ?     1340           
Files with missing lines Coverage Δ
crates/pop-cli/src/common/wallet.rs 0.00% <0.00%> (ø)
crates/pop-contracts/src/up.rs 61.25% <84.84%> (ø)
crates/pop-cli/src/commands/up/contract.rs 37.40% <80.34%> (ø)

@al3mart al3mart marked this pull request as ready for review December 13, 2024 22:49
@al3mart al3mart requested a review from peterwht December 13, 2024 22:49
Copy link
Contributor

@peterwht peterwht left a comment

Choose a reason for hiding this comment

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

Nice!!! Looking good. Left a few comments for minor adjustments / thoughts.

Comment on lines +541 to +542
gas_limit: None,
proof_size: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if these were some value to ensure pop-cli isn't just taking the default None.

@@ -46,6 +46,7 @@ mod tests {
// This is a helper test for an actual running pop CLI.
// It can serve as the "frontend" to query the payload, sign it
// and submit back to the CLI.
#[ignore]
#[tokio::test]
async fn sign_call_data() -> anyhow::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

can be removed if its purpose is properly represented elsewhere in the tests.

// This struct implements the [`Payload`] trait and is used to submit
// pre-encoded SCALE call data directly, without the dynamic construction of transactions.
struct CallData(Vec<u8>);
impl Payload for CallData {
Copy link
Contributor

Choose a reason for hiding this comment

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

leaving a note: I copied this from the pop call parachain PR (IIRC). Once everything gets merged in we should hopefully be able to just import this.

/// Transaction payload to be sent to frontend for signing.
#[derive(Serialize, Debug)]
#[cfg_attr(test, derive(Deserialize, Clone))]
pub struct TransactionData {
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason this wasn't just imported?

/// Test the contract lifecycle: new, build, up, call
#[tokio::test]
async fn contract_lifecycle() -> Result<()> {
const DEFAULT_PORT: u16 = 9944;
const DEFAULT_ENDPOINT: &str = "ws://127.0.0.1:9944";
Copy link
Contributor

Choose a reason for hiding this comment

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

would recommend changing this port away from 9944 -- maybe use free_port as well?

/// Test the contract lifecycle: new, build, up, call
#[tokio::test]
async fn contract_lifecycle() -> Result<()> {
const DEFAULT_PORT: u16 = 9944;
const DEFAULT_ENDPOINT: &str = "ws://127.0.0.1:9944";
const WALLET_INT_URI: &str = "http://127.0.0.1:9090";
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but likely need something to componentize things a little more and let us alter the address for the wallet integration. This port 9090 is going to conflict with some other tests.

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.

2 participants