[Tunnelbroker] Add integration tests
ClosedPublic
Actions

Authored by • jon on Apr 27 2023, 7:21 AM.

Details

Reviewers

varun
bartek

Commits

rCOMM2bead14e6f9c: [Tunnelbroker] Add integration tests

Summary

For some features, it's easier to just have a unittest demonstrate
the expected behavior. This sets up that framework, and modifies
the existing code to make it more explicit as to what is going on
when debugging.

Depends on D7659

Test Plan

(cd services/tunnelbroker && RUST_LOG=debug cargo run &)
cd services/commtest && cargo test --test tunnelbroker_integration_test

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• jon created this revision.Apr 27 2023, 7:21 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald TranscriptApr 27 2023, 7:21 AM

Harbormaster completed remote builds in B18867: Diff 25845.Apr 27 2023, 7:39 AM

• jon requested review of this revision.Apr 27 2023, 7:39 AM

• jon edited the summary of this revision. (Show Details)Apr 27 2023, 8:55 AM

bartek added inline comments.Apr 29 2023, 9:51 AM

services/commtest/tests/tunnelbroker_integration_test.rs
4–5 ↗	(On Diff #25845)	I know this is a "hello world" test, but just two considerations for future design: Is this function intended to be a test? Or a utility to open a connection? Isn't it better to keep the `anyhow::Result<()>` return type? Anyway, it's an easy change if ever needed. When opening a connection for tests, panicking makes sense
services/tunnelbroker/src/websockets/mod.rs
70 ↗	(On Diff #25845)	I'd consider it `trace!` because it feels redundant as there's already a debug log in line 45

bartek accepted this revision.Apr 29 2023, 11:38 AM

This revision is now accepted and ready to land.Apr 29 2023, 11:38 AM

• jon marked 2 inline comments as done.Apr 30 2023, 10:28 PM

• jon added inline comments.

services/commtest/tests/tunnelbroker_integration_test.rs
4–5 ↗	(On Diff #25845)	Is this function intended to be a test? Or a utility to open a connection? It is meant to be a test. It's easier for me to iterate with rust than JS. So I've been implementing it first in rust to just to ensure that the rust is working before trying to debug stuff in JS. Isn't it better to keep the anyhow::Result<()> return type? The test is meant to a be a happy case, if it panics or returns an Err it will still fail the test. I don't feel super strongly about this, and might make the transition in the future to use anyhow. Still not sure if anyhow or retaining the original error is more beneficial
services/tunnelbroker/src/websockets/mod.rs
70 ↗	(On Diff #25845)	line 45 doesn't get executed until a message is received, where as this gets emitted right before it begins polling for an event.