Page MenuHomePhabricator

[DRAFT] [services] Tests - Add main
AbandonedPublic

Authored by karol on May 31 2022, 6:51 AM.
Tags
None
Referenced Files
F3299489: D4167.diff
Sun, Nov 17, 12:38 PM
Unknown Object (File)
Thu, Nov 14, 10:26 AM
Unknown Object (File)
Thu, Nov 14, 10:26 AM
Unknown Object (File)
Thu, Nov 14, 10:26 AM
Unknown Object (File)
Thu, Nov 14, 10:26 AM
Unknown Object (File)
Thu, Nov 14, 10:26 AM
Unknown Object (File)
Thu, Nov 14, 10:26 AM
Unknown Object (File)
Thu, Nov 14, 10:26 AM

Details

Reviewers
tomek
varun
ashoat
Summary

Depends on D4227

Finally, adding the main script. It connects to the local backup service, tries to add a new backup, and then, adds a new log.
Of course, this is a super-early alfa version. In the future, I'm planning to isolate tests for separate services, so for each service, we're going to have a different folder, etc.

Test Plan

This doesn't yet work with docker, I had problems with connecting to other services from rust code run in another container.
You can still run this locally building the code on the host machine:

cd services/commtest
COMM_TEST_TARGET=backup cargo run

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 6:58 AM
Harbormaster failed remote builds in B9546: Diff 13240!
karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, ashoat, varun.
Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 7:25 AM
Harbormaster failed remote builds in B9553: Diff 13247!
Harbormaster returned this revision to the author for changes because remote builds failed.May 31 2022, 7:29 AM
Harbormaster failed remote builds in B9558: Diff 13252!

I know that Ci failed but it's related to docker which I know is not working properly right now.

I don't feel confident enough to review Rust :(

tomek requested changes to this revision.Jun 1 2022, 7:17 AM
tomek added inline comments.
services/commtest/build.rs
6–12 ↗(On Diff #13252)

This should depend on e.g. an env flag.

services/commtest/src/main.rs
20–21 ↗(On Diff #13252)

Not sure about good practices, but we don't want to mutate them so maybe &str makes more sense? Also, I think it is not necessary to have type annotations here.

This revision now requires changes to proceed.Jun 1 2022, 7:17 AM
services/commtest/build.rs
6–12 ↗(On Diff #13252)

It does, target_service keeps a value from an env flag.

services/commtest/src/main.rs
20–21 ↗(On Diff #13252)

I can remove annotations, just wanted to have them explicit here. I couldn't make it compile using &str unfortunately. I had issues with lifetimes. Maybe @varun can help with this?

varun requested changes to this revision.Jun 7 2022, 8:13 AM
varun added inline comments.
services/commtest/src/main.rs
3–5

You can remove this since you already have it in backup_utils

7

this doesn't need to be pub i think

20
23
27
29

Create a new Error type as discussed in this diff

This revision now requires changes to proceed.Jun 7 2022, 8:13 AM
services/commtest/src/main.rs
3–5

right, thx

7

yes

tomek requested changes to this revision.Jun 8 2022, 6:20 AM
tomek added inline comments.
services/commtest/src/main.rs
21 ↗(On Diff #13398)

Is this url service-specific? Maybe we should use different ports based on the service?

services/commtest/src/send_log.rs
7–13 ↗(On Diff #13398)

The formatting should be performed in a diff that introduced these

services/commtest/src/main.rs
21 ↗(On Diff #13398)

Definitely. This is a temp string so just this service works. Once we're good with this, we can parameterize this.