Change log level of methods name logged in Scaffolding #2136
Conversation
| @@ -1,6 +1,7 @@ | |||
| [package] | |||
| name = "uniffi-fixture-logging-callback-interface" | |||
| edition = "2021" | |||
| authors = ["Firefox Sync Team <[email protected]>", "Viktor Jansson <[email protected]>"] | |||
There was a problem hiding this comment.
Please let me know if you want me to revert this change. I git blamed and saw that Ben has contributed previously, let me know if you want me to add him as well (I think Mark is a member of the sync team?).
There was a problem hiding this comment.
Ben's also in the sync team - we'd also be up for removing this line entirely, but until then I think this is fine.
| @@ -6,7 +6,7 @@ use std::sync::{Once, RwLock}; | |||
|
|
|||
| // Logger trait that the foreign code implements | |||
| pub trait Logger: Sync + Send { | |||
| fn log_message(&self, message: String); | |||
| fn log_message(&self, message: String, level: String); | |||
There was a problem hiding this comment.
String is more clear than u8, and seems like overkill introducing a UniFFI exported Enum.
There was a problem hiding this comment.
The actual fix of #1702
There was a problem hiding this comment.
Will make sense when you read test.py...
| @@ -1,6 +1,7 @@ | |||
| [package] | |||
| name = "uniffi-fixture-logging-callback-interface" | |||
| edition = "2021" | |||
| authors = ["Firefox Sync Team <[email protected]>", "Viktor Jansson <[email protected]>"] | |||
There was a problem hiding this comment.
Ben's also in the sync team - we'd also be up for removing this line entirely, but until then I think this is fine.
| @@ -285,7 +285,7 @@ pub(super) fn gen_ffi_function( | |||
| #[doc(hidden)] | |||
| #[no_mangle] | |||
| pub extern "C" fn #ffi_ident(#(#param_names: #param_types,)*) -> ::uniffi::Handle { | |||
| ::uniffi::deps::log::debug!(#name); | |||
| ::uniffi::deps::log::trace!(#name); | |||
There was a problem hiding this comment.
I guess I don't really understand why most of the above changes - ie, I'm not sure how exactly the fixture changes really ensures these 2 lines actually work. Or to put it yet another way, I think I'd r+ this with just these 2 lines.
There was a problem hiding this comment.
The fixture does not assert one of the changed to trace, the one for async functions.
The logger in the fixture is set to TRACE, and the test asserts that the Method names - the log statement: uniffi::deps::log::trace!(#name); is indeed logged (present in the array of messages - which it also would before this PRs change from debug!(#name).
But then later in the test, the used loggers level is set to debug, and the method name of the subsequent call is not printed (is not in messages). Which it would have been without this PRs change.
So if an async method would be included then this PR, would test the affected source code in full, right?
There was a problem hiding this comment.
I'm not quite sure what you are saying there, but my point is that unless I'm still missing something, I think the test code here is testing the log crate rather than the uniffi crate?
There was a problem hiding this comment.
The changed code in the test piggybacks on an existing test, but it does indeed test uniffi crate.
It tests the changed UniFFI source code. Without the changed source code the test would fail.
The test ensures the level at which UniFFI source code logs name of methods is trace.
There was a problem hiding this comment.
Without the changed source code the test would fail.
I hope it's clear that I do infact understand that.
There was a problem hiding this comment.
@Vinnstah I would benefit from this PR getting landed :D
Sounds like Mark would prefer if the changes in the test be reverted, maybe you can revert and this might get merged :)
There was a problem hiding this comment.
To be clear, I'm only weakly against tests for this - IMO the value is lower than the cost. But I'm open to be convinced otherwise, and it's difficult to argue too strongly against new tests really :)
|
Hi, is there any interest in removing some of the test code here? If I don't hear back soon, I'll put up the 2-line PR that actually fixes this and close this, but I'd like to offer the opportunity to have it completed here. |
|
Hi Mark, |
|
thank you for this! @Vinnstah ! |
Fixes #1702
Change the log level to trace for method names logged by UniFFI internals when FFI calls into Rust.
Augmented existing logging tests in Python, asserting that the changed level works as intended.
Let me know if you want me to put the tests in a new fixture instead of recycling an existing one.