test: add integration tests for nft permissions #549
Conversation
|
Task linked: CU-2kbujbr pallet-nft integration tests for basilisk |
|
Crate versions that have been updated:
Runtime version has not been increased. |
Codecov Report
@@ Coverage Diff @@
## master #549 +/- ##
=======================================
Coverage 81.22% 81.22%
=======================================
Files 19 19
Lines 1928 1928
=======================================
Hits 1566 1566
Misses 362 362 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| #[test_case(ClassType::Redeemable, false ; "redeemable class")] | ||
| #[test_case(ClassType::Auction, false ; "auction class")] | ||
| #[test_case(ClassType::HydraHeads, false ; "hydra heads class")] | ||
| fn nft_permissions_should_allow_marketplace_class_creation(class_type: ClassType, is_allowed: bool) { |
There was a problem hiding this comment.
Great job with the tests!
Just one thing as stated in one of my other comments, we could make this even more readable by introducing two consts:
const ALLOW: bool = true;
const NOT_ALLOW: bool = false;
And using these in the test_case attributes. By doing this, we could remove the magic booleans. But as you want, I approve the PR anyway. ;)
| #[test_case(ClassType::Redeemable; "redeemable class")] | ||
| #[test_case(ClassType::Auction; "auction class")] | ||
| #[test_case(ClassType::HydraHeads; "hydra heads class")] | ||
| fn create_class_should_be_without_deposit_when_created_using_trait(class_type: ClassType) { |
There was a problem hiding this comment.
I would like to have tests like this also for the rest of the trait’s functions.
There was a problem hiding this comment.
Ok. But in this case we are not talking about integration tests because we are just testing one implementation of the traits and there is no interaction with other pallets involved. And testing the implementation of it implies that it will also work when integrated into other pallets. So I would add tests for this directly to NFT pallet in warehouse repo. @dmoka what do you think?
There was a problem hiding this comment.
In my opinion this should be in the basilisk repo in the nft integration test (not in the marketplace).
e.g if someone removes classType or changes permissions in the warehouse and updates tests in the warehouse, we may not catch it. It's the same situations as we had with minting without deposits, warehouse repo was updated and merged and we found problem after we tried to use NFT pallet in LM.
In my opinion this is integration test because it test integration of NFT pallet in basilisk chain with basilisk specific requirements.
There was a problem hiding this comment.
The generic core logic that we are testing here is basically this:
But we are actually testing how this pallet_nft is configured/integrated within Basilisk runtime, because we run this:
<basilisk_runtime::Runtime as pallet_nft::Config>::Permissions::...
... with ClassType enum used in the basilisk runtime pallet_nft config implementation
So I think this is fine as an integration test.
There was a problem hiding this comment.
Just reading @martinfridrich's answer, I agree with that, as it is not only related to marketplace, we could put them into a separate integration test file.
There was a problem hiding this comment.
Hmm. Ok. But I would prefer to have tests next to implementations. Right now we use structs and trait implementations from warehouse crate and test them in Basilisk repo. And we will need to do the same in Hydra repo. In my opinion the best solution would be to not use generic names for structs and traits (e.g. CreateTypedClassWithoutPermissionCheck). Then we don't need to be afraid of some changes in warehouse repo. But I'm totally fine with what you agreed on.
|
This tests are not specific to marketplace, I think they should be in separate NFT integration tests |
| ); | ||
| } | ||
|
|
||
| // Nonfungibles traits |
There was a problem hiding this comment.
hm, now when I'm looking at it you were right. This look more like unit tests, tested code is not using permissions at all. I should look at nft implementation first, sorry.
I think they should be in warehosue as you said, but I'm ok if it's stay here.
No description provided.