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

Update multisig example doc test #1198

Merged
merged 1 commit into from Mar 24, 2022
Merged

Conversation

HCastano
Copy link
Contributor

We've made some changes with how cross-contract call building works since the Multisig
example was last updated. The existing doc tests haven't kept up.

Part of the reason is that they're not run. This is because the examples are cdylibs.
See the error below when I try and run cargo test --doc:

warning: doc tests are not supported for crate type(s) `cdylib` in package `multisig`

Maybe in the future we should find some way to run the doc tests in our examples, maybe
by having cargo-contract remove the crate-type line or something.

Closes #1197.

@codecov-commenter
Copy link

codecov-commenter commented Mar 24, 2022

Codecov Report

Merging #1198 (723fd6e) into master (3d15fef) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1198      +/-   ##
==========================================
- Coverage   79.03%   78.98%   -0.05%     
==========================================
  Files         229      229              
  Lines        8667     8667              
==========================================
- Hits         6850     6846       -4     
- Misses       1817     1821       +4     
Impacted Files Coverage Δ
...ates/storage/src/collections/hashmap/fuzz_tests.rs 95.74% <0.00%> (-4.26%) ⬇️

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 3d15fef...723fd6e. Read the comment docs.

@paritytech-cicd-pr
Copy link

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 1.1.0-2b7b50f and comparing them to ink! master:

Δ Optimized Size Δ Used Gas Total Optimized Size Total Used Gas
accumulator 1.03 K
adder 2.17 K
contract-introspection 2.37 K
contract-terminate 0.94 K 214_418
contract-transfer 8.15 K 14_418
delegate-calls 2.99 K 14_950
delegator 6.37 K 46_295
dns 8.84 K 43_254
erc1155 17.23 K 86_508
erc20 8.49 K 43_254
erc721 11.81 K 115_344
flipper 1.31 K 14_418
forward-calls 2.90 K 29_536
incrementer 1.21 K 14_418
multisig 25.20 K 93_527
rand-extension 3.92 K 14_418
subber 2.19 K
trait-erc20 8.76 K 43_254
trait-flipper 1.00 K 14_418
trait-incrementer 1.19 K 28_836
upgradeable-flipper 1.55 K

Link to the run | Last update: Thu Mar 24 03:19:26 CET 2022

@@ -308,40 +308,54 @@ mod multisig {
///
/// Since this message must be send by the wallet itself it has to be build as a
/// `Transaction` and dispatched through `submit_transaction` and `invoke_transaction`:
/// ```no_run
/// use ink_env::{DefaultEnvironment as Env, AccountId, call::{CallParams, Selector}, test::CallData};
/// ```should_panic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the should_panic needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't actually fire() a call from the examples, so it'll panic (but it's fine, we just need it to compile successfully)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah got it. Can you create a follow-up issue for actually running the doc tests in CI?

@HCastano HCastano merged commit 4ac378b into master Mar 24, 2022
@HCastano HCastano deleted the hc-fix-multisig-doc-tests branch March 24, 2022 17:05
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.

Broken Multisig Example
5 participants