Details

Reviewers

tomek
varun

Commits

Summary

Depends on D4633

Adding a piece of test for "add attachments for logs" functionality.

Test Plan

cd services
yarn run-performance-tests backup 1

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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

• karol added reviewers: tomek, varun.

• karol requested review of this revision.Jul 26 2022, 2:53 AM

tomek requested changes to this revision.Jul 26 2022, 5:32 AM

tomek added inline comments.

services/commtest/tests/backup_performance_test.rs
182 ↗	(On Diff #14926)
184 ↗	(On Diff #14926)	We don't need to iterate the collection when we need only the index
188–201 ↗	(On Diff #14926)	Is this code formatted?

This revision now requires changes to proceed.Jul 26 2022, 5:32 AM

address feedback

varun requested changes to this revision.Jul 26 2022, 10:17 AM

varun added inline comments.

services/commtest/tests/backup_performance_test.rs
188–201 ↗	(On Diff #14964)	i think it makes more sense to check if not empty then do something

This revision now requires changes to proceed.Jul 26 2022, 10:17 AM

services/commtest/tests/backup_performance_test.rs
188–201 ↗	(On Diff #14964)	My thoughts: https://www.notion.so/commapp/Always-exit-early-9df0993fda5e41fd9c6e5e9a2248436d

• karol requested review of this revision.Jul 27 2022, 6:17 AM

• karol added inline comments.

services/commtest/tests/backup_performance_test.rs
188–201 ↗	(On Diff #14964)	Ok, I'm going to re-request the review since we want to go for this early exit.
188–201 ↗	(On Diff #14926)	yes

varun requested changes to this revision.Jul 27 2022, 10:15 AM

varun added inline comments.

services/commtest/tests/backup_performance_test.rs
188–201 ↗	(On Diff #14964)	There's an implicit return at the end of `add_attachments::run()` so I think the "return early" idea is fulfilled either way. I think it's bad style to introduce an explicit return when it's unneeded. Separately, `return ()` is equivalent to `return` and the latter is more readable (no chance of someone confusing `unit` for a function call).

This revision now requires changes to proceed.Jul 27 2022, 10:15 AM

services/commtest/tests/backup_performance_test.rs
188–201 ↗	(On Diff #14964)	Is there a way to do this without increasing indentation? My #1 biggest pet peeve with Rust code is that it tends to be extremely indented, which I think really affects readability

services/commtest/tests/backup_performance_test.rs
188–201 ↗	(On Diff #14964)	Is there a way to do this without increasing indentation? My #1 biggest pet peeve with Rust code is that it tends to be extremely indented, which I think really affects readability I share that gripe, but in this case the indentation doesn't really change. Current: if item_cloned.backup_item.attachments_holders.is_empty() { return (); } add_attachments::run(&mut client_cloned, &item_cloned, None) .await .unwrap(); My suggestion: if !item_cloned.backup_item.attachments_holders.is_empty() { add_attachments::run(&mut client_cloned, &item_cloned, None) .await .unwrap(); }