Skip to content
This repository has been archived by the owner on Aug 30, 2022. It is now read-only.

PB-570: add the first coordinator::core::Service test #352

Merged
merged 1 commit into from
Mar 30, 2020

Conversation

little-dude
Copy link
Contributor

@little-dude little-dude commented Mar 27, 2020

The main tasks, ie that tasks that implement most of the business
logic are aggregator::Service for the aggregator and
coordinator::Service for the coordinator. This commit implements a
first tests for coordinator::core::Service.

In order to test the service, we need to control its inputs and
outputs

There are three source:

  • an RPC server that receives requests from the network and sends back responses
  • an RPC client that sends requests to the network and receives responses back
  • an REST API that receives requests from the network, and sends back responses

However, two of these are not direct sources of the Service. The
API and RPC server layers are already isolated and run in their own
task. They communicate with the Service via a Handle as
illustrated below:

                                           +-------------+
                                           |     API     |
                                           | +------+    |
    +---------------------------+      +----->Handle|    <---+
    |           Service         |      |   | +------+    |   |
    | +----------+   +--------+ |      |   +-------------+   |
  +--->RPC client|   |Receiver<--------+   +-------------+   |
  | | +----------+   +--------+ |      |   |  RPC server |   |
  | +---------------------------+      |   | +------+    |   |
  |                                    +----->Handle|    <---+
  |                                        | +------+    |   |
  |                                        +-------------+   |
  |                                                          |
  |                   +---------+                            |
  |                   |         |                            |
  +-------------------> Network <----------------------------+
                      |         |
                      +---------+

We already control these handles and we can use them to send requests
to the service. Thus, there is no need to mock the API and RPC server
and our system under test can already be simplified:

                                           +------+
  +---------------------------+      +----->Handle|
  |           Service         |      |     +------+
  | +----------+   +--------+ |      |
+--->RPC client|   |Receiver<--------+
| | +----------+   +--------+ |      |
| +---------------------------+      |     +------+
|                                    +----->Handle|
|                                          +------+
|
|
|                   +---------+
|                   |         |
+-------------------> Network |
                    |         |
                    +---------+

The RPC client is the only remaining piece, and the idea of the PR is
to mock it away so that our system under test is finally fully under
our control:

                                         +------+
+---------------------------+      +----->Handle|
|           Service         |      |     +------+
| +----------+   +--------+ |      |
| |  Mock    |   |Receiver<--------+
| +----------+   +--------+ |      |
+---------------------------+      |     +------+
                                   +----->Handle|
                                         +------+

For the mocking itself, we use the mockall crate. The idea is to
leverage rust's conditional compilation to replace the rpc::Client
by our mock when running cargo test. For this, we use the
#[cfg(test)] and #[cfg(not(test))] attributes.

The mock itself doesn't expose the exact same API than the
rpc::Client: the types in some of the function signatures are
slightly simpler. That's works well in Rust because most APIs are
designed around traits.

For instance the real rpc::Client::select method looks like this:

pub fn select(
    &mut self,
    ctx: Context,
    credentials: Credentials
) -> impl Future<Output = Result<Result<(), ()>>> + '_

Thus, the return type is _some type that implements
Future<Output=io::Result<Result<(), ()>>>, and an anonymous
lifetime (the '_ part). But in our mock, the return type is an
actual concrete type:

fn select(&mut self, ctx: Context, credentials: Credentials) -> future::Ready<io::Result<Result<(), ()>>>;

It works, because that concrete type satisfies the
Future<Output = Result<Result<(), ()>>> + '_
trait bound.

In the coordinator binary, we use a random selector. In order to keep
full control of the selection, we defined a very simple selector
MaxSelector that selects all the waiting clients. For more
sophisticated tests, we could define other Selector implementors.

These tests are require quite a lot of boilerplate code, so it would
be nice to keep them out of src/ in their own tests/
directory. Unfortunately, this doesn't work well with the RPC client
mock, which must be compiled with the rest of the crate.

Therefore we created a tests/ directory inside src/. tests/lib/
contains code that can easily be re-used as well as mocks. When
compiling the tests, some module may use symbols from
tests::lib. For instance in aggregator/rpc.rs we have:

use crate::tests::lib::rpc::aggregator::Client;

@little-dude little-dude changed the title add the first coordinator::core::Service test PB-570: add the first coordinator::core::Service test Mar 27, 2020
@little-dude little-dude force-pushed the PB-570-coordinator-service-test-infra branch from 0ae47be to 8474d94 Compare March 27, 2020 09:16
@little-dude little-dude force-pushed the PB-570-coordinator-service-test-infra branch from 8474d94 to 31ddc22 Compare March 28, 2020 14:20
The main tasks, _ie_ that tasks that implement most of the business
logic are `aggregator::Service` for the aggregator and
`coordinator::Service` for the coordinator. This commit implements a
first tests for `coordinator::core::Service`.

In order to test the service, we need to control its inputs and
outputs

There are three source:

- an RPC server that receives requests from the network and sends back responses
- an RPC client that sends requests to the network and receives responses back
- an REST API that receives requests from the network, and sends back responses

However, two of these are not _direct_ sources of the `Service`. The
API and RPC server layers are already isolated and run in their own
task. They communicate with the `Service` via a `Handle` as
illustrated below:

```
                                           +-------------+
                                           |     API     |
                                           | +------+    |
    +---------------------------+      +----->Handle|    <---+
    |           Service         |      |   | +------+    |   |
    | +----------+   +--------+ |      |   +-------------+   |
  +--->RPC client|   |Receiver<--------+   +-------------+   |
  | | +----------+   +--------+ |      |   |  RPC server |   |
  | +---------------------------+      |   | +------+    |   |
  |                                    +----->Handle|    <---+
  |                                        | +------+    |   |
  |                                        +-------------+   |
  |                                                          |
  |                   +---------+                            |
  |                   |         |                            |
  +-------------------> Network <----------------------------+
                      |         |
                      +---------+
```

We already control these handles and we can use them to send requests
to the service. Thus, there is no need to mock the API and RPC server
and our system under test can already be simplified:

```
                                           +------+
  +---------------------------+      +----->Handle|
  |           Service         |      |     +------+
  | +----------+   +--------+ |      |
+--->RPC client|   |Receiver<--------+
| | +----------+   +--------+ |      |
| +---------------------------+      |     +------+
|                                    +----->Handle|
|                                          +------+
|
|
|                   +---------+
|                   |         |
+-------------------> Network |
                    |         |
                    +---------+
```

The RPC client is the only remaining piece, and the idea of the PR is
to mock it away so that our system under test is finally fully under
our control:

```
                                         +------+
+---------------------------+      +----->Handle|
|           Service         |      |     +------+
| +----------+   +--------+ |      |
| |  Mock    |   |Receiver<--------+
| +----------+   +--------+ |      |
+---------------------------+      |     +------+
                                   +----->Handle|
                                         +------+
```

For the mocking itself, we use the `mockall` crate. The idea is to
leverage rust's conditional compilation to replace the `rpc::Client`
by our mock when running `cargo test`. For this, we use the
`#[cfg(test)]` and `#[cfg(not(test))]` attributes.

The mock itself doesn't expose the exact same API than the
`rpc::Client`: the types in some of the function signatures are
slightly simpler. That's works well in Rust because most APIs are
designed around traits.

For instance the real `rpc::Client::select` method looks like this:

```
pub fn select(
    &mut self,
    ctx: Context,
    credentials: Credentials
) -> impl Future<Output = Result<Result<(), ()>>> + '_
```

Thus, the return type is _some type that implements
`Future<Output=io::Result<Result<(), ()>>>`, and an anonymous
lifetime (the `'_` part). But in our mock, the return type is an
actual concrete type:

```
fn select(&mut self, ctx: Context, credentials: Credentials) -> future::Ready<io::Result<Result<(), ()>>>;
```

It works, because that concrete type satisfies the
`Future<Output = Result<Result<(), ()>>> + '_`
trait bound.

In the coordinator binary, we use a random selector. In order to keep
full control of the selection, we defined a very simple selector
`MaxSelector` that selects all the waiting clients. For more
sophisticated tests, we could define other `Selector` implementors.

These tests are require quite a lot of boilerplate code, so it would
be nice to keep them out of `src/` in their own `tests/`
directory. Unfortunately, this doesn't work well with the RPC client
mock, which must be compiled with the rest of the crate.

Therefore we created a `tests/` directory inside `src/`. `tests/lib/`
contains code that can easily be re-used as well as mocks. When
compiling the tests, some module may `use` symbols from
`tests::lib`. For instance in `aggregator/rpc.rs` we have:

```rust
use crate::tests::lib::rpc::aggregator::Client;
```
@little-dude little-dude force-pushed the PB-570-coordinator-service-test-infra branch from 31ddc22 to 5441e47 Compare March 28, 2020 14:24
@little-dude little-dude merged commit 1b8bcfc into master Mar 30, 2020
@little-dude little-dude deleted the PB-570-coordinator-service-test-infra branch March 30, 2020 12:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants