[services] Tunnelbroker - Add test for `removeMessageItemsByIDsForDeviceID` database function
ClosedPublic
Actions

Authored by • max on Jul 7 2022, 8:32 AM.

Details

Reviewers

• karol
tomek

Commits

rCOMMeec927757d3d: [services] Tunnelbroker - Add test for `removeMessageItemsByIDsForDeviceID`…

Summary

This diff introduces a test for removeMessageItemsByIDsForDeviceID database function which was introduced in D4476.
The function removes messages by the list of IDs and receiver deviceID in a batch.

Linear task: ENG-1361

Test Plan

Tests not fails when running yarn test-tunnelbroker-service

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Jul 7 2022, 8:32 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptJul 7 2022, 8:32 AM

• max mentioned this in D4476: [services] Tunnelbroker - Database function that removes messages by a batch using messageIDs and receiver DeviceID..Jul 7 2022, 8:32 AM

• max edited the summary of this revision. (Show Details)Jul 7 2022, 8:36 AM

• max added parent revisions: D4372: [services] Tunnelbroker - Add DYNAMODB_MAX_BATCH_ITEMS to Constants, D4436: [services] Tunnelbroker - Add exponential backoff related constants, D4373: [services] Inner function to write to DynamoDB by batch, D4473: [services] Tunnelbroker - Switching to use composite key in a messages table in Terraform, D4475: [services] Tunnelbroker - Switching to use composite key in a messageItem entity, D4476: [services] Tunnelbroker - Database function that removes messages by a batch using messageIDs and receiver DeviceID..

• max published this revision for review.Jul 7 2022, 8:38 AM

• max added reviewers: • karol, tomek.

• max mentioned this in D4221: [services] Tunnelbroker - Add test for the RemoveMessageItemsOlderThenCheckpoint database method.Jul 7 2022, 8:46 AM

Harbormaster completed remote builds in B10377: Diff 14285.Jul 7 2022, 8:57 AM

tomek accepted this revision.Jul 8 2022, 2:51 AM

tomek added inline comments.

services/tunnelbroker/test/DatabaseManagerTest.cpp
318–339 ↗	(On Diff #14285)	We should have a test that verifies that not all the messages are deleted - we should put a message with the same deviceID that isn't deleted and another message with a different deviceID

This revision is now accepted and ready to land.Jul 8 2022, 2:51 AM

• karol requested changes to this revision.Jul 11 2022, 3:16 AM

• karol added inline comments.

services/tunnelbroker/test/DatabaseManagerTest.cpp
305–321 ↗	(On Diff #14285)	Wouldn't it be better to first check if the table is available and only then create the items?
309–316 ↗	(On Diff #14285)	Shouldn't we add a constant with a value of `256`?
320 ↗	(On Diff #14285)	Does this compile? Am I missing something or `item` is not defined in the scope here?

This revision now requires changes to proceed.Jul 11 2022, 3:16 AM

• max planned changes to this revision.Jul 12 2022, 3:29 PM

• max mentioned this in rCOMM5bd8a63d6277: [services] Tunnelbroker - Database function that removes messages by a batch….Jul 25 2022, 6:45 AM

Updates based on the comments.

services/tunnelbroker/test/DatabaseManagerTest.cpp
305–321 ↗	(On Diff #14285)	Wouldn't it be better to first check if the table is available and only then create the items? To invocate `.getTableName()` we need to create an `database::MessageItem` instance anyway. We can do this before but need to create an empty instance and we will have an additional instance of the `database::MessageItem`. So it doesn't worth it here.
309–316 ↗	(On Diff #14285)	Shouldn't we add a constant with a value of `256`? Yes, sure! Done.
318–339 ↗	(On Diff #14285)	We should have a test that verifies that not all the messages are deleted - we should put a message with the same deviceID that isn't deleted and another message with a different deviceID Good catch. I've changed the test to insert three messages for the same deviceID, but to delete 2 of 3 messages. Then we check that the one message still persisted and was not deleted after calling the batch removing.
320 ↗	(On Diff #14285)	Does this compile? Am I missing something or `item` is not defined in the scope here? Fixed that nit. Re-checks it, now it compiles without errors. Thanks.