Database (DynamoDB) item tests initialization.
Related linear task: ENG-686
Details
Details
Run yarn test-tunnelbroker-service all tests are run and succeed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- tunnelbroker-tests-database
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I think this diff is too big, could you split this? You could add each test in a new diff.
Comment Actions
Would this really be easier to review if it was multiple diffs? I don't feel strongly
Comment Actions
I think @karol-bisztyga is right here. Maybe it's too big and messy, I'll split it by the entities.
| services/tunnelbroker/test/DatabaseManagerTest.cpp | ||
|---|---|---|
| 1–10 ↗ | (On Diff #11272) | It would be cleaner to introduce imports in diffs that require them |
Comment Actions
@geekbrother I'm not sure why you've added me as a blocking reviewer after I've accepted this diff. The result of this action is that the diff reappeared in my queue. Do you need a deeper review, or was the intention just to allow this diff to be landed? If the second is correct, then there's no need to have someone formally assigned as a blocking reviewer - it's only useful when you want a diff to be reviewed by someone else when it is already accepted.
Comment Actions
@palys-swm you were not a blocking reviewer. I've updated it, but I thought it will not add the already reviewed diff to your queue as the only blocking-non blocking changed. Now, will know that phabricator will re-add it.
Comment Actions
Ok, that makes sense. But overall, there's no need to explicitly add me as a blocking reviewer. If I accept your diff, you can land it or I'll add someone else as a blocking reviewer. It looks like the main use case for a blocking review is when someone reviewed and accepted your diff, but you want someone else to also take a look.
At this point we don't have any rule on Phabricator that would forbid landing a diff without blocking reviewers.
Obviously, if someone requested changes, we should wait with landing util that reviewer also accepted.
Comment Actions
It looks like the main use case for a blocking review is when someone reviewed and accepted your diff, but you want someone else to also take a look.
Unrelated to this diff, but I do think the "blocking reviewer" Phabricator feature is something we could use more on the team. Two examples:
- It can indicate who is the "primary" reviewer if you had a long list of potential reviewers but want to avoid the "tragedy of the commons" problem where everybody sees other reviewers and so they feel less responsibility to do the review themselves.
- In a situation where you need somebody in particular to accept a diff, it can be useful. We have a rule that a diff can't be landed unless it's approved by one of four people, and additionally some diffs need to be reviewed by me.
| services/tunnelbroker/test/DatabaseManagerTest.cpp | ||
|---|---|---|
| 1–10 ↗ | (On Diff #11272) | agree |
| services/tunnelbroker/test/DatabaseManagerTest.cpp | ||
|---|---|---|
| 1–10 ↗ | (On Diff #11272) |
Agree with you guys too, just missed it while splitting it. Thanks. |
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:
(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?)