Page MenuHomePhabricator
Differential D3686

[services] Tunnelbroker - Database SessionItem tests
ClosedPublic
Actions

Authored by • max on Apr 11 2022, 3:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 18, 9:41 AM
Unknown Object (File)
Sun, Mar 16, 12:40 AM
Unknown Object (File)
Wed, Mar 12, 12:05 PM
Unknown Object (File)
Tue, Mar 11, 2:10 AM
Unknown Object (File)
Tue, Mar 11, 1:58 AM
Unknown Object (File)
Tue, Mar 11, 1:57 AM
Unknown Object (File)
Tue, Mar 11, 1:56 AM
Unknown Object (File)
Tue, Mar 11, 1:56 AM
View All Files
Subscribers
• abosh
• adrian
ashoat
atul
• benschac
• karol
tomek

Details

Reviewers
varun
tomek
• karol
Commits
rCOMM3819740fbf82: [services] Tunnelbroker - Database SessionItem tests
Summary

Database (DynamoDB) item tests. For the SessionItem database entity we perform:

  • Check if the table is available,
  • Create and fulfill database item entity,
  • Put into the database table,
  • Find the record using the primary key,
  • Compare record fields of created item and item from database record,
  • Remove the testing record from the database.

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-database-sessionitem
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

• max created this revision.Apr 11 2022, 3:34 AM
Herald added subscribers: • abosh, • benschac, • karol and 4 others. · View Herald TranscriptApr 11 2022, 3:34 AM
• max retitled this revision from [services] Tunnelbroker - Database SessionItem tests. to [services] Tunnelbroker - Database SessionItem tests.Apr 11 2022, 3:35 AM
• max edited the summary of this revision. (Show Details)
• max added reviewers: varun, tomek, • karol.
• max added a parent revision: D3643: [services] Tunnelbroker - Database item tests initialization.
Harbormaster completed remote builds in B8054: Diff 11273.Apr 11 2022, 3:39 AM
• max requested review of this revision.Apr 11 2022, 3:39 AM
• max mentioned this in D3643: [services] Tunnelbroker - Database item tests initialization.Apr 11 2022, 3:55 AM
tomek requested changes to this revision.Apr 11 2022, 5:01 AM

We also need to clarify our approach to randomness in D3654

services/tunnelbroker/test/DatabaseManagerTest.cpp
44–49

It's always better to use functions provided by the testing framework, as in case of failure, they provide a lot more useful messages. So in this case, if we wanted to compare char* it would be better to use a combination of ASSERT_THAT and ElementsAre matcher... but in this case there's no need for that. We want to compare item.getDeviceID so why would we want to extract bytes from it? Can't we just compare strings?

BTW, your solution would not fail if item.getDeviceID() was a prefix of foundItem->getDeviceID() because we're comparing only item.getDeviceID().size() bytes.

This revision now requires changes to proceed.Apr 11 2022, 5:01 AM
tomek mentioned this in D3687: [services] Tunnelbroker - Database SessionSignatureItem tests.Apr 11 2022, 5:14 AM
• max updated this revision to Diff 11283.Apr 11 2022, 6:19 AM
• max marked an inline comment as done.

Switch from memcmp to a simple EQ.

services/tunnelbroker/test/DatabaseManagerTest.cpp
44–49

It's always better to use functions provided by the testing framework, as in case of failure, they provide a lot more useful messages. So in this case, if we wanted to compare char* it would be better to use a combination of ASSERT_THAT and ElementsAre matcher... but in this case there's no need for that. We want to compare item.getDeviceID so why would we want to extract bytes from it? Can't we just compare strings?

BTW, your solution would not fail if item.getDeviceID() was a prefix of foundItem->getDeviceID() because we're comparing only item.getDeviceID().size() bytes.

This is a good catch! Switched to simple EQ.

• max planned changes to this revision.Apr 11 2022, 6:20 AM

Switching to the Plan changes until we clarify the random/non-random value for tests.

Harbormaster completed remote builds in B8064: Diff 11283.Apr 11 2022, 6:24 AM
• max mentioned this in D3696: [services] Blob - Switch from memcmp to a simple EQ in database tests.Apr 11 2022, 7:16 AM
• max mentioned this in rCOMMfa38e5944b38: [services] Blob - Switch from memcmp to a simple EQ in database tests.Apr 12 2022, 6:46 AM
• max updated this revision to Diff 11547.Apr 18 2022, 6:56 AM

Added tests with static variables along with generated ones following D3654#102087.

Harbormaster completed remote builds in B8256: Diff 11547.Apr 18 2022, 7:01 AM
• max updated this revision to Diff 11583.Apr 19 2022, 12:08 AM

Verbose output for generated values in case of failure was added.

Harbormaster completed remote builds in B8287: Diff 11583.Apr 19 2022, 12:14 AM
• max added 1 blocking reviewer(s): tomek.Apr 19 2022, 2:07 AM
tomek accepted this revision.Apr 19 2022, 2:28 AM
This revision is now accepted and ready to land.Apr 19 2022, 2:28 AM
• max updated this revision to Diff 11841.Apr 25 2022, 4:30 AM

Rebase on master.

Harbormaster completed remote builds in B8495: Diff 11841.Apr 25 2022, 4:35 AM
Closed by commit rCOMM3819740fbf82: [services] Tunnelbroker - Database SessionItem tests (authored by • max). · Explain WhyApr 25 2022, 4:37 AM
This revision was automatically updated to reflect the committed changes.
• max added a commit: rCOMM3819740fbf82: [services] Tunnelbroker - Database SessionItem tests.

Revision Contents
Changeset List

PathSize
services/
tunnelbroker/
test/
58 lines

Diff 11273

View Options

services/tunnelbroker/test/DatabaseManagerTest.cpp

// @flow // @flow
import Icon from '@expo/vector-icons/FontAwesome5.js';
import { useNavigationState } from '@react-navigation/native'; import { useNavigationState } from '@react-navigation/native';
import invariant from 'invariant'; import invariant from 'invariant';
import * as React from 'react'; import * as React from 'react';
import { View } from 'react-native'; import { View, Text } from 'react-native';
import genesis from 'lib/facts/genesis.js'; import genesis from 'lib/facts/genesis.js';
import { threadInfoSelector } from 'lib/selectors/thread-selectors.js'; import { threadInfoSelector } from 'lib/selectors/thread-selectors.js';
import { import {
userInfoSelectorForPotentialMembers, userInfoSelectorForPotentialMembers,
userSearchIndexForPotentialMembers, userSearchIndexForPotentialMembers,
} from 'lib/selectors/user-selectors.js'; } from 'lib/selectors/user-selectors.js';
import { getPotentialMemberItems } from 'lib/shared/search-utils.js'; import { getPotentialMemberItems } from 'lib/shared/search-utils.js';
▲ Show 20 Lines • Show All 189 Lines • ▼ Show 20 Lines if (this.frozen) {
this.pendingListDataWithHeights = listDataWithHeights; this.pendingListDataWithHeights = listDataWithHeights;
} else { } else {
this.setState({ listDataWithHeights }); this.setState({ listDataWithHeights });
} }
}; };
} }
const unboundStyles = { const unboundStyles = {
pinnedCountBanner: {
backgroundColor: 'panelForeground',
height: 30,
flexDirection: 'row',
textAlign: 'center',
justifyContent: 'center',
alignItems: 'center',
},
pinnedCountText: {
color: 'panelBackgroundLabel',
marginRight: 5,
},
container: { container: {
backgroundColor: 'listBackground', backgroundColor: 'listBackground',
flex: 1, flex: 1,
}, },
threadContent: { threadContent: {
flex: 1, flex: 1,
}, },
hiddenThreadContent: { hiddenThreadContent: {
▲ Show 20 Lines • Show All 124 Lines • ▼ Show 20 Lines ) {
const styles = useStyles(unboundStyles); const styles = useStyles(unboundStyles);
const overlayContext = React.useContext(OverlayContext); const overlayContext = React.useContext(OverlayContext);
const measureMessages = useHeightMeasurer(); const measureMessages = useHeightMeasurer();
const genesisThreadInfo = useSelector( const genesisThreadInfo = useSelector(
state => threadInfoSelector(state)[genesis.id], state => threadInfoSelector(state)[genesis.id],
); );
const bannerText = React.useMemo(() => {
if (!threadInfo.pinnedCount || threadInfo.pinnedCount === 0) {
return '';
}
const messageNoun = threadInfo.pinnedCount === 1 ? 'message' : 'messages';
return `${threadInfo.pinnedCount} pinned ${messageNoun}`;
}, [threadInfo.pinnedCount]);
atulUnsubmitted
Not Done
Inline Actions

You don't need to use React.useMemo(...) here.

The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).

On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.

See below:

875403.png (620×810 px, 104 KB)

(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)

atul: You don't need to use `React.useMemo(...)` here. The `useMemo()` hook is helpful for…
const pinnedCountBanner = React.useMemo(() => {
if (!bannerText) {
return null;
}
return (
<View style={styles.pinnedCountBanner}>
<Text style={styles.pinnedCountText}>{bannerText}</Text>
<Icon
name="chevron-right"
size={12}
color={colors.panelBackgroundLabel}
/>
</View>
);
}, [
bannerText,
styles.pinnedCountBanner,
styles.pinnedCountText,
colors.panelBackgroundLabel,
]);
return ( return (
<MessageListContextProvider threadInfo={threadInfo}> <MessageListContextProvider threadInfo={threadInfo}>
{pinnedCountBanner}
<MessageListContainer <MessageListContainer
{...props} {...props}
usernameInputText={usernameInputText} usernameInputText={usernameInputText}
updateUsernameInput={updateUsernameInput} updateUsernameInput={updateUsernameInput}
userInfoInputArray={userInfoInputArray} userInfoInputArray={userInfoInputArray}
updateTagInput={updateTagInput} updateTagInput={updateTagInput}
resolveToUser={resolveToUser} resolveToUser={resolveToUser}
otherUserInfos={otherUserInfos} otherUserInfos={otherUserInfos}
Show All 14 Lines