Details

Reviewers

tomek
varun
• max
• jon
• abosh

Commits

rCOMM57955f8eb92e: [services] Tests - Fix scripts

Summary

Depends on D4250

I changed my approach a bit when it comes to testing the services.

I divided the tests into two groups:

unit tests
integration tests

Now, if you want to run the tests for one service/all services, you'll do something like this:

yarn run-unit-tests backup
yarn run-integration-tests backup
yarn run-unit-tests blob
yarn run-integration-tests blob
yarn run-unit-tests all
yarn run-integration-tests all

Test Plan

cd services
yarn run-unit-tests
yarn run-integration-tests

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

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

• karol retitled this revision from [services] Tests - Add scripts to [services] Tests - Fix scripts.Jun 13 2022, 6:52 AM

new stack

• karol removed a parent revision: D4233: [services] Backup - Fix data size database limit.Jun 13 2022, 6:58 AM

• karol edited the summary of this revision. (Show Details)Jun 13 2022, 7:22 AM

• karol added a parent revision: D4250: [services] Tests - Add dummy tests.

• karol added a child revision: D4251: [services] Blob - Fix unit tests.Jun 13 2022, 7:24 AM

• karol edited the summary of this revision. (Show Details)Jun 13 2022, 7:27 AM

• karol edited the test plan for this revision. (Show Details)

Didn't we agree that once the nix is fully integrated we can replace npm with it here?

Does Nix really replace npm in this way? I would guess that in Nix, we would still need something like a bash script. Nix would just let us add the bash script to $PATH or something, but it doesn't have an equivalent of package.json's scripts section (I think)

Harbormaster completed remote builds in B9727: Diff 13472.Jun 14 2022, 2:25 AM

tomek requested changes to this revision.Jun 14 2022, 2:47 AM

tomek added inline comments.

services/scripts/run_integration_tests.sh
12 ↗	(On Diff #13472)
12 ↗	(On Diff #13472)	Why do we need to use `$1` in two places? (env variable and `$1_test` expression?
41 ↗	(On Diff #13472)	Should we somehow set a return value of the script so that when a test fails, the caller of this script would know it? Also, when we run a couple of tests and any of them fails, we should return a non-zero code. (It depends on our CI if that is really needed, but we need to somehow let CI know that a test failed).
services/scripts/run_unit_tests.sh
35–37 ↗	(On Diff #13472)	We assume here that one service name can't be a substring of other service name. Is there a way to avoid this assumption?

This revision now requires changes to proceed.Jun 14 2022, 2:47 AM

In D4216#119953, @ashoat wrote:

Didn't we agree that once the nix is fully integrated we can replace npm with it here?

Does Nix really replace npm in this way? I would guess that in Nix, we would still need something like a bash script. Nix would just let us add the bash script to $PATH or something, but it doesn't have an equivalent of package.json's scripts section (I think)

All in all, we can get back to this once the nix stuff is done, right? https://linear.app/comm/issue/ENG-1259/get-rid-of-npm-for-calling-services-scripts

services/scripts/run_integration_tests.sh
12 ↗	(On Diff #13472)	This is because we first need to pass the env var to the build script which then compiles the proper proto file. Then we need to launch the target test. I'm not sure what's wrong with using `$1` twice?
41 ↗	(On Diff #13472)	We have set -e for that.
services/scripts/run_unit_tests.sh
35–37 ↗	(On Diff #13472)	We probably can do it differently but I'm not sure if we need it. I'd say this is a low-prio stuff, I created a task for this: https://linear.app/comm/issue/ENG-1258/enable-one-service-name-to-be-a-substring-of-another-one

remove redundant space

Harbormaster completed remote builds in B9734: Diff 13479.Jun 14 2022, 4:14 AM

tomek accepted this revision.Jun 14 2022, 5:16 AM

tomek added inline comments.

services/scripts/run_integration_tests.sh
12 ↗	(On Diff #13472)	It feels like a duplication and increases the possibility of a mistake. If we compile the code with env variable set, is it possible to run the result for a different service, e.g. can `COMM_TEST_TARGET=backup cargo test blob_test --test '*' --manifest-path=commtest/Cargo.toml` run correctly? My point is, if these two always need to have the same value, there's no point in separating them. What do you think?
41 ↗	(On Diff #13472)	Ok, that makes sense. But are we sure that `run_integration_test $SERVICE` returns non-zero in case of a single test failure?

This revision is now accepted and ready to land.Jun 14 2022, 5:17 AM

In D4216#120035, @karol-bisztyga wrote:

In D4216#119953, @ashoat wrote:

Didn't we agree that once the nix is fully integrated we can replace npm with it here?

Does Nix really replace npm in this way? I would guess that in Nix, we would still need something like a bash script. Nix would just let us add the bash script to $PATH or something, but it doesn't have an equivalent of package.json's scripts section (I think)

All in all, we can get back to this once the nix stuff is done, right? https://linear.app/comm/issue/ENG-1259/get-rid-of-npm-for-calling-services-scripts

Sounds good, thanks for creating a task 👍

• karol added inline comments.Jun 20 2022, 4:13 AM

services/scripts/run_integration_tests.sh
26 ↗	(On Diff #13479)	bug
12 ↗	(On Diff #13472)	With the changes added to D4161 we no longer need `COMM_TEST_TARGET` so this is solved, I will push according changes here.
41 ↗	(On Diff #13472)	yes I explicitly re-checked and if one test fails, the whole script does as well.

fix bug

Harbormaster completed remote builds in B9834: Diff 13606.Jun 20 2022, 4:20 AM

• karol mentioned this in rCOMMef3d6d8ac48e: [services] Tests - Add pull backup.Jun 27 2022, 4:21 AM

retrigger CI

Harbormaster failed remote builds in B10147: Diff 14017!Jun 30 2022, 7:46 AM

Harbormaster completed remote builds in B10147: Diff 14017.Jun 30 2022, 8:52 AM

• jon added a subscriber: • jon.Jun 30 2022, 2:07 PM

• jon added inline comments.

services/scripts/run_integration_tests.sh
8 ↗	(On Diff #14017)	probably want to canonicalize the path before invoking, or else invoking this from the project root will cause this to resolve incorrectly
9 ↗	(On Diff #14017)	may want to assert that someone invoked the script with an argument

ashoat added a reviewer: • jon.Jun 30 2022, 2:08 PM

add all option

Harbormaster completed remote builds in B10198: Diff 14076.Jul 1 2022, 2:50 AM

I think @yayabosh should review any bash going forward, he's done some really good reviews on it

services/scripts/run_unit_tests.sh
1 ↗	(On Diff #14076)	@yayabosh can you run your Bash sanitizer here and see how it looks?
22 ↗	(On Diff #14076)	Please don't introduce any more single-bracket conditionals in `bash`, @yayabosh is working in ENG-157 to fix this...

This revision now requires review to proceed.Jul 2 2022, 6:32 PM

@ashoat I can also add shellcheck to the nix dev env. Closest thing to a linter for bash, that I'm aware of.

@jonringer-comm Sure that’d be awesome! Here’s the Linear issue to track that: https://linear.app/comm/issue/ENG-1296/add-shellcheck-or-something-similar-to-the-ci

• karol planned changes to this revision.Jul 5 2022, 1:48 AM

• karol added inline comments.

services/scripts/run_integration_tests.sh
8 ↗	(On Diff #14017)	Why would anyone want to trigger this from the root directory? If this is needed, we can always use `cd`/`pushd`. We do not do this in any script, I think we should either apply this technique in all scripts as a follow-up or nowhere. I do not mind personally, but I don't think this will be super useful. What's your opinion?
9 ↗	(On Diff #14017)	this is done here: if [ -z "$1" ]; then echo "No service specified"; list_expected; exit 1; fi I can add `all` option to the hint.

address fdbck

Harbormaster failed remote builds in B10253: Diff 14145!Jul 5 2022, 1:49 AM

@yayabosh when can I expect a review from you here? You are a blocking reviewer... thx

Some style fixes here and there! Should be good after that.

services/scripts/list_services.sh
5 ↗	(On Diff #14145)	Don't use ls \| grep. Use a glob or a for loop with a condition to allow non-alphanumeric filenames. This suggested edit looks kind of messy and that's because find (both the GNU and BSD variants) do not support lookahead/lookbehind. So it's just a suggested solution, but you can also use a for loop or if statement to loop through the files.
services/scripts/run_integration_tests.sh
8 ↗	(On Diff #14145)	Use $(...) notation instead of legacy backticked ...
13 ↗	(On Diff #14145)	Double quote to prevent globbing and word splitting.
30 ↗	(On Diff #14145)	Double quote to prevent globbing and word splitting.
35 ↗	(On Diff #14145)	Not that it really matters in this particular instance since the command ends right after the variable is set, but you can use a here string instead of the `echo` + pipe. In my opinion it looks cleaner and if the script did go further, you could run into some issues using `echo`. Also make sure to double quote to prevent globbing and word splitting and use $(...) notation instead of legacy backticked ...
43 ↗	(On Diff #14145)	Double quote to prevent globbing and word splitting.
services/scripts/run_unit_tests.sh
8 ↗	(On Diff #14145)	Use $(...) notation instead of legacy backticked ...
13 ↗	(On Diff #14145)
14 ↗	(On Diff #14145)
30 ↗	(On Diff #14145)
35 ↗	(On Diff #14145)	Not that it really matters in this particular instance since the command ends right after the variable is set, but you can use a here string instead of the `echo` + pipe. In my opinion it looks cleaner and if the script did go further, you could run into some issues using `echo`. Also make sure to double quote to prevent globbing and word splitting and use $(...) notation instead of legacy backticked ...
43 ↗	(On Diff #14145)