Details

Reviewers

tomek
varun

Commits

rCOMMfdf06113620d: [services] Tests - Add backup test logic

Summary

Depends on D4251

Adding code for the backup test itself.

It does the following things:

define starting data
create a new backup and add attachments to it based on the starting data
send logs and add attachments to them based on the starting data
perform "pull backup" and save the results
compare the obtained data with the starting data

Test Plan

cd services
yarn run-integration-tests backup

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Jun 13 2022, 7:08 AM

Herald added subscribers: • abosh, atul, • adrian and 2 others. · View Herald TranscriptJun 13 2022, 7:08 AM

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

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

• karol added reviewers: tomek, varun.

• karol added a parent revision: D4251: [services] Blob - Fix unit tests.

Harbormaster completed remote builds in B9729: Diff 13474.Jun 14 2022, 2:35 AM

• karol requested review of this revision.Jun 14 2022, 2:35 AM

• karol planned changes to this revision.Jun 14 2022, 3:58 AM

• karol added inline comments.

services/commtest/tests/backup_test.rs
23 ↗	(On Diff #13474)	I think we should use `http://localhost:50052`

use localhost

Harbormaster completed remote builds in B9735: Diff 13480.Jun 14 2022, 4:20 AM

• karol added a child revision: D4270: [services] Lib - fix bugs in ClientWriteReactorBase.Jun 15 2022, 3:14 AM

tomek requested changes to this revision.Jun 20 2022, 4:42 AM

tomek added inline comments.

services/commtest/tests/backup_test.rs
1–12 ↗	(On Diff #13480)	Why do we need to specify the paths? Can't we place the code in place where rust expects it?
23–24 ↗	(On Diff #13480)	Shouldn't we take this value from somewhere (env, config file)? It's not really maintainable when you need to search the codebase in order to modify the port.
39 ↗	(On Diff #13480)
73–78 ↗	(On Diff #13480)	Use `assert_eq` in all the assertions

This revision now requires changes to proceed.Jun 20 2022, 4:42 AM

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

services/commtest/tests/backup_test.rs
1–12 ↗	(On Diff #13480)	This discussion is already up in D4228, let's follow up there. EDIT: I created a task for this: https://linear.app/comm/issue/ENG-1300/[rust]-get-rid-of-import-paths-in-tests
23–24 ↗	(On Diff #13480)	Yes, definitely. That goes to the blob test as well. D4379
39 ↗	(On Diff #13480)	ok, we can do this
73–78 ↗	(On Diff #13480)	But why? What difference does this make?

• karol mentioned this in D4379: [services] Tests - Read port from the env vars.Jun 28 2022, 7:49 AM

varun added inline comments.Jun 28 2022, 8:07 AM

services/commtest/tests/backup_test.rs
73–78 ↗	(On Diff #13480)	`assert_eq` has a much more insightful panic message than `assert` when testing for equality https://stackoverflow.com/questions/43767552/why-do-assert-eq-and-assert-ne-exist-when-a-simple-assert-will-suffice

• karol added inline comments.Jun 29 2022, 2:37 AM

services/commtest/tests/backup_test.rs
73–78 ↗	(On Diff #13480)	I see

address feedback

Harbormaster failed remote builds in B10077: Diff 13931!Jun 29 2022, 2:53 AM

tomek accepted this revision.Jun 29 2022, 3:34 AM

tomek added inline comments.

services/commtest/tests/backup_test.rs
83–88 ↗	(On Diff #13931)	Do we need to specify the variables again? I guess that `assert_eq`'s message will contain the values.

varun accepted this revision.Jun 29 2022, 3:32 PM

This revision is now accepted and ready to land.Jun 29 2022, 3:32 PM

retrigger CI

use functions with scope

Harbormaster completed remote builds in B10149: Diff 14019.Jun 30 2022, 7:57 AM

Harbormaster failed remote builds in B10161: Diff 14031!Jun 30 2022, 8:42 AM

Harbormaster completed remote builds in B10161: Diff 14031.Jun 30 2022, 8:47 AM

• karol added inline comments.Jul 8 2022, 12:22 AM

services/commtest/tests/backup_test.rs
83–88 ↗	(On Diff #13931)	I've been using this pattern all around. For me, it's easier to deduce what exactly went wrong (expected vs got sounds better than left vs right). I also do not think this is harmful whatsoever. If you still feel like this isn't good, feel free to create a task for this. I can change it but testing is way simpler for me when I see such a message.