Page MenuHomePhabricator
Differential D3696

[services] Blob - Switch from memcmp to a simple EQ in database tests
ClosedPublic
Actions

Authored by • max on Apr 11 2022, 7:16 AM.
Tags
None
Referenced Files
F3393913: D3696.diff
Sat, Nov 30, 4:25 PM
Unknown Object (File)
Mon, Nov 25, 11:46 AM
Unknown Object (File)
Fri, Nov 15, 8:16 AM
Unknown Object (File)
Sun, Nov 3, 4:58 PM
Unknown Object (File)
Sun, Nov 3, 4:58 PM
Unknown Object (File)
Sun, Nov 3, 4:58 PM
Unknown Object (File)
Sun, Nov 3, 4:58 PM
Unknown Object (File)
Sun, Nov 3, 4:49 PM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
• karol
tomek

Details

Reviewers
• karol
tomek
Commits
rCOMMfa38e5944b38: [services] Blob - Switch from memcmp to a simple EQ in database tests
Summary

@palys-swm revealed a bug related to the use of memcmp in this case:
The test would not fail if the test item is a prefix of the found from database item, because we're comparing only
item.*someItem.size() bytes. We don't need to compare the sizes, because the switch to the simple comparison of strings solves the
problem.

Related diff comment https://phabricator.ashoat.com/D3686#101268

Test Plan

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

Diff Detail

Repository
rCOMM Comm
Branch
blob-remove-tests-memcmp
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Apr 11 2022, 7:16 AM
Herald added subscribers: • abosh, • benschac, • karol and 3 others. · View Herald TranscriptApr 11 2022, 7:16 AM
• max added reviewers: • karol, tomek.Apr 11 2022, 7:17 AM
Harbormaster completed remote builds in B8074: Diff 11293.Apr 11 2022, 7:21 AM
• max requested review of this revision.Apr 11 2022, 7:21 AM
• karol accepted this revision.Apr 12 2022, 1:23 AM
tomek accepted this revision.Apr 12 2022, 3:12 AM

Thanks for fixing it!

This revision is now accepted and ready to land.Apr 12 2022, 3:12 AM
• max updated this revision to Diff 11343.Apr 12 2022, 6:42 AM

Rebase on master.

• max added a comment.Apr 12 2022, 6:44 AM
In D3696#101938, @palys-swm wrote:

Thanks for fixing it!

Thank you for revealing this bug!

This revision was landed with ongoing or failed builds.Apr 12 2022, 6:46 AM
Closed by commit rCOMMfa38e5944b38: [services] Blob - Switch from memcmp to a simple EQ in database tests (authored by • max). · Explain Why
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMMfa38e5944b38: [services] Blob - Switch from memcmp to a simple EQ in database tests.
Harbormaster completed remote builds in B8116: Diff 11343.Apr 12 2022, 6:47 AM

Revision Contents
Changeset List

PathSize
services/
blob/
test/
24 lines

Diff 11343

View Options

services/blob/test/DatabaseManagerTest.cpp

Loading...