[services] Tests - Add dummy tests
ClosedPublic
Actions

Authored by • karol on Jun 13 2022, 6:51 AM.

Details

Reviewers

tomek
varun

Commits

rCOMMd0b8ddc64eb4: [services] Tests - Add dummy tests

Summary

Depends on D4249

Added the rust files for tests where they're supposed to be placed. For now, hardcoded throwing unimplemented but it's supposed to be replaced with tests' logic.

Test Plan

none for now

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• karol created this revision.Jun 13 2022, 6:51 AM

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

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

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

• karol added reviewers: tomek, varun.

• karol added a parent revision: D4249: [services] Tests - Update packages.

• karol added a child revision: D4216: [services] Tests - Fix scripts.

Harbormaster returned this revision to the author for changes because remote builds failed.Jun 13 2022, 10:05 AM

Harbormaster failed remote builds in B9726: Diff 13471!

CI failed on an unrelated stage - key server
CI services build - no space left on device

Harbormaster completed remote builds in B9726: Diff 13471.Jun 14 2022, 3:24 AM

tomek accepted this revision.Jun 14 2022, 10:50 AM

tomek added inline comments.

services/commtest/tests/backup_test.rs
7 ↗	(On Diff #13471)	Do tests need to return value? If assertions were not violated, we could assume that it passed

varun accepted this revision.Jun 15 2022, 8:51 AM

varun added inline comments.

services/commtest/tests/backup_test.rs
7 ↗	(On Diff #13471)	yeah we shouldn't return anything
services/commtest/tests/blob_test.rs
7 ↗	(On Diff #13471)	don't return anything
services/commtest/tests/tunnelbroker_test.rs
7 ↗	(On Diff #13471)	don't return anything

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

Returning Result comes in handy because we may use the ? operator. This is goingto be used in every test as we need to connect to the service and connecting functions use ? as well. So, technically, we could remove the return type and lazily add it when the test bodies are added but I don't think this is harmless or expensive.

Do tests need to return value? If assertions were not violated, we could assume that it passed

They do not need to return value, notice that we return () which is void. That's wrapped into result only because we want to handle errors nicely with the ? operator.

If an error occurs, the test fails.

I'd not change a thing here but I see you two agree on this one so if you still think we should change this, then let me know and I will change but please note that I'm most probably going to add these return types in the diffs that add the tests' bodies.

In D4250#121224, @karol-bisztyga wrote:

Returning Result comes in handy because we may use the ? operator. This is goingto be used in every test as we need to connect to the service and connecting functions use ? as well. So, technically, we could remove the return type and lazily add it when the test bodies are added but I don't think this is harmless or expensive.

Do tests need to return value? If assertions were not violated, we could assume that it passed

They do not need to return value, notice that we return () which is void. That's wrapped into result only because we want to handle errors nicely with the ? operator.

If an error occurs, the test fails.

I'd not change a thing here but I see you two agree on this one so if you still think we should change this, then let me know and I will change but please note that I'm most probably going to add these return types in the diffs that add the tests' bodies.

According to https://doc.rust-lang.org/book/ch11-01-writing-tests.html#using-resultt-e-in-tests it is fine to return a result, but I feel that a tests should return a result only when necessary. These empty tests don't need to return it, so we shouldn't do that.