Details

Reviewers

tomek
varun

Commits

rCOMMd2dfb7c29002: [services] Tests - Add chunk limit to tools

Summary

Depends on D4247

Adding an equivalent to GRPC_CHUNK_SIZE_LIMIT - https://github.com/CommE2E/comm/blob/7e9032d9732db37fadbc2761ccfa3baac5875b2d/services/lib/src/GlobalConstants.h#L20

Test Plan

none really

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Jun 13 2022, 6:50 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJun 13 2022, 6:50 AM

• karol edited the summary of this revision. (Show Details)Jun 13 2022, 7:15 AM

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

• karol added reviewers: tomek, varun.

• karol added a parent revision: D4247: [services] Tests - Add add_attachments.

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

• karol added a child revision: D4249: [services] Tests - Update packages.Jun 13 2022, 7:21 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 13 2022, 10:02 AM

Harbormaster failed remote builds in B9724: Diff 13469!

CI failed on an unrelated stage - key server
CI services build - no space left on device

Harbormaster completed remote builds in B9724: Diff 13469.Jun 14 2022, 3:33 AM

varun requested changes to this revision.Jun 15 2022, 8:40 AM

varun added inline comments.

services/commtest/tests/lib/tools.rs
28 ↗	(On Diff #13469)	can we add a unit to this variable name? idk what size means in this context. also, just curious, why usize instead of u64?

This revision now requires changes to proceed.Jun 15 2022, 8:40 AM

varun added inline comments.Jun 15 2022, 8:42 AM

services/commtest/tests/lib/tools.rs
28 ↗	(On Diff #13469)	nvm i understand why you chose usize

Seems like you answered your own question. I'm not sure what is the state of this diff so I'm going to pass this back to you.

tomek requested changes to this revision.Jun 20 2022, 4:29 AM

tomek added inline comments.

services/commtest/tests/lib/tools.rs
28 ↗	(On Diff #13469)	Please add a unit to the name. It seems like this is in bytes. Or maybe, we can use `ByteSize`?

This revision now requires changes to proceed.Jun 20 2022, 4:29 AM

add unit to the name

Harbormaster completed remote builds in B9917: Diff 13711.Jun 23 2022, 6:06 AM

tomek accepted this revision.Jun 23 2022, 6:41 AM

tomek added inline comments.

services/commtest/tests/lib/tools.rs
21 ↗	(On Diff #13711)	It is no longer a dead code, is it?
24–27 ↗	(On Diff #13711)	I guess we can use a variable instead of a function because the value doesn't change

varun accepted this revision.Jun 26 2022, 9:24 PM

This revision is now accepted and ready to land.Jun 26 2022, 9:24 PM

rebase

Harbormaster failed remote builds in B10047: Diff 13887!Jun 28 2022, 5:25 AM

• karol added inline comments.Jun 28 2022, 5:34 AM

services/commtest/tests/lib/tools.rs
21 ↗	(On Diff #13711)	we can remove this
24–27 ↗	(On Diff #13711)	I wrapped this into a function because I had problems with having it as a variable. `const` will not work because of the `as_u64` which is done in the runtime.