Polkadex-Substrate / polkadexTEE-worker

Polkadex Off-chain Orderbook
Apache License 2.0
10 stars 1 forks source link

Clean up unit tests #305

Open haerdib opened 3 years ago

haerdib commented 3 years ago

Some of our unit tests depend on things to be initialized first, or are not running independently on their own (for exmaple: https://github.com/Polkadex-Substrate/polkadexTEE-worker/blob/develop/worker/src/tests/mod.rs#L32-L41)

To have a reliable code, we should fix these unit tests to run really independent. Probably some trait abstractions introduction will be necessary to achieve this.

We also have #[allow(unused)] tags to remove clippy warnings. But tests are important, so clippy should warn about unused tests (https://github.com/Polkadex-Substrate/polkadexTEE-worker/blob/develop/enclave/src/test_polkadex_balance_storage.rs#L47)

simonsso commented 3 years ago

Hey @ilja-khabarov I’m using ZenHub in GitHub, click this link to join my workspace and see other features available in GitHub or download the ZenHub extension and sign up with your GitHub account. Posted using ZenHub

haerdib commented 3 years ago

A good starting point to clean up could be to fix the gateway_tests I think. It will be no easy task, as abstractions and mocking will be necessary, in a no_std environment.

First things first: everything in the enclave folder is a no_std environment. Cargo test is not working. Hence, for unit testing we're using the following function : https://github.com/Polkadex-Substrate/polkadexTEE-worker/blob/develop/enclave/src/tests.rs#L80 . All tests in this function will be called when ./substratee-worker test --unit is run.

We have two caches implented here: https://github.com/Polkadex-Substrate/polkadexTEE-worker/tree/develop/enclave/src/polkadex_cache (cancel & create order) which are used in the polkadex_gateway. For "unit" testing this gateway, there are some tests defined in this file: https://github.com/Polkadex-Substrate/polkadexTEE-worker/blob/develop/enclave/src/test_polkadex_gateway.rs

Since some functions in the gateway depend on these caches, we are not actually unit testing the gateway, since the caches are always called as well.

So lets treat this as a first task: Abstract the caches inside the gateway such that unit testing can happend without testing the actual caches as well. One possible solution would be to define Cachetrait(s) and create a mock struct implementing these traits and give the struct as an argument to the function. If it makes sense a new gateway struct could be created, such that not every function needs to take the cache struct as an argument.. what ever you think makes the most sense in this case.

Example: 1) The to be tested function takes a struct implementing a specific trait as an argument: https://github.com/integritee-network/worker/blob/d550d82de69a9845e49731d4b7f616562837cc8e/enclave-runtime/src/cert.rs#L190-L194 2) for testing the function, https://github.com/integritee-network/worker/blob/d550d82de69a9845e49731d4b7f616562837cc8e/enclave-runtime/src/test/cert_tests.rs#L39-L41 the mock is given as an argument 3) Mock defined here: https://github.com/integritee-network/worker/blob/d550d82de69a9845e49731d4b7f616562837cc8e/enclave-runtime/src/test/mocks/attestation_ocall_mock.rs which implements the trait EnclaveAttestationOCallApi

I hope that makes somewhat sense

ilja-khabarov commented 3 years ago

Sorry for closing the issue, missclicked. Thanks, @haerdib. Totally makes things clearer.