Details

Reviewers

varun
tomek
• max
ashoat

Commits

rCOMM07c9aceaa2ad: [services] Tests - Initialize project

Summary

Please note that this is a draft, I'm checking for your opinions about the idea.

This initializes the project:

adds dependencies - you'll see me using them later in the stack
adds the port for the test "service"
adds the build script - the idea is that we're going to pass the target service as an argument to the test "service" and in the testing container we're going to compile files only for the proto of the target service.

I think we can treat the test project as another service that will never leave the dev environment. This is because we do not want devs to download dependencies and build the project on their host machines, it's going to be built inside of the docker container.

Test Plan

This change itself doesn't do anything, so for now, none.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

new lines

Harbormaster failed remote builds in B9633: Diff 13350!Jun 6 2022, 4:34 AM

Harbormaster completed remote builds in B9634: Diff 13351.Jun 6 2022, 4:42 AM

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

varun added inline comments.

services/commtest/build.rs
5	Why do we need to call `map_err` here? I think we can just do `let target_service = env::var("COMM_TEST_TARGET")?;`, right? Separately, you can cut out some of the syntax in this line. `.map_err(\|err\| format!("invalid test target: {}", err))?;` should work

This revision now requires changes to proceed.Jun 6 2022, 6:50 AM

Back to your queue, I think the reason for using map_err is good but if you feel strongly that we should avoid it, please let me know.

services/commtest/build.rs
5	The point here is to decorate the error message. If this env var is not set, the error message will be something like (more or less): `value is not present`, I remember it was something not very descriptive and I don't think it's going to scale well, once we have more code here, such an error message will not make debugging easy.

tomek accepted this revision.Jun 7 2022, 1:46 AM

• karol planned changes to this revision.Jun 7 2022, 6:45 AM

update

Harbormaster completed remote builds in B9652: Diff 13378.Jun 7 2022, 7:18 AM

varun requested changes to this revision.Jun 7 2022, 7:18 AM

varun added inline comments.

services/commtest/build.rs
5	How about this? The output will be: thread 'main' panicked at 'Invalid environment variable: NotPresent', build.rs:5:34 In general, you don't want to map errors to Strings because you lose some of the data in the error. If this weren't a build script, I'd suggest creating a new Error enum that wraps the third party errors we encounter in this function.

This revision now requires changes to proceed.Jun 7 2022, 7:18 AM

• karol added inline comments.Jun 7 2022, 7:35 AM

services/commtest/build.rs
5	I see. I wanted to avoid panicking. This is why I asked the other day about rust best practices. I read a bit about error handling and using `Result` seems to be the cleanest option. I know this is a build script but it still may grow some day, right? If this weren't a build script, I'd suggest creating a new Error enum that wraps the third party errors we encounter in this function. Why cannot we do this in a build script? I know you have more experience in rust, I just think that `panic` does not look like a good solution, I think it would be good to agree on one way of handling errors, stick to it and be consistent. What do you think? Maybe we could agree on using `Result` with that error enum (can you provide an example and also what do you mean that some info is lost, what is lost exactly)?

back to your Q

varun requested changes to this revision.Jun 7 2022, 7:46 AM

varun added inline comments.

services/commtest/build.rs
5	IMO it's not a big deal to panic in a build script since we're not propagating errors, but we can avoid them here if you'd like. My suggestion is to do something like this: use std::env; fn main() -> Result<(), Error> { let target_service = env::var("COMM_TEST_TARGET")?; tonic_build::compile_protos(format!( "../../native/cpp/CommonCpp/grpc/protos/{}.proto", target_service ))?; Ok(()) } #[derive( Debug, derive_more::Display, derive_more::From, derive_more::Error, )] enum Error { #[display(...)] EnvVar(env::VarError), #[display(...)] Proto(std::io::Error), } When you use `format!` to convert the error to a String, all the data stored in the error type besides what is returned by the `Display` trait implementation is lost. Does that make sense?

This revision now requires changes to proceed.Jun 7 2022, 7:46 AM

varun added inline comments.Jun 7 2022, 7:48 AM

services/commtest/build.rs
5	Ex: https://github.com/CommE2E/comm/blob/master/services/identity/src/database.rs#L224

• karol added inline comments.Jun 8 2022, 12:51 AM

services/commtest/build.rs
5	I see, thx for the explanation. The example you provided looks good, I'm going to update the code with this.

Harbormaster completed remote builds in B9633: Diff 13350.Jun 8 2022, 1:29 AM

• karol added a child revision: D4164: [services] Tests - Add backup utils.Jun 13 2022, 4:40 AM

• karol removed a child revision: D4162: [DRAFT] [services] Tests - Add Dockerfile.

• karol retitled this revision from [DRAFT] [services] Tests - Initialize project to [services] Tests - Initialize project.Jun 13 2022, 6:41 AM

new stack

Harbormaster completed remote builds in B9712: Diff 13457.Jun 13 2022, 6:49 AM

varun requested changes to this revision.Jun 13 2022, 6:59 AM

varun added inline comments.

.lintstagedrc.js
47 ↗	(On Diff #13457)	why can't we just `return 'cargo fmt';`?
services/commtest/src/main.rs
2 ↗	(On Diff #13457)	just fyi, you can write `unimplemented!()`. you don't have to change it, just wanted to share this shortcut https://doc.rust-lang.org/std/macro.unimplemented.html

This revision now requires changes to proceed.Jun 13 2022, 6:59 AM

ashoat added inline comments.Jun 13 2022, 12:03 PM

.lintstagedrc.js
47 ↗	(On Diff #13457)	Will this cause `git commit` to crash if the user edits a `.rs` file inside `services/commtest` and the dev environment doesn't have `cargo`? Will this cause `git commit` to crash if a user edits some other file and the dev environment doesn't have `cargo`?

use unimplemented!();, avoid pushd into dirs and use --manifest-path instead.

Harbormaster completed remote builds in B9733: Diff 13478.Jun 14 2022, 2:56 AM

(My questions haven't been answered)

Sorry, forgot to hit "submit" button

.lintstagedrc.js
47 ↗	(On Diff #13457)	why can't we just `return 'cargo fmt';`? We have to explicitly visit (or specify) certain paths where we have `Cargo.toml`. If there's a simpler way (like where rust automatically detects that), please, let me know. Will this cause `git commit` to crash if the user edits a `.rs` file inside `services/commtest` and the dev environment doesn't have `cargo`? Will this cause `git commit` to crash if a user edits some other file and the dev environment doesn't have `cargo`? Yes. No.
services/commtest/src/main.rs
2 ↗	(On Diff #13457)	nice, thx

we should probably also have pre-commit hooks for cargo check and cargo test. also, have you considered using this package: https://github.com/swellaby/rusty-hook ?

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

I checked this out to play around a little and didn't like that cargo test fails because we only compile protos for one service. We should just compile the protos for all services and then use the option --test [<NAME>] to specify which tests to execute (e.g. cargo test --test backup_test). See inline comment for suggested change

services/commtest/build.rs
1–20 ↗	(On Diff #13478)

This revision now requires changes to proceed.Jun 15 2022, 11:10 AM

• karol added inline comments.Jun 20 2022, 3:06 AM

services/commtest/build.rs
1–20 ↗	(On Diff #13478)	Ok, we can compile all protos, but if we do this, I'd rather just iterate through the files from this directory than list them manually.

In D4161#120532, @varun wrote:

we should probably also have pre-commit hooks for cargo check and cargo test.

I'm not sure if this is a good idea. First, we should consider that there are devs whose work wouldn't touch anything in the services directory. Therefore they'd probably not install anything, run any services locally, etc. So cargo check and cargo test would fail for them. Plus, integration tests (which we agreed to run using cargo test) require the services to be run locally. Kind of an overkill to force users to run services locally just to push some js changes on native or something, right?

Having just a code formatter like eslint is not a problem since it works only on the staged files.

also, have you considered using this package: https://github.com/swellaby/rusty-hook ?

No, I have not.

compile all proto files

Harbormaster completed remote builds in B9830: Diff 13594.Jun 20 2022, 3:34 AM

• karol mentioned this in D4216: [services] Tests - Fix scripts.Jun 20 2022, 4:13 AM

I'm not sure if this is a good idea. First, we should consider that there are devs whose work wouldn't touch anything in the services directory. Therefore they'd probably not install anything, run any services locally, etc. So cargo check and cargo test would fail for them.

This is very easy to solve, just don't run those things unless the changes touch relevant files. This is easy to configure in lint-staged (eg. see D4161).

Plus, integration tests (which we agreed to run using cargo test) require the services to be run locally. Kind of an overkill to force users to run serices locally just to push some js changes on native or something, right?

We should not run integration tests in commit hooks.

also, have you considered using this package: https://github.com/swellaby/rusty-hook ?

No, I have not.

This may be some confusion about English. When somebody asks you this question, they're asking you to do the investigation.

This revision now requires changes to proceed.Jun 20 2022, 10:39 AM

This is very easy to solve, just don't run those things unless the changes touch relevant files. This is easy to configure in lint-staged (eg. see D4161).

Did you really mean to link this very diff here as a reference?

Anyway, the question is whether we want to have cargo check and cargo test run on the commit hooks. Please note that this may be tricky as cargo test runs integration tests for the commtest and for the identity it probably runs unit tests. We can wrap these things in some conditions, the question is: do we want to have unit tests run on the commit hooks? If so, then we probably will want to also run the c++ unit tests.

We can do cargo check, I think it's ok, but I will wait with the changes until we have an agreement here.

We should not run integration tests in commit hooks.

Ok, as above, how about the unit tests then?

This may be some confusion about English. When somebody asks you this question, they're asking you to do the investigation.

I looked at this lib and I'm not sure we need this. What features are we missing right now that this lib would bring? I think we'll be good with just running simple cargo commands. We already have a tool that recognizes changed files in the pre commit hook, what else do we need? I don't see why we'd add a lib to every language we use (we use the same linstaged for cpp). Besides, this lib is still in beta, community is acceptable, but I'm not really convinced of adding this tbh.

Did you really mean to link this very diff here as a reference?

Haha, I wasn't paying attention... remembered seeing this in some diff, opened another tab to find the diff, and simply didn't realize it was the same diff (lol!)

Anyway, the question is whether we want to have cargo check and cargo test run on the commit hooks. Please note that this may be tricky as cargo test runs integration tests for the commtest and for the identity it probably runs unit tests. We can wrap these things in some conditions, the question is: do we want to have unit tests run on the commit hooks? If so, then we probably will want to also run the c++ unit tests.

Unit tests – yes, integration tests – no

I looked at this lib and I'm not sure we need this. What features are we missing right now that this lib would bring? I think we'll be good with just running simple cargo commands. We already have a tool that recognizes changed files in the pre commit hook, what else do we need? I don't see why we'd add a lib to every language we use (we use the same linstaged for cpp). Besides, this lib is still in beta, community is acceptable, but I'm not really convinced of adding this tbh.

Thanks for the response, this is all that I was looking for! I wasn't the original asker, so will defer to @varun to respond here.

Anyway, the question is whether we want to have cargo check and cargo test run on the commit hooks. Please note that this may be tricky as cargo test runs integration tests for the commtest and for the identity it probably runs unit tests. We can wrap these things in some conditions, the question is: do we want to have unit tests run on the commit hooks? If so, then we probably will want to also run the c++ unit tests.

i feel like running unit tests on a pre-commit hook is a good practice

We can do cargo check, I think it's ok, but I will wait with the changes until we have an agreement here.

let's do this

I looked at this lib and I'm not sure we need this. What features are we missing right now that this lib would bring? I think we'll be good with just running simple cargo commands. We already have a tool that recognizes changed files in the pre commit hook, what else do we need? I don't see why we'd add a lib to every language we use (we use the same linstaged for cpp). Besides, this lib is still in beta, community is acceptable, but I'm not really convinced of adding this tbh.

package seemed like a really simple way to add multiple git hooks (not just pre-commit but also pre-push, post-commit, etc.). but i'm not opposed to this approach

services/commtest/build.rs
8–11 ↗	(On Diff #13594)	i have no problem with `unwrap` here but i remember you wanted to avoid it
12 ↗	(On Diff #13594)	what's going on here?

This revision is now accepted and ready to land.Jun 22 2022, 8:06 AM

Ok, I'm going to add cargo check here but I think the unit tests for the pre-commit hooks should be done as a follow-up as they refer to the c++ code as well - https://linear.app/comm/issue/ENG-1290/add-unit-tests-of-services-to-the-pre-commit-hooks

services/commtest/build.rs
8–11 ↗	(On Diff #13594)	Ok, I can get rid of `unwrap`s
12 ↗	(On Diff #13594)	I copied this solution from the Internet, I just needed to list the files from the given directory as strings. Is there a simpler way of doing this?

address feedback

• karol added inline comments.Jun 23 2022, 2:07 AM

scripts/rust_pre_commit.sh
7 ↗	(On Diff #13707)	I removed identity from here. I have a couple of questions @varun Is identity meant to be run only in the docker image? Are you planning to do unit tests for the identity? If the identity is run only inside of a docker container then we may not be able to run stuff like `cargo test` or `cargo check` on the host machine. This is another reason why separating `cargo fmt` was a good idea (as it just format the code and can be run without problems) but ok.

Harbormaster failed remote builds in B9913: Diff 13707!Jun 23 2022, 2:12 AM

Harbormaster completed remote builds in B9913: Diff 13707.Jun 23 2022, 9:20 AM

Closed by commit rCOMM07c9aceaa2ad: [services] Tests - Initialize project (authored by • karol). · Explain WhyJun 27 2022, 4:21 AM

This revision was automatically updated to reflect the committed changes.

• karol added a commit: rCOMM07c9aceaa2ad: [services] Tests - Initialize project.

varun added inline comments.Jun 27 2022, 11:35 AM

scripts/rust_pre_commit.sh
7 ↗	(On Diff #13707)	the service will be deployed in a docker container in production, but we should be able to run it locally with `cargo run` (i.e. without docker). moving the .proto file to the native directory made this a little trickier (the identity service build script expects the .proto file to be in the cargo project directory), but i've gotten it working with a hard link i expect we will write unit tests for some functions in the future

Hard links are always a bit sketchy... would be great if we could find a way to avoid them, and maybe potentially address ENG-1231 in the process

In D4161#123941, @ashoat wrote:

Hard links are always a bit sketchy... would be great if we could find a way to avoid them, and maybe potentially address ENG-1231 in the process

Better solution than hard links: https://linear.app/comm/issue/ENG-1313/check-for-proto-file-in-multiple-places

ENG-1231 is currently assigned to @jonringer-comm so I'll ask him about it when we sync tomorrow

[services] Tests - Initialize project
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 13351

services/commtest/.gitignore

services/commtest/Cargo.lock

services/commtest/Cargo.toml

services/commtest/build.rs

services/commtest/rustfmt.toml

services/commtest/src/main.rs

[services] Tests - Initialize projectClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 13351

services/commtest/.gitignore

services/commtest/Cargo.lock

services/commtest/Cargo.toml

services/commtest/build.rs

services/commtest/rustfmt.toml

services/commtest/src/main.rs

[services] Tests - Initialize project
ClosedPublic
Actions

Revision Contents
Changeset List