Page MenuHomePhabricator

[services] Tests - Add create new backup
ClosedPublic

Authored by karol on May 31 2022, 6:50 AM.
Tags
None
Referenced Files
F3506638: D4165.id13605.diff
Fri, Dec 20, 5:36 PM
F3506467: D4165.id13395.diff
Fri, Dec 20, 4:43 PM
Unknown Object (File)
Fri, Dec 20, 12:16 AM
Unknown Object (File)
Wed, Dec 18, 10:15 PM
Unknown Object (File)
Wed, Dec 18, 10:02 PM
Unknown Object (File)
Wed, Dec 18, 9:48 PM
Unknown Object (File)
Wed, Dec 18, 8:39 PM
Unknown Object (File)
Wed, Dec 18, 6:53 PM

Details

Summary

Depends on D4225

Adding a script for creating a new backup.

Test Plan

This, for now, doesn't do anything.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 6:56 AM
Harbormaster failed remote builds in B9544: Diff 13238!
karol edited the test plan for this revision. (Show Details)
karol added reviewers: ashoat, tomek, varun.
Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 7:23 AM
Harbormaster failed remote builds in B9551: Diff 13245!
Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 7:27 AM
Harbormaster failed remote builds in B9556: Diff 13250!

I know that Ci failed but it's related to docker which I know is not working properly right now.

services/commtest/src/create_new_backup.rs
14–15 ↗(On Diff #13250)

Hacked it like this but I feel like this isn't a good solution.

I don't feel confident enough to review Rust :(

tomek requested changes to this revision.Jun 1 2022, 6:42 AM
tomek added inline comments.
services/commtest/src/create_new_backup.rs
14–15 ↗(On Diff #13250)

It looks like you're trying to make String from &String. Maybe this let cloned_device_id = String::from(device_id); can work? Also let cloned_device_id = device_id.clone(); might work.

17–45 ↗(On Diff #13250)

Why do we need to wrap it with a loop when it does only one iteration and breaks?

This revision now requires changes to proceed.Jun 1 2022, 6:42 AM
services/commtest/src/create_new_backup.rs
10 ↗(On Diff #13250)

Are you sure &String is the best option here? What do you think about using &str?

services/commtest/src/create_new_backup.rs
10 ↗(On Diff #13250)

explained in the other inline.

14–15 ↗(On Diff #13250)

The problem here is with lifetimes. I tried to use str but the compiler was yelling that it should have a static lifetime. And when I made it static it also tried to force me to make all things using these variables to be static. I don't think we want them to be static at all. Maybe I'm doing something wrong, IDK.

.clone() works fine as well, I thought I've already tried it and something was broken but apparently it works.

17–45 ↗(On Diff #13250)

Yes, good catch, I took this code from somewhere and forgot to remove the loop, thx.

karol marked an inline comment as done.
varun requested changes to this revision.Jun 7 2022, 7:23 AM
varun added inline comments.
services/commtest/src/create_new_backup.rs
10–11 ↗(On Diff #13352)
19 ↗(On Diff #13352)
24 ↗(On Diff #13352)

Thanks for the comments! I decided to wrap these strings into a struct so it's handled differently.

tomek requested changes to this revision.Jun 8 2022, 5:51 AM
tomek added inline comments.
services/commtest/src/create_new_backup.rs
17–19 ↗(On Diff #13395)

Just wondering what is a good practice regarding this. Currently we pretend to borrow, but we copy the values when they arrive. Maybe instead we can own an argument and at the callsite we could decide if we want to copy or move? What do you think?

55–57 ↗(On Diff #13395)

Is it possible that we assign a value to backup_id more than once? Can we check if backup_id is present in only one response?

This revision now requires changes to proceed.Jun 8 2022, 5:51 AM
karol added inline comments.
services/commtest/src/create_new_backup.rs
17–19 ↗(On Diff #13395)

I do not think this is possible honestly. If we move the backup data into this function, we're not going to be able to use it for further calls that need to use this data. I think we have to borrow.

Currently we pretend to borrow, but we copy the values when they arrive.

Yes, it may look like this. In fact, we do borrow, not only pretend. We copy only the stuff that we need in the async calls. I think we talked about this and I guess rust just doesn't recognize whether we await all asyncs or not and assumes we may not, therefore it sees the lifetimes' collisions (that we launch some async stuff in this function, that will live, for instance, for 10 secs, then we exit this function and the outer scope that owns the backup data terminates after 3secs, so we would end up with dead references (if we passed the refs to the async function)).

Not sure about the best practice for this. @varun @jimpo?

Also a question - do I understand this correctly?

55–57 ↗(On Diff #13395)

We can do it, yes.

karol retitled this revision from [DRAFT] [services] Tests - Add create new backup to [services] Tests - Add create new backup.Jun 13 2022, 6:41 AM
tomek added inline comments.
services/commtest/src/create_new_backup.rs
17–19 ↗(On Diff #13395)

Yes, you're right and what you said makes sense. I still feel that there should be a better way to do it, but can't find one.

services/commtest/tests/backup/create_new_backup.rs
21–23 ↗(On Diff #13460)

I talked to the guy in swm who is good at rust and he said that this pattern's ok, it doesn't look the best but it's common in rust. Thing is, if we do not clone, rust will try to move these values into the outbound. We do not want that, we want to use them again later.

He showed me a similar pattern that he's seen many times in rust code:

let f = {
  let clon_a = a.clone();
  let clon_b = b.clone();
  move || {
    return clon_a + clon_b;
  }
}
services/commtest/tests/backup/create_new_backup.rs
21–23 ↗(On Diff #13460)

Great! Thanks for checking that

varun added inline comments.
services/commtest/tests/backup/create_new_backup.rs
27 ↗(On Diff #13460)

i don't think you need to_string() here

32 ↗(On Diff #13460)

same as above comment

61 ↗(On Diff #13460)

using is_empty is clearer and more explicit: backup_id.is_empty()

This revision is now accepted and ready to land.Jun 15 2022, 11:39 AM