Details

Reviewers

• karol
tomek
varun

Commits

rCOMM8a4616ee6e07: [services] Tunnelbroker - DeliveryBroker tests

Summary

DeliveryBroker - is an internal class to deliver and queue messages between the gRPC threads and AMQP thread.
This diff is a DeliveryBroker class tests. We performs checks for:

Push/Pop message to/from the deviceID queue,
Check if queue is empty,
Erase the queue for the certain deviceID.

Related linear task: ENG-686

Test Plan

Run yarn test-tunnelbroker-service all tests are run and succeed.

Diff Detail

Repository

rCOMM Comm

Branch

tunnelbroker-tests-deliverybroker

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Apr 7 2022, 5:57 AM

Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 7 2022, 5:57 AM

• max edited the summary of this revision. (Show Details)Apr 7 2022, 5:59 AM

• max added reviewers: • karol, tomek, varun.

• max added a parent revision: D3279: [services] Tunnelbroker - Fix delivering messages one by one and remove mutex..Apr 7 2022, 6:02 AM

Harbormaster completed remote builds in B7965: Diff 11169.Apr 7 2022, 8:26 AM

• max requested review of this revision.Apr 7 2022, 8:26 AM

What exactly do we test here? Is there anything beyond just testing whether folly::ConcurrentHashMap works properly?

services/tunnelbroker/test/DeliveryBrokerTest.cpp
21	we probably shouldn't use randomly generated data for tests.

This revision now requires changes to proceed.Apr 8 2022, 12:24 AM

Added tests with static variables along with generated ones. Switched from TEST_F to TEST, because we don't use fixtures here.

• max retitled this revision from [services] Tunnelbroker - DeliveryBroker tests. to [services] Tunnelbroker - DeliveryBroker tests.Apr 18 2022, 5:14 AM

Harbormaster completed remote builds in B8251: Diff 11542.Apr 18 2022, 5:18 AM

In D3645#100490, @karol-bisztyga wrote:

What exactly do we test here? Is there anything beyond just testing whether folly::ConcurrentHashMap works properly?

We are using a quirky structure here: map[receiver deviceID] → Queue → Message information. DeliveryBroker is a helper to use this structure because on insert and pop we need to operate the map and the queue inside it. It relies on the D3279 (which is handle a realization) in this diff stack.

Verbose output for generated values in case of failure was added.

Harbormaster completed remote builds in B8289: Diff 11585.Apr 19 2022, 12:50 AM

Remove == equality to EQ only.

Harbormaster completed remote builds in B8292: Diff 11588.Apr 19 2022, 1:11 AM

tomek accepted this revision.Apr 19 2022, 2:00 AM

tomek added inline comments.

services/tunnelbroker/test/DeliveryBrokerTest.cpp
62–64 ↗	(On Diff #11588)	Does the default message contain expected and actual values?

• max marked an inline comment as done.Apr 19 2022, 2:05 AM

• max added inline comments.

services/tunnelbroker/test/DeliveryBrokerTest.cpp
62–64 ↗	(On Diff #11588)	Does the default message contain expected and actual values? Yes, right.

tomek added inline comments.Apr 19 2022, 2:06 AM

services/tunnelbroker/test/DeliveryBrokerTest.cpp
105 ↗	(On Diff #11588)	Names of all these tests don't make too much sense for me. `TestOperationsOnErase` - what does it mean? We can try choosing some naming scheme that is meaningful - we can e.g. use what js uses `ShouldBeEmptyAfterErase`, or e.g.`TestIfEmptyAfterErase`. Please update all the test names before landing - feel free to request a review in case of any doubts.

• karol accepted this revision.Apr 19 2022, 7:08 AM

This revision is now accepted and ready to land.Apr 19 2022, 7:08 AM

• max mentioned this in D3279: [services] Tunnelbroker - Fix delivering messages one by one and remove mutex..Apr 20 2022, 7:22 AM

Changed the test names to be more obvious.

Harbormaster completed remote builds in B8379: Diff 11691.Apr 20 2022, 11:39 PM

• max marked an inline comment as done.Apr 20 2022, 11:39 PM

• max added inline comments.

services/tunnelbroker/test/DeliveryBrokerTest.cpp
105 ↗	(On Diff #11588)	Names of all these tests don't make too much sense for me. `TestOperationsOnErase` - what does it mean? We can try choosing some naming scheme that is meaningful - we can e.g. use what js uses `ShouldBeEmptyAfterErase`, or e.g.`TestIfEmptyAfterErase`. That makes sense! Let's stick to the js-like test names like `ShouldBeEmptyAfterErase` and etc. I've changed all the names and will change them on other tests before landing. Please update all the test names before landing - feel free to request a review in case of any doubts. I think that would be enough for test names )) and we should go further and land those. Thanks!