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: pallet api fungibles #278

Merged
merged 26 commits into from
Sep 13, 2024
Merged

Conversation

Daanvdplas
Copy link
Collaborator

@Daanvdplas Daanvdplas commented Sep 11, 2024

  • Renamed helper functions to pallet_assets_... like in pop-api/integrations-tests
  • Variable names used throughout the test
  • small test for encoding of ReadResult
  • Error cases tested for calling the pallet assets dispatchables (two missing because not sure how to without spending too much time on it).
  • ensure_signed test coverage
  • read state functions refactored and more extensive checks
  • weight output tested

@Daanvdplas Daanvdplas changed the base branch from main to daan/test-devnet_runtime_versioning September 11, 2024 11:58
@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 99.69880% with 1 line in your changes missing coverage. Please review.

Project coverage is 51.03%. Comparing base (4b9da05) to head (4e7fbaa).

Files with missing lines Patch % Lines
pallets/api/src/fungibles/mod.rs 93.75% 1 Missing ⚠️
@@             Coverage Diff              @@
##           daan/api     #278      +/-   ##
============================================
+ Coverage     48.89%   51.03%   +2.13%     
============================================
  Files            47       47              
  Lines          4532     4701     +169     
  Branches       4532     4701     +169     
============================================
+ Hits           2216     2399     +183     
  Misses         2254     2254              
+ Partials         62       48      -14     
Files with missing lines Coverage Δ
pallets/api/src/fungibles/tests.rs 100.00% <100.00%> (ø)
pallets/api/src/mock.rs 100.00% <ø> (ø)
runtime/devnet/src/config/api/versioning.rs 90.60% <100.00%> (ø)
pallets/api/src/fungibles/mod.rs 92.89% <93.75%> (+7.41%) ⬆️

Copy link
Collaborator

@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.

Great coverage on this! Left a few small improvement comments.

The main reason we have this pallet in-between contracts <> pallet-assets, is so we can control if anything changes. So, I think testing the integrity of our errors, dispatchables, and parameters is crucial to ensure further upgrades don't break anything

So, my recommendation is this:

  • Please create a test for errors in pallets-assets. This test will simply assert_eq!() each error to its encoded index. For example assert_eq!(Error::BalanceLow::encode(), 0) (the encode() bit will need tweaking). May also be useful to validate the total variants in an enum
  • Please create a test to verify codec indexes of Read. Once again, simple assert_eq!() from each entry to its index. So, assert Read::TotalSupply = index 0
  • Please assert every dispatchable call_index to numbers, in our pallet and pallet-assets.

If we want to do this after we merge pop-api into main, that is fine. But these tests should hopefully be pretty easy, and are crucial to ensuring our API remains stable.

pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@evilrobot-01 evilrobot-01 left a comment

Choose a reason for hiding this comment

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

Not sure if this was supposed to be reviewed.

pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Show resolved Hide resolved
Copy link
Collaborator

@chungquantin chungquantin left a comment

Choose a reason for hiding this comment

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

LGTM!

Base automatically changed from daan/test-devnet_runtime_versioning to daan/api September 13, 2024 16:44
primitives/src/lib.rs Outdated Show resolved Hide resolved
// Approves a value to spend that is equal to the current allowance.
assert_eq!(
Fungibles::approve(signed(owner), token, spender, value / 2),
Ok(Some(WeightInfo::approve(0, 0)).into())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See here for test that the weight is properly returned

@Daanvdplas Daanvdplas merged commit a81c8b4 into daan/api Sep 13, 2024
10 checks passed
@Daanvdplas Daanvdplas deleted the daan/test-pallet_api_fungibles branch September 13, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants