[service] Tunnelbroker - Fix calling of `removeMessageItem` in DatabaseManager test
ClosedPublic
Actions

Authored by • max on Aug 4 2022, 10:22 AM.

Details

Reviewers

• karol
tomek

Commits

rCOMM0b5ab05eb70d: [service] Tunnelbroker - Fix calling of `removeMessageItem` in DatabaseManager…

Summary

This diff is a fix.
One occurrence of removeMessageItem(...) was missed to update due to the changes introduced in D4694. This diff fixes DatabaseManager tests to run without errors. At the moment DatabaseManager unit tests building fails with an error:

/transferred/test/DatabaseManagerTest.cpp: In member function 'virtual void DatabaseManagerTest_RemoveMessageItemsInBatch_Test::TestBody()':
/transferred/test/DatabaseManagerTest.cpp:387:45: error: no matching function for call to 'comm::network::database::DatabaseManager::removeMessageItem(std::string)'
  387 |       messageThirdToNotRemove.getMessageID());

Related linear task: ENG-1491

Test Plan

Built and run unit tests for DatabaseManager in test/DatabaseManagerTest.cpp.
Tests were successfully built and run without errors.

Diff Detail

Repository

rCOMM Comm

Branch

fix-database-manager-test

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Aug 4 2022, 10:22 AM

• max held this revision as a draft.

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptAug 4 2022, 10:22 AM

Harbormaster completed remote builds in B11127: Diff 15320.Aug 4 2022, 10:36 AM

• max published this revision for review.Aug 4 2022, 10:36 AM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added reviewers: • karol, tomek.

• max edited the summary of this revision. (Show Details)

This is concerning. The test plan in D4694 consists of running the tests. Now we've discovered that it introduced a compilation error. Was it a failure of our CI? Was the test run before landing?

At the moment DatabaseManager unit tests building fails with an error

So why CI haven't warned us about it? But even if the CI failed / tests weren't run by it, we should never land without testing our code!

This revision is now accepted and ready to land.Aug 5 2022, 5:17 AM

In D4739#136818, @tomek wrote:

This is concerning. The test plan in D4694 consists of running the tests. Now we've discovered that it introduced a compilation error. Was it a failure of our CI? Was the test run before landing?

At the moment DatabaseManager unit tests building fails with an error

So why CI haven't warned us about it? But even if the CI failed / tests weren't run by it, we should never land without testing our code!

Our CI doesn't run unit tests right now. It's tracked by the ENG-1468. The problem was that the local branch for the unit tests was out of sync with the remote. We should run unit tests in our CI and that will leverage such problems in the future for tests. I've made a few fixes in follow-up diffs regarding the unit tests and running them in CI. After we accept and land them we can use our CI to run the unit tests and check such problems before they can occur. Thanks for a fast review @tomek !