Various test clean ups#1145
Conversation
|
It seems that Azure pipelines rebases the pr on master, which had an additional commit compare to my local branch. That is why the CI is failing. |
Thomasdezeeuw
force-pushed
the
test-cleanup
branch
from
dbbdca9 to
d8f618e
Compare
|
After a rebase on master the CI is green now. |
kleimkuhler
left a comment
kleimkuhler
left a comment
There was a problem hiding this comment.
I like a lot of what this PR does, but I think the assert_... macros provide a lot of value in testing; I'm not sure removing these is the best path forward. For example, dealing with test errors with the macro:
failures:
---- unix_stream_try_clone stdout ----
thread 'unix_stream_try_clone' panicked at 'assertion failed: error = Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }', tests/unix_stream.rs:166:16
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
failures:
unix_stream_try_clone
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 13 filtered out
without the macro:
failures:
---- unix_stream_try_clone stdout ----
thread 'unix_stream_try_clone' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 35, kind: WouldBlock, message: "Resource temporarily unavailable" }', src/libcore/result.rs:1165:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
failures:
unix_stream_try_clone
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 13 filtered out
With the macro, the failure output provides the exact line in the source where the error occurred. With only unwrap, you don't know which unwrap caused the failure in the test. For tests that have a lot of unwraps this can be difficult to hunt down.
Overall, I really like the addition of expect_read! and expect_write!! Thanks for consolidating a lot of those assertions.
|
@kleimkuhler I agree that when using I also fixed the |
| } | ||
| } | ||
|
|
||
| impl From<Interests> for Readiness { |
There was a problem hiding this comment.
I like this! The Mio update in Tokio could probably use something similar instead of the explicit checks that are there.
|
@Thomasdezeeuw Even with There are going to be some conflicts with the UDS tests getting split up, but I think most will just be replacing the macros with |
The unwrap function has the same effect.
The unwrap_err function has the same effect.
Used in the ExpectEvent structure.
Removing expect_readiness completely. It didn't use the token so it was unclear what event should have read/write closed indicators.
This also removes the echo_server test from the tcp module. All functionality is tested elsewhere and this test is just too complex to attempt to fix (it used the TryRead and TryWrite traits).
Some places checked the return value after a write, but all. This ensure that all write, send and send_to calls check the return value.
Testing macro that calls read/recv/peek or recv_from/peek_from and
checks if the received bytes are expected. In case of {recv,peek}_from
it also checks the source address.
Thomasdezeeuw
force-pushed
the
test-cleanup
branch
from
ab12e60 to
9025e98
Compare
|
Rebased this on master @kleimkuhler can you take another look? I'll to merge this quickly as rebasing this is a pain in the.. you know what :) |
kleimkuhler
left a comment
kleimkuhler
left a comment
There was a problem hiding this comment.
Looks good! Thanks for dealing with the rebase and getting these all cleaned up.
3 participants
Various test clean ups. Easiest to review by commit.
Updates #995