matrix-org / matrix-ircd

An IRCd implementation backed by Matrix.
Apache License 2.0
224 stars 41 forks source link

GSOC 2020: Add additional test coverage for matrix / http implementations #66

Closed VanillaBrooks closed 4 years ago

VanillaBrooks commented 4 years ago

This pr adds some additional tests to the matrix module to ensure that the correct http requests are being called with each. The overall test coverage is approximately doubled with these additions.

phlmn commented 4 years ago

I when I run the tests, sometimes there are warnings like Mock response write error: Broken pipe (os error 32). Any ideas on that?

cargo test
   Compiling matrix-ircd v0.1.0 (/Users/phlmn/Coding/matrix-ircd)
    Finished test [unoptimized + debuginfo] target(s) in 5.06s
     Running target/debug/deps/matrix_ircd-f7cb6d6fcfd3be9d

running 15 tests
test http::tests::basic_response ... ok
test http::tests::basic_chunked_response ... ok
test http::tests::basic_length_response ... ok
test http::tests::chunked_response_missing_last_newline ... ok
test http::tests::chunked_response_slow ... ok
test http::tests::mutliple ... ok
test http::tests::chunked_response_slow_block ... ok
test irc::protocol::tests::simple_numeric ... ok
test irc::protocol::tests::simple_nick ... ok
test irc::protocol::tests::simple_prefix ... ok
test irc::protocol::tests::simple_user ... ok
test matrix::protocol::tests::sync_response ... ok
test matrix::tests::send_text_message ... ok
warning: Mock response write error: Broken pipe (os error 32)
test matrix::tests::matrix_login ... ok
test matrix::sync::tests::matrix_sync_request ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
jplatte commented 4 years ago

when I run the tests, sometimes there are warnings like Mock response write error: Broken pipe (os error 32).

Sounds like the mock server is writing an HTTP response but the other end (the main things the tests executes) doesn't read the (full) response and closes the connection early.

Since tests (probably?) run in parallel, it would be nice to figure out which test exactly is causing this (one can filter tests by running cargo test something to only run tests whose names contain something).

VanillaBrooks commented 4 years ago

Since tests (probably?) run in parallel, it would be nice to figure out which test exactly is causing this

cargo test does run in parallel. running with cargo test -- --test-threads=1 still produces the problem (as does running the tests individually). I dont see any relevant errors in the mockito repo, and I am unable to reproduce the error (after several hours) with hyper which leads me to believe its a problem in http.rs.

The warning is also not present in the futures 0.3 code with pure hyper.

jplatte commented 4 years ago

running with cargo test -- --test-threads=1 still produces the problem (as does running the tests individually).

So you can reproduce the same error message with any of the tests or what do you mean? I previously thought it was one specific test that triggered this.

VanillaBrooks commented 4 years ago

So you can reproduce the same error message with any of the tests or what do you mean? I previously thought it was one specific test that triggered this.

Each test using mockito / http mocking will trigger the warning.

$cargo test matrix_sync_request 

running 1 test
warning: Mock response write error: Broken pipe (os error 32)
test matrix::sync::tests::matrix_sync_request ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 14 filtered out
$cargo test matrix_login

running 1 test
warning: Mock response write error: Broken pipe (os error 32)
test matrix::tests::matrix_login ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 14 filtered out
$ cargo test send_text_message

running 1 test
warning: Mock response write error: Broken pipe (os error 32)
warning: Mock response write error: Broken pipe (os error 32)
test matrix::tests::send_text_message ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 14 filtered out

I am not sure why send_text_message produces two warnings.

jplatte commented 4 years ago

Okay, let's merge this then if it doesn't prevent the tests from passing and there's a way towards getting rid of the warnings.