Page MenuHomePhabricator

[services] Tunnelbroker - DeliveryBroker tests
ClosedPublic

Authored by max on Apr 7 2022, 5:57 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 11:55 PM
Unknown Object (File)
Wed, Oct 16, 8:37 AM
Unknown Object (File)
Oct 5 2024, 4:35 AM
Unknown Object (File)
Sep 27 2024, 12:54 AM
Unknown Object (File)
Sep 22 2024, 9:29 PM
Unknown Object (File)
Sep 22 2024, 9:29 PM
Unknown Object (File)
Sep 22 2024, 9:29 PM
Unknown Object (File)
Sep 22 2024, 9:29 PM

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
Branch
tunnelbroker-tests-deliverybroker
Lint
No Lint Coverage
Unit
No Test Coverage

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

Does the default message contain expected and actual values?

max added inline comments.
services/tunnelbroker/test/DeliveryBrokerTest.cpp
62–64

Does the default message contain expected and actual values?

Yes, right.

services/tunnelbroker/test/DeliveryBrokerTest.cpp
105

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

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.