Flying-Toast / simple-websockets

MIT License
18 stars 7 forks source link

Added tests for send and receive #17

Closed StephanMoeller closed 1 year ago

StephanMoeller commented 1 year ago

I have added initial tests prior to changing/adding any new features. I added a test framework that allows tests for being run with "cargo nextest run" and show test result in a more concise manner compared to "cargo test".

Flying-Toast commented 1 year ago

I left a few comments. When I run the tests, all 5 are failing:

---- tests::connect_disconnect_test stdout ----
thread 'tests::connect_disconnect_test' panicked at 'Can't connect: Url(UnableToConnect("ws://127.0.0.1:9000/"))', tests/tests.rs:16:82

---- tests::send_binary_message_test stdout ----
thread 'tests::send_binary_message_test' panicked at 'Can't connect: Url(UnableToConnect("ws://127.0.0.1:9004/"))', tests/tests.rs:146:65

---- tests::receive_binary_message_test stdout ----
thread 'tests::receive_binary_message_test' panicked at 'Can't connect: Url(UnableToConnect("ws://127.0.0.1:9002/"))', tests/tests.rs:84:69
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- tests::receive_text_message_test stdout ----
thread 'tests::receive_text_message_test' panicked at 'Can't connect: Url(UnableToConnect("ws://127.0.0.1:9001/"))', tests/tests.rs:55:69

---- tests::send_text_message_test stdout ----
thread 'tests::send_text_message_test' panicked at 'Can't connect: Url(UnableToConnect("ws://127.0.0.1:9003/"))', tests/tests.rs:114:65
StephanMoeller commented 1 year ago

Ohhhh, what environment do you run it in? As it is the connect step, I guess the bind went fine, so my guess it is a local firewall not allowing for connecting to localhost? Maybe using "localhost" may work over "127.0.0.1" - could you give that a try?

Flying-Toast commented 1 year ago

Hmm, doesn't seem to be the problem. localhost doesn't work in the test either. The server does get started though, I can confirm I can netcat localhost {UNIQUE_TEST_PORT}.

StephanMoeller commented 1 year ago

What OS/Build environment do you execute the cargo test command in? As a narrow-down process, does it work if you disable your firewall?

Flying-Toast commented 1 year ago

Ah, the issue is that the tests are trying to connect before the thread running the server has had time to start up. For now, adding a short delay after all the calls to launch() in the tests will fix it - things seem to be working for me with a thread::sleep(500ms) after launch().

StephanMoeller commented 1 year ago

Ohhh, great you found the reason! Even though sleeping is never 100% stable, as one day the bind may take longer than that, for 99.9% of the time it will be sufficient, I guess. And with a low number of tests, I hope the total run time is acceptable?

Should I add the sleep to the PR?

Flying-Toast commented 1 year ago

Yeah, lets add the sleep to the PR. I've been planning some rework on simple-websockets' API for a while that would remove a lot of these rough edges, but until then let's just leave the delay :)

Flying-Toast commented 1 year ago

Looks good now. Thanks :)