Page MenuHomePhabricator

[services] Tests - Add pull backup
ClosedPublic

Authored by karol on Jun 7 2022, 6:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jul 5, 3:52 AM
Unknown Object (File)
Thu, Jul 4, 10:33 AM
Unknown Object (File)
Wed, Jul 3, 9:09 AM
Unknown Object (File)
Sun, Jun 30, 2:48 AM
Unknown Object (File)
Sat, Jun 29, 8:26 PM
Unknown Object (File)
Thu, Jun 27, 4:13 AM
Unknown Object (File)
Wed, Jun 26, 3:29 PM
Unknown Object (File)
Wed, Jun 26, 8:37 AM

Details

Summary

Depends on D4227

Adding pull backup module. It returns an instance of the BackupData with obtained sizes so they can be compared with the original ones later.

Test Plan

Please see D4216

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, varun, max.
tomek requested changes to this revision.Jun 14 2022, 3:53 AM
tomek added inline comments.
services/commtest/tests/backup/pull_backup.rs
25 ↗(On Diff #13463)

Is it a good idea to implement Display by using Debug?

56 ↗(On Diff #13463)

Instead of having long if-else-if chains, shouldn't we use match?

87–109 ↗(On Diff #13463)

Is there a way to reduce duplication of logic here?

This revision now requires changes to proceed.Jun 14 2022, 3:53 AM
services/commtest/tests/backup/pull_backup.rs
25 ↗(On Diff #13463)

We can remove this, ok

56 ↗(On Diff #13463)

yes, we can use match, thx!

87–109 ↗(On Diff #13463)

I don't think so, if you have something specific on your mind, please, let me know

91 ↗(On Diff #13463)

seems redundant

karol added inline comments.
services/commtest/tests/backup/pull_backup.rs
105 ↗(On Diff #13522)

we shouldn't panic here

tomek requested changes to this revision.Jun 20 2022, 3:38 AM
tomek added inline comments.
services/commtest/tests/backup/pull_backup.rs
1–4 ↗(On Diff #13524)

Is there a way to avoid specifying the paths? Why do we need that?

9 ↗(On Diff #13524)

Is it a good idea to import ::*? Can we specify all the imports instead?

16–17 ↗(On Diff #13524)

Why do we need PartialEq on State? How does it work?

47 ↗(On Diff #13524)

Is the type annotation needed?

57 ↗(On Diff #13524)

What does this message mean? Can you make it a little less cryptic?

64 ↗(On Diff #13524)

What happens if we panic here? Would the whole service stop working?

105 ↗(On Diff #13524)

When we can have None here? Shouldn't we return an error?

This revision now requires changes to proceed.Jun 20 2022, 3:38 AM
services/commtest/tests/backup/pull_backup.rs
1–4 ↗(On Diff #13524)

I couldn't find a better way to do this, maybe @varun can help with this?

I may not be remembering this 100% correctly now, but:

The problem was with how the tests work vs how the program with main works. In main you need to only once declare the mod and then you can import it in other files, right?
In the tests, you have to define the module in every test file and the problem is how rust resolves these mods.
Imagine you have:

- a
- dir
  - b
  - c

now:

  • in b you have a struct
  • in c you have a function that takes this struct as an arg
  • in a you try to invoke this function and pass some object created in a

Without paths like this, you'll end up with something like type mismatch, expected c::structx, received a::structx (or received b::structx I don't remember).

9 ↗(On Diff #13524)

I do not see a point in doing so.
This enum looks liker this:

pub mod pull_backup_response {
  #[derive(Clone, PartialEq, ::prost::Oneof)]
  pub enum Data {
    #[prost(bytes, tag = "1")]
    CompactionChunk(::prost::alloc::vec::Vec<u8>),
    #[prost(bytes, tag = "2")]
    LogChunk(::prost::alloc::vec::Vec<u8>),
    #[prost(string, tag = "3")]
    AttachmentHolders(::prost::alloc::string::String),
  }
}

There's no redundancy, we use everything we import. Why would you like the explicit list here?

16–17 ↗(On Diff #13524)

We need this so we can compare the values of the objects of this enum. What do you mean how does it work? I'm just not sure what's unclear, I bet you already know this https://doc.rust-lang.org/reference/attributes/derive.html.

47 ↗(On Diff #13524)

It's more clear to me when it's there. If you really think it's annoying, I can remove this.

57 ↗(On Diff #13524)

yes

64 ↗(On Diff #13524)

Yes, good catch, I'll try to debug this, thanks.

105 ↗(On Diff #13524)

we cannot but the compiler forces the None section. I think we can panic here.

services/commtest/tests/backup/pull_backup.rs
105 ↗(On Diff #13524)

Ok I checked and None seems to be sent when the connection's being terminated, so we should NOT panic.

services/commtest/tests/backup/pull_backup.rs
64 ↗(On Diff #13524)

I fixed this bug in D4341

tomek added inline comments.
services/commtest/tests/backup/pull_backup.rs
1–4 ↗(On Diff #13524)

Not sure if it will help, but you can check https://doc.rust-lang.org/book/ch11-03-test-organization.html where integration tests are written without any path specified.

9 ↗(On Diff #13524)

Overall in any language importing wildcards might cause issues when two different modules export the same name. I guess rust will raise an error in such a case.

16–17 ↗(On Diff #13524)

Sorry, my mistake. For some reason I thought you were deriving PartialOrd - which would indeed be confusing. With PartialEq everything is fine

services/commtest/tests/backup/pull_backup.rs
1–4 ↗(On Diff #13524)

Unfortunately, that does not help. They show a very simple example. I created a minimal repro which shows our case: https://github.com/karol-bisztyga/rust-tests-paths-repro

You can try to remove all occurrences of #[path = "./tools.rs"] and see what happens.

9 ↗(On Diff #13524)

What is the problem with keeping this and in a case of a collision listing all items manually and using as? I mean, as long as it works as it is, I think we're good. If it breaks, we can easily fix this but until it works, the shorter form the better.

I'd leave this as is. If you feel strongly, we can start a discussion about this and get someone else's vote about this.

This revision is now accepted and ready to land.Jun 26 2022, 9:28 PM
max added inline comments.
services/commtest/tests/lib/tools.rs
22 ↗(On Diff #13708)

Maybe we should follow the services approach in the case of constants and move it to its source file called constants.rs?

services/commtest/tests/lib/tools.rs
22 ↗(On Diff #13708)

good idea

This revision was automatically updated to reflect the committed changes.
services/commtest/tests/lib/tools.rs
22 ↗(On Diff #13708)
22 ↗(On Diff #13708)

Sorry wrong diff, I couldn't make it work there