Use `MockedClientHandle` in other tests #3241

jvff · 2021-12-16T01:01:43Z

Motivation

While adding a field to the Client type, I realized I would have to add a dummy instance of the field to many mock client instances in tests. Since I had written a MockedClientHandle for some other tests, I wondered if I could use that in the other tests so that I would only need to update one place later.

Solution

I moved the MockedClientHandle to another module to make it easier to find, and also extended it to perform the operations the other tests required. I ended up trying to refactor those tests a bit to try to improve readability.

Review

@teor2345 recently wrote some of the changed tests.

Reviewer Checklist

Code implements Specs and Designs
Tests for Expected Behaviour
Tests for Errors

teor2345

This is a good idea, ~~but we have to be careful that we don't reduce actual peer::Client test coverage.~~

zebra-network/src/peer/client/tests/vectors.rs

zebra-network/src/peer_set/set/tests.rs

zebra-network/src/peer_set/initialize/tests/vectors.rs

zebra-test/src/service_extensions.rs

jvff · 2021-12-16T20:52:09Z

I changed how the initialization is done to use a MockClientBuilder, in order to:

Try to make it clearer that a real Client instance is created
MockedClientHandle only contains the mocked data fed into the Client
Prepare for writing tests for tracking the background tasks. I'll need the builder pattern to be able to configure custom mock background tasks.

teor2345

I'm sorry for the misunderstandings in my last review, I found these changes really confusing.

I have a bunch of suggestions for improving the documentation and names in this PR.

We might also want to improve the corresponding peer::Client and peer::Connection documentation, because it is outdated.

now-or-later/Cargo.toml

Cargo.toml

zebra-network/src/peer/client/tests/vectors.rs

zebra-test/src/service_extensions.rs

zebra-network/src/peer/client/tests.rs

zebra-test/src/service_extensions.rs

I misunderstood the PR

now-or-later/Cargo.toml

teor2345 · 2021-12-17T05:03:50Z

This is a low priority by itself, but #3253 depends on it, so I've marked it as medium.

It's more closely related to a `Client` than the `PeerSet`, and this prepares it to be used by other tests.

Reduce confusion, and clarify that the client is not mocked. Co-authored-by: teor <[email protected]>

Explicitly say how the generated data is returned.

The `Client` service only represents one direction of a connection, so `is_connected` is not the exact term. Co-authored-by: teor <[email protected]>

Move where the conversion from mocked `Client` to mocked `LoadTrackedClient` in order to make the test helper more easily used by other tests.

Replace the boilerplate code to create a fake `Client` instance with usages of the `ClientTestHarness` constructor.

Create a helper type to wrap the result, to make it easier to assert on specific events after trying to receive a request.

Share the `ErrorSlot` between the `Client` and the handle, so that the handle can be used to inspect the contents of the `ErrorSlot`.

Assuming it is initially empty. If it already has an error, the code will panic.

Close the endpoint with the appropriate call to the `close()` method.

Forcefully closes the endpoint.

Also rename the related methods to include `outbound_client_request_receiver` to make it more precise. Co-authored-by: teor <[email protected]>

Allows the `Client` to detect that the channel has been closed.

Avoid negated method names. Co-authored-by: teor <[email protected]>

Checks if a `Service` is not ready to be called.

Reduce repeated code and try to improve readability.

A builder to create test `Client` instances using mock data which can be tracked and manipulated through a `ClientTestHarness`.

Add a `with_version` builder method.

Use the builder to set the peer version, so that the `version` parameter can be removed from the constructor later.

Reduce noise when setting up the harness for tests that don't really care about the remote peer version.

The `with_version` builder method should be used instead.

teor2345

I'm happy with this PR, ~~but I'd like to merge the fixes in PR #3262 soon as well.~~

teor2345

In #3262 (comment) @jvff said:

Doing the assertion last is also not ideal, since some assertions require the client instance to be polled first.

Do these requirements apply to all Clients, or only to ClientTestHarness?

This isn't a blocker, but it would be nice to document these requirements as part of this PR. (They were a bit of a surprise to me.)

teor2345 · 2021-12-21T08:51:03Z

Doing the assertion last is also not ideal, since some assertions require the client instance to be polled first.

Do these requirements apply to all Clients, or only to ClientTestHarness?

This isn't a blocker, but it would be nice to document these requirements as part of this PR. (They were a bit of a surprise to me.)

PR #3273 might help clarify these requirements. If I'm using the Client or test harness wrong, let me know, and I'll fix it.

And document existing tests.

jvff · 2021-12-21T14:04:20Z

Doing the assertion last is also not ideal, since some assertions require the client instance to be polled first.

Do these requirements apply to all Clients, or only to ClientTestHarness?

Sorry I wasn't clear. I meant that doing it last would mean that we wouldn't be asserting on the correct things. We would assert things before client.is_ready().await, and what we actually want is to assert that things changed after the call to client.is_ready().await.

PR #3273 might help clarify these requirements. If I'm using the Client or test harness wrong, let me know, and I'll fix it.

Looks good to me 👍

* Replace NowOrLater with the futures::poll! macro in zebrad * Replace NowOrLater with the futures::poll! macro in zebra-test * Remove the now-or-later crate

…dle-in-other-tests

teor2345

I think we're all done here!

teor2345 · 2021-12-21T20:13:00Z

Note for changelog: this PR includes 3 changes:

Use MockedClientHandle in other tests (#3241)
Add extra client tests for zero and multiple readiness checks (#3273)
Replace NowOrLater with futures::poll! (#3272)

jvff requested a review from teor2345 December 16, 2021 01:01

zfnd-bot bot assigned jvff Dec 16, 2021

jvff force-pushed the use-mocked-client-handle-in-other-tests branch from 412d1e9 to 73cf624 Compare December 16, 2021 01:02

teor2345 previously requested changes Dec 16, 2021

View reviewed changes

jvff force-pushed the use-mocked-client-handle-in-other-tests branch from 73cf624 to 56dc0ba Compare December 16, 2021 13:42

jvff requested a review from teor2345 December 16, 2021 13:43

jvff force-pushed the use-mocked-client-handle-in-other-tests branch from 56dc0ba to 8ef552f Compare December 16, 2021 20:50

teor2345 reviewed Dec 16, 2021

View reviewed changes

teor2345 reviewed Dec 17, 2021

View reviewed changes

now-or-later/Cargo.toml Outdated Show resolved Hide resolved

jvff mentioned this pull request Dec 17, 2021

Keep track of background peer tasks #3253

Merged

3 tasks

teor2345 added A-network Area: Network protocol updates or fixes C-cleanup Category: This is a cleanup C-testing Category: These are tests P-Medium labels Dec 17, 2021

jvff and others added 13 commits December 18, 2021 15:49

Move MockedClientHandle to peer module

0e2572a

It's more closely related to a `Client` than the `PeerSet`, and this prepares it to be used by other tests.

Rename MockedClientHandle to ClientTestHarness

b3c2ce7

Reduce confusion, and clarify that the client is not mocked. Co-authored-by: teor <[email protected]>

Add clarification to mock_peers documentation

40601c5

Explicitly say how the generated data is returned.

Rename method to wants_connection_heartbeats

1ab120a

The `Client` service only represents one direction of a connection, so `is_connected` is not the exact term. Co-authored-by: teor <[email protected]>

Mock Client instead of LoadTrackedClient

09f099f

Move where the conversion from mocked `Client` to mocked `LoadTrackedClient` in order to make the test helper more easily used by other tests.

Use ClientTestHarness in initialize tests

12511e3

Replace the boilerplate code to create a fake `Client` instance with usages of the `ClientTestHarness` constructor.

Allow receiving requests from Client instance

d2edd5e

Create a helper type to wrap the result, to make it easier to assert on specific events after trying to receive a request.

Allow inspecting the current error in the slot

7d53167

Share the `ErrorSlot` between the `Client` and the handle, so that the handle can be used to inspect the contents of the `ErrorSlot`.

Allow placing an error into the ErrorSlot

fa7dbcb

Assuming it is initially empty. If it already has an error, the code will panic.

Allow gracefully closing the request receiver

6e20aad

Close the endpoint with the appropriate call to the `close()` method.

Allow dropping the request receiver endpoint

4663808

Forcefully closes the endpoint.

Rename field to client_request_receiver

0cd8b71

Also rename the related methods to include `outbound_client_request_receiver` to make it more precise. Co-authored-by: teor <[email protected]>

Allow dropping the heartbeat shutdown receiver

b9bdad4

Allows the `Client` to detect that the channel has been closed.

jvff and others added 8 commits December 18, 2021 22:05

Rename method to is_failed

178c13c

Avoid negated method names. Co-authored-by: teor <[email protected]>

Add a IsReady::is_pending extension method

163b50b

Checks if a `Service` is not ready to be called.

Use ClientTestHarness in Client test vectors

d6623db

Reduce repeated code and try to improve readability.

Create a new ClientTestHarnessBuilder type

ca17067

A builder to create test `Client` instances using mock data which can be tracked and manipulated through a `ClientTestHarness`.

Allow configuring the Client's mocked version

edf6b85

Add a `with_version` builder method.

Use ClientTestHarnessBuilder in PeerVersions

68764f3

Use the builder to set the peer version, so that the `version` parameter can be removed from the constructor later.

Use a default mock version where possible

fea2247

Reduce noise when setting up the harness for tests that don't really care about the remote peer version.

Remove Version parameter from the build method

057d162

The `with_version` builder method should be used instead.

jvff force-pushed the use-mocked-client-handle-in-other-tests branch from c174644 to 057d162 Compare December 18, 2021 22:17

Fix some typos and outdated info in the release checklist

956f0f0

This was referenced Dec 20, 2021

Fix capacity exhaustion risks in zebra_test::IsReady #3261

Closed

Take ownership in IsReady, to avoid capacity exhaustion #3262

Closed

teor2345 previously approved these changes Dec 20, 2021

View reviewed changes

teor2345 reviewed Dec 21, 2021

View reviewed changes

This was referenced Dec 21, 2021

Replace NowOrLater with futures::poll! #3272

Merged

Add peer::Client tests with zero and multiple readiness checks #3273

Merged

oxarbitrage mentioned this pull request Dec 21, 2021

Add PeerSet readiness and request future cancel-safety tests #3252

Merged

9 tasks

Add extra client tests for zero and multiple readiness checks (#3273)

ad838f6

And document existing tests.

oxarbitrage dismissed teor2345’s stale review via ad838f6 December 21, 2021 13:48

Merge branch 'main' into use-mocked-client-handle-in-other-tests

6e33f34

teor2345 and others added 4 commits December 21, 2021 11:04

Replace NowOrLater with futures::poll! (#3272)

1d363bb

* Replace NowOrLater with the futures::poll! macro in zebrad * Replace NowOrLater with the futures::poll! macro in zebra-test * Remove the now-or-later crate

remove unused imports

1b1e251

rustfmt

c430475

Merge remote-tracking branch 'origin/main' into use-mocked-client-han…

de2eb7b

…dle-in-other-tests

teor2345 approved these changes Dec 21, 2021

View reviewed changes

teor2345 merged commit b718332 into main Dec 21, 2021

teor2345 deleted the use-mocked-client-handle-in-other-tests branch December 21, 2021 20:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `MockedClientHandle` in other tests #3241

Use `MockedClientHandle` in other tests #3241

jvff commented Dec 16, 2021

teor2345 left a comment •

edited

Loading

jvff commented Dec 16, 2021

teor2345 left a comment

teor2345 commented Dec 17, 2021

teor2345 left a comment •

edited

Loading

teor2345 left a comment •

edited

Loading

teor2345 commented Dec 21, 2021

jvff commented Dec 21, 2021

teor2345 left a comment

teor2345 commented Dec 21, 2021

Use MockedClientHandle in other tests #3241

Use MockedClientHandle in other tests #3241

Conversation

jvff commented Dec 16, 2021

Motivation

Solution

Review

Reviewer Checklist

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

jvff commented Dec 16, 2021

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Dec 17, 2021

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 left a comment • edited Loading

Choose a reason for hiding this comment

teor2345 commented Dec 21, 2021

jvff commented Dec 21, 2021

teor2345 left a comment

Choose a reason for hiding this comment

teor2345 commented Dec 21, 2021

Use `MockedClientHandle` in other tests #3241

Use `MockedClientHandle` in other tests #3241

teor2345 left a comment •

edited

Loading

teor2345 left a comment •

edited

Loading

teor2345 left a comment •

edited

Loading