Depends on D4376
Adding the code for adding attachments to log/backup.
Details
Details
- Reviewers
tomek varun - Commits
- rCOMM36491757ed9a: [services] Tests - Add add_attachments
This change is eventually tested on the end of this stack when the logic for the backup test is added
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
CI failed on an unrelated stage - key server
CI services build - no space left on device
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 11 ↗ | (On Diff #13468) | Can we remove it? |
| 14–15 ↗ | (On Diff #13468) | Please be consistent in your comments. Is it logIndex or log_index? |
| 23–31 ↗ | (On Diff #13468) | We should merge these. The current approach makes it harder to maintain the consistency. |
| 36 ↗ | (On Diff #13468) | Why do we need to do so much here (take a reference, convert to_string, take a slice)? Please find a way to simplify it. |
| 47 ↗ | (On Diff #13468) | Use shorthand |
Comment Actions
address feedback
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 11 ↗ | (On Diff #13468) | yes, sorry |
| 14–15 ↗ | (On Diff #13468) | log_index, I'll fix this |
| 23–31 ↗ | (On Diff #13468) | totally, thx |
| 36 ↗ | (On Diff #13468) | this function requires str slice and we have a char. This is just one way we can do this that allocates memory on the heap. We can also do this by using https://doc.rust-lang.org/std/primitive.char.html#method.encode_utf8 I honestly do not see much of a difference between these two approaches, the code that uses encode_utf8 looks like this: let mut buff = [0; 1];
ATTACHMENT_DELIMITER.encode_utf8(&mut buff);
let attachments: String = match log_index {
Some(log_index) => backup_data.log_items[log_index]
.attachments_holders
.join(ATTACHMENT_DELIMITER.encode_utf8(&mut buff)),
None => backup_data
.backup_item
.attachments_holders
.join(ATTACHMENT_DELIMITER.encode_utf8(&mut buff)),
}; |
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 36 ↗ | (On Diff #13468) | What do you think about changing ATTACHMENT_DELIMITER to be a string slice then? |
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 44–51 ↗ | (On Diff #13710) | shouldn't we check what is returned here? |
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 44–51 ↗ | (On Diff #13710) | bumping this q |
| services/commtest/tests/lib/tools.rs | ||
|---|---|---|
| 21 ↗ | (On Diff #13886) | It isn't a dead code |
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 44–51 ↗ | (On Diff #13710) | I'm wondering if add_attachments returns anything noteworthy in the Ok variant that we should inspect. If the return type of that function is Result<(), SomeError> we can just remove the semicolon on this line and remove the Ok(())` on line 52 |
| services/commtest/tests/backup/add_attachments.rs | ||
|---|---|---|
| 44–51 ↗ | (On Diff #13710) | It returns tonic::Response, so I'd leave this code as is. |
| services/commtest/tests/lib/tools.rs | ||
| 21 ↗ | (On Diff #13886) | For some reason Rust perceives it as a dead code, that's why I added this line to suppress the fake warnings: warning: constant is never used: `ATTACHMENT_DELIMITER` --> tests/./lib/tools.rs:36:1 | 36 | pub const ATTACHMENT_DELIMITER: &str = ";"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(dead_code)]` on by default warning: constant is never used: `ATTACHMENT_DELIMITER` --> tests/./blob/../lib/tools.rs:36:1 | 36 | pub const ATTACHMENT_DELIMITER: &str = ";"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ warning: `commtest` (test "tunnelbroker_test") generated 1 warning warning: constant is never used: `ATTACHMENT_DELIMITER` --> tests/./backup/../lib/tools.rs:36:1 | 36 | pub const ATTACHMENT_DELIMITER: &str = ";"; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(dead_code)]` on by default The number of lines are different as I tested this on the top of the stack. |
| services/commtest/tests/lib/tools.rs | ||
|---|---|---|
| 21 ↗ | (On Diff #13886) | I arc patched this diff and there's no warning when i remove the `#[allow(dead_code)] |
| services/commtest/tests/lib/tools.rs | ||
|---|---|---|
| 21 ↗ | (On Diff #13886) | what command did you invoke to check this? |
| services/commtest/tests/lib/tools.rs | ||
|---|---|---|
| 21 ↗ | (On Diff #13886) | cargo build |
| services/commtest/tests/lib/tools.rs | ||
|---|---|---|
| 21 ↗ | (On Diff #13886) | Yeah, cargo build doesn't even touch this. Try to edit any test .rs file and make a syntax error like this: async fn blob_test() -> Result<(), Error> {blah blah blah
let port = env::var("COMM_SERVICES_PORT_BLOB")Now, cargo build doesn't see this. You have to run cargo test to see the error. |