-
Notifications
You must be signed in to change notification settings - Fork 451
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
Code changes for the new Clippy version and running test on Windows #1704
Conversation
@@ -1,3 +1,5 @@ | |||
#![cfg(unix)] |
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.
These tests use use std::os::unix::fs::PermissionsExt
which cannot be used on Windows.
@@ -435,9 +435,11 @@ mod tests { | |||
}) | |||
.expect("callback registration should succeed"); | |||
|
|||
_ = meter_provider.force_flush(); |
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.
why force_flush
here?
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.
Just to trigger the callback code deterministically. This lets us assert things on the receiver end with certainty instead of waiting for interval * 2
milliseconds and hoping that the sender sends the data in that time.
@@ -129,6 +129,7 @@ impl CollectorHttpClient { | |||
} | |||
|
|||
#[cfg(test)] | |||
#[allow(dead_code)] |
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.
This is to suppress the clippy warning for the struct not being constructed. Check this for more details: https://github.com/open-telemetry/opentelemetry-rust/actions/runs/8943870592/job/24569519425?pr=1704
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.
I think the issue is that this module test_http_client
is always defined, while used only when the feature rt-tokio
is enabled. Should we define this module only when the rt-tokio
is enabled?
#[cfg(test)]
#[cfg(feature = "rt-tokio")]
pub(crate) mod test_http_client {
..
}
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.
That's a good idea. Covered this in #1711
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.
LGTM apart from the nit comment. Thanks for fixing the rust toolchain upgrade issues also :)
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.
👍 Thanks for the fix
Changes
There's a newer version of the rust toolchain which brings in newer clippy lints:
Fix some tests to resolve the complaints from
cargo test
on WindowsAfter these changes, the commands specified in the test.sh file run successfully on Windows: