polkadot icon indicating copy to clipboard operation
polkadot copied to clipboard

Mock builder: usage example for the auction's Leaser type

Open lemunozm opened this issue 2 years ago • 7 comments

This PR shows some simplifications/improvements of using mock-builder for mocking/testing pallets.

@kianenigma @gavofyork @shawntabrizi @bkchr It's a response to a Polkadot forum thread. There's no intention to be merged.

lemunozm avatar Jun 21 '23 06:06 lemunozm

The CI pipeline was cancelled due to failure one of the required jobs. Job name: test-linux-stable Logs: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/3038219

paritytech-cicd-pr avatar Jun 21 '23 06:06 paritytech-cicd-pr

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/mock-builder-a-utility-to-create-mock-pallets/2955/9

Polkadot-Forum avatar Jun 21 '23 07:06 Polkadot-Forum

@shawntabrizi @gavofyork @xlc @bkchr any comments on the above? Would you think this would be a value adding thing for the Substarte codebase itself?

mustermeiszer avatar Jun 28 '23 11:06 mustermeiszer

Looking at this it looks nice. However, what I don't get is why this is "pallet centric" when it is only about implementing a trait? Or are there ideas to mock other things which are not a trait?

bkchr avatar Jul 10 '23 07:07 bkchr

Hi @bkchr, the idea to be "pallet centric" is to be able to reuse the lifetime/storage of a pallet. Benefits:

  1. When instantiating the mock closure, that closure must be allocated somewhere to be called later, having it as a pallet gives you a transparent API to not deal with how and where to save the closure (compare it with mockall when used with static methods. You need to handle a context per mocked method by yourself, if you have mocked 7 methods, you need to keep along the code 7 different ctx, which is quite undesirable and boilerplate). It also allows us to avoid concurrent issues that already happen in mockall when 2 tests use the same mock method at the same time.

  2. Having it as a pallet, you can have different mocks for the same trait at the same time without collisions, which in some cases, is very useful. i.e. My pallet_foo uses two different oracles as pallet_oracle::<Instance1> and pallet_oracle::<Instance2>, and I want to mock both at the same time.

  3. You can configure the types used by the mock using the Config trait, which makes the mock configuration very easy and powerful.

lemunozm avatar Jul 10 '23 08:07 lemunozm

  1. When instantiating the mock closure, that closure must be allocated somewhere to be called later, having it as a pallet gives you a transparent API to not deal with how and where to save the closure (compare it with mockall when used with static methods. You need to handle a context per mocked method by yourself, if you have mocked 7 methods, you need to keep along the code 7 different ctx, which is quite undesirable and boilerplate). It also allows us to avoid concurrent issues that already happen in mockall when 2 tests use the same mock method at the same time.

This is basically just a thread local storage that you could guard with some kind of thread local variable to clean the storage when the test is finished. You already need some way to store the closures and then "invalidate" them when the test is finished?

2. Having it as a pallet, you can have different mocks for the same trait at the same time without collisions, which in some cases, is very useful. i.e. My pallet_foo uses two different oracles as pallet_oracle::<Instance1> and pallet_oracle::<Instance2>, and I want to mock both at the same time.

As for instances, you could just pass some random generic to your type implementing the trait to make it "instantiable".

3. You can configure the types used by the mock using the Config trait, which makes the mock configuration very easy and powerful.

This could even be combined with 2 to make this generic implement the special Config trait.

I still only see a mocking infrastructure for traits, that is not really related to pallets. It actually only makes it more complicated to have it part of the entire FRAME machinery.

bkchr avatar Jul 10 '23 20:07 bkchr

Could this be implemented using thread_local instead of creating a pallet? Maybe, but using a pallet allows us:

  1. The lifetime of the mock is the lifetime of the runtime where the pallet is added. If using thread_local directly, you need to reset the state manually. More boilerplate and error prune if you forget to do it each test case.
  2. If running tests in parallel, the user does not need to do anything like create mutex or similar to protect the storage. It’s done automatically because pallets are added to different runtimes, and the storage is duplicated.
  3. A new user already knows how to use pallets, so this way of mocking is familiar to them. They do not need to learn how to interact with thread_local, and LocalKey types for doing mocks by themselves.

You already need some way to store the closures and then "invalidate" them when the test is finished?

No, this is done automatically, thanks to the pallet infrastructure

As for instances, you could just pass some random generic to your type implementing the trait to make it "instantiable”.

You can specify different types with this, but if having 2 instances at the same time, how do you know which mock closure you need to call from a static method of the trait? If possible, sure, it’s not easy and requires much effort for the tester to make it work. Using pallets, you have a solution out of the box for this, simply adding two mock pallets to the runtime.

It actually only makes it more complicated to have it part of the entire FRAME machinery.

In which ways it does it more complicated? From my point of view, adding a mock as if it were a pallet to the runtime is the more straightforward and natural way I found to deal with this:

  1. use the #[mock_builder::pallet] macro to create the pallet_mock
  2. one line to add pallet_mock to the runtime
  3. few lines to add the pallet_mock::Config section

lemunozm avatar Jul 11 '23 06:07 lemunozm