-
Notifications
You must be signed in to change notification settings - Fork 172
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix matching_status_* test failures #662
Fix matching_status_* test failures #662
Conversation
zenoh/tests/matching.rs
Outdated
|
||
let publisher1 = ztimeout!(session1 | ||
.declare_publisher("zenoh_matching_status_any_test") | ||
.allowed_destination(Locality::Any) | ||
.res_async()) | ||
.unwrap(); | ||
.res_async())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of tests it's better to use unwrap()
because the error message will tell exactly at which line the test has panicked. If ?
is used, it's then harder to identify which is the line that fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. Then I understand why we have our customized zerror since it prints more information than common errors.
#[cfg(feature = "unstable")] | ||
#[test] | ||
fn zenoh_matching_status_any() { | ||
fn zenoh_matching_status_any() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on my comment below on unwrap()
, there is no need to return a Result
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
zenoh/tests/matching.rs
Outdated
}; | ||
let config2 = zenoh::config::client([Locator::from_str(locator)?]); | ||
|
||
let session1 = ztimeout!(zenoh::open(config1).res_async())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use unwrap()
instead of ?
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments above.
@Mallets I've reverted the changes, can you help review this? |
3357fef
to
a1fdb49
Compare
Since we don't avoid the multicast port usage while testing. Sometimes the CI test on the main branch would fail due to the conflict of
cargo nextest run --exclude zenoh-examples --exclude zenoh-plugin-example --workspace -E 'test(=zenoh_session_multicast) | test(zenoh_matching_status)'
The same error happens frequently on the tokio branch. That's why we found it. And it's better to have
Result
to verbose the failure.