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

feat: add multi test utils #29

Merged
merged 19 commits into from
Jul 29, 2022
Merged

feat: add multi test utils #29

merged 19 commits into from
Jul 29, 2022

Conversation

dadamu
Copy link
Contributor

@dadamu dadamu commented Jul 27, 2022

This PR implements DesmosApp for contract integration tests.

@codecov
Copy link

codecov bot commented Jul 27, 2022

Codecov Report

Merging #29 (6b4d54c) into main (168ddc3) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   90.94%   91.03%   +0.08%     
==========================================
  Files          27       27              
  Lines        2276     2276              
==========================================
+ Hits         2070     2072       +2     
+ Misses        206      204       -2     
Impacted Files Coverage Δ
packages/bindings-test/src/chain_communication.rs 0.00% <ø> (ø)
packages/bindings/src/mocks.rs 98.14% <ø> (ø)
packages/bindings/src/subspaces/mocks.rs 100.00% <ø> (ø)
packages/bindings/src/subspaces/querier.rs 91.70% <ø> (ø)
packages/bindings/src/msg.rs 81.25% <0.00%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32f7d1b...6b4d54c. Read the comment docs.

@dadamu dadamu marked this pull request as ready for review July 28, 2022 11:19

/// Represents the implementation of [`Module`](cw_multi_test::Module) for handling the desmos execution and query messages.
#[derive(Default)]
pub struct DesmosKeeper {}
Copy link
Contributor Author

@dadamu dadamu Jul 28, 2022

Choose a reason for hiding this comment

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

Instead of implementing the whole state inside desmos chain, I make DesmosKeeper as a module which always returns the successful response with proper events. Devs can handle event in the reply phase as the sample.

}

/// Represents failing desmos app always returning error.
pub type FailingDesmosApp = BasicApp<DesmosMsg, DesmosQuery>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for testing the reply function if the sub msg is failed.

@leobragaz
Copy link
Contributor

Looks good to me! Maybe use the // DONTCOVER notation in those file that can be excluded from the coverage.
This will help to raise it a little bit matching the real coverage of the bindings themselves

@@ -179,3 +179,7 @@ jobs:
- name: Build feature (mocks)
working-directory: ./packages/bindings
run: cargo build --no-default-features --features mocks

Choose a reason for hiding this comment

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

This test can be removed

@@ -33,4 +35,5 @@ reactions = []
query = []
msg = []
mocks = ["query"]
integration-test = ["query", "msg", "mocks"]

Choose a reason for hiding this comment

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

This feature can be removed

Suggested change
integration-test = ["query", "msg", "mocks"]

@@ -24,4 +24,7 @@ pub mod relationships;
pub mod reports;
#[cfg(feature = "subspaces")]
pub mod subspaces;
#[cfg(feature = "integration-test")]

Choose a reason for hiding this comment

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

Instead of using a custom feature is better to use the #[cfg(not(target_arch = "wasm32"))] so that the testing_utils module will be automaticaly compiled when performing the integration tests.

Suggested change
#[cfg(feature = "integration-test")]
#[cfg(not(target_arch = "wasm32"))]

Copy link
Contributor Author

@dadamu dadamu Jul 28, 2022

Choose a reason for hiding this comment

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

You are right. I didn't think about it. Since it is a lib that mock desmos app for integration test, I use mocks as its feature.

packages/bindings/src/test_utils.rs Show resolved Hide resolved
packages/bindings/src/test_utils.rs Show resolved Hide resolved
cosmwasm-std = { version = "1.0.0" }
cw-multi-test = { version = "0.13.4", optional = true }
Copy link
Contributor Author

@dadamu dadamu Jul 28, 2022

Choose a reason for hiding this comment

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

Not sure why it has conflict with test-contract cosmwasm-std, so I make it optional and only use it when the feature is mocks.

Choose a reason for hiding this comment

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

I tried to remove the optional flag and the dependency requirement with the mocks feature and on my computer running cargo build everiting compiles correctly.
Can you tell me the command that triggers the error for you?

I'm using:

  • cargo 1.61.0 (a028ae4 2022-04-29)
  • rustc 1.61.0 (fe5b13d68 2022-05-18)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error happens while compileing the test contract with command cargo optimize.
It shows error as follows:

   Compiling cw-multi-test v0.13.4
error[E0432]: unresolved import `cosmwasm_std::testing`
 --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/cw-multi-test-0.13.4/src/app.rs:5:19
  |
5 | use cosmwasm_std::testing::{mock_env, MockApi, MockStorage};
  |                   ^^^^^^^ could not find `testing` in `cosmwasm_std`

error[E0432]: unresolved import `cosmwasm_std::testing`
  --> /usr/local/cargo/registry/src/github.com-1ecc6299db9ec823/cw-multi-test-0.13.4/src/wasm.rs:24:19
   |
24 | use cosmwasm_std::testing::mock_wasmd_attr;
   |                   ^^^^^^^ could not find `testing` in `cosmwasm_std`

Both cargo and rustup version on my PC are 1.64.0-nightly.

Choose a reason for hiding this comment

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

Ok found the issue, becouse cw-multi-test is marked as dependency inisde the desmos-binding toml it's compiled also when targeting wasm32-unknown-unknown.
cosmwasm-std expose the testing module only when not compiling for wasm32-unknown-unknown causing this compilation error.
Moving the cw-multi-test into the dev-dependencies will fix this issue

@dadamu dadamu requested a review from manu0466 July 28, 2022 16:27
@@ -175,7 +175,3 @@ jobs:
- name: Build feature (msg+reports)
working-directory: ./packages/bindings
run: cargo build --no-default-features --features msg,reports

- name: Build feature (mocks)
Copy link
Contributor Author

@dadamu dadamu Jul 29, 2022

Choose a reason for hiding this comment

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

Remove mocks feature building since it is the mock lib for testing and it occurs error related to cw-multi-test importing.

@dadamu dadamu requested a review from manu0466 July 29, 2022 12:27
@manu0466 manu0466 merged commit 2904dff into main Jul 29, 2022
@manu0466 manu0466 deleted the paul/DCD-57/add-multi-test-utils branch July 29, 2022 13:07
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