Page MenuHomePhabricator

[services] Tests - Add tools
ClosedPublic

Authored by karol on Jun 7 2022, 6:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jul 1, 3:47 PM
Unknown Object (File)
Sat, Jun 29, 9:42 PM
Unknown Object (File)
Sat, Jun 29, 7:50 PM
Unknown Object (File)
Sat, Jun 29, 9:15 AM
Unknown Object (File)
Sat, Jun 29, 2:40 AM
Unknown Object (File)
Thu, Jun 27, 8:59 PM
Unknown Object (File)
Wed, Jun 26, 9:27 AM
Unknown Object (File)
Wed, Jun 26, 9:27 AM

Details

Summary

Depends on D4164

Adding tools with one function: generate_nbytes. It returns n bytes (that are the same).

Test Plan

this doesn't do anything on its own. Tests can be built.

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, max, varun.

It's true we can replace this function inline with just vec![n, b] but using such a named function is more readable for me. If you feel like it's useless, we can abandon.

services/commtest/src/tools.rs
2 ↗(On Diff #13380)

I'm not a Rust expert, but seems confusing to "shadow" the existing variable named c with another variable named c

karol retitled this revision from [services] Tests - Add tools to [DRAFT] [services] Tests - Add tools.Jun 8 2022, 12:54 AM
services/commtest/src/tools.rs
2 ↗(On Diff #13380)

I know, I think it goes beyond rust - I mean it doesn't matter what language we use, we may like shadowing or not.

For me it looks ok but I do not mind changing it.

I don't think this function is really useful, because as you said, we can just use vec![n, b]. One option to make it a better is to consider generating random bytes: we can use seedable random to make tests repeatable.

varun requested changes to this revision.Jun 8 2022, 6:49 AM

I think we should generate random bytes. You can do something like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3d609eb08cf49a2d3a97ca9a6599786b

Some other notes:

  • are you using cargo fmt on your code? line 2 looks like it has an extra space between c and the colon
  • n and c aren't very descriptive names for parameters. would prefer something like num_bytes and byte_value
This revision now requires changes to proceed.Jun 8 2022, 6:49 AM

I think we should generate random bytes. You can do something like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3d609eb08cf49a2d3a97ca9a6599786b

Not sure if generating pure random data is a good idea as it makes tests not repeatable. Instead, we can use seedable random, which uses a seed that can be hardcoded and based on that generates the same result for every test call.

In D4225#119539, @palys-swm wrote:

I think we should generate random bytes. You can do something like this: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3d609eb08cf49a2d3a97ca9a6599786b

Not sure if generating pure random data is a good idea as it makes tests not repeatable. Instead, we can use seedable random, which uses a seed that can be hardcoded and based on that generates the same result for every test call.

Good point. this crate makes it easy to create a thread-local rng from a seed. https://github.com/smol-rs/fastrand#examples

As for the random data - the specific reason why I didn't want to use random data is that it takes really a lot of time to fill bigger buffers with random bytes. Filling just with, let's say, one hardcoded byte is much more efficient. And in these tests, eventually, we'll want to test mega and giga-bytes-long pieces of data. I remember in c++ it took too long to fill a couple of GB with random data.

With that said, I'm down to try to replace just mocking one byte and filling the buffer with random data. We could resign from this function, but please note, that it's going to be easier to maintain this code. If we ever want to replace this logic with, for example, random bytes, we'll just need to replace the code in this one function. When using inline replacements, we'll have to find every place in which we do this operation.

Ok, good point - let's keep the function.

karol retitled this revision from [DRAFT] [services] Tests - Add tools to [services] Tests - Add tools.Jun 13 2022, 6:41 AM
karol edited the test plan for this revision. (Show Details)

new stack

varun requested changes to this revision.Jun 14 2022, 10:16 AM
varun added inline comments.
services/commtest/tests/lib/tools.rs
5 ↗(On Diff #13459)

See my earlier comment about parameter names

6 ↗(On Diff #13459)

this is pretty obvious from the function signature

13–25 ↗(On Diff #13459)

I don't it's a good practice to have a global Error enum, if that's what this is. You can look at how database.rs and config.rs have different Error enums, for example

This revision now requires changes to proceed.Jun 14 2022, 10:16 AM
services/commtest/tests/lib/tools.rs
13–25 ↗(On Diff #13459)

I tried to implement them separately but then the compiler complained about different types when invoking methods with ?, for example when you invoke create_new_backup::run(&mut client, &backup_data).await?; in backup_test and you have error types defined separately for backup test and create new backup, it's going to cause errors and probably it needs some imports/more boilerplate.
I do not think this error type is expensive and essentially, probably all the functionalities in these tests may encounter the same errors. I'd leave this, if you feel strongly, we can investigate further.

With that said, I just spotted that EnvVar is redundant.

17–18 ↗(On Diff #13459)

remove this

If the Error enum variants here are relevant to all the test functions, then I'm ok with this.

This revision is now accepted and ready to land.Jun 15 2022, 8:45 AM
services/commtest/tests/lib/tools.rs
5–8 ↗(On Diff #13521)
This revision was automatically updated to reflect the committed changes.