Page MenuHomePhabricator

[services] Tunnelbroker - DeliveryBroker tests
ClosedPublic

Authored by max on Apr 7 2022, 5:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 18, 10:11 AM
Unknown Object (File)
Sat, Nov 16, 10:21 PM
Unknown Object (File)
Sat, Nov 16, 1:51 AM
Unknown Object (File)
Sat, Nov 16, 1:51 AM
Unknown Object (File)
Sat, Nov 16, 1:51 AM
Unknown Object (File)
Fri, Nov 15, 6:37 PM
Unknown Object (File)
Fri, Nov 15, 9:04 AM
Unknown Object (File)
Fri, Nov 15, 9:04 AM

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

services/tunnelbroker/test/DeliveryBrokerTest.cpp
21 ↗(On Diff #11169)

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
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.

Remove == equality to EQ only.

tomek added inline comments.
services/tunnelbroker/test/DeliveryBrokerTest.cpp
62–64 ↗(On Diff #11588)

Does the default message contain expected and actual values?

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.

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.

This revision is now accepted and ready to land.Apr 19 2022, 7:08 AM
max marked an inline comment as done.

Changed the test names to be more obvious.

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!

max marked an inline comment as done.

Rebase on master.