Copilot commented on code in PR #514:
URL: https://github.com/apache/avro-rs/pull/514#discussion_r3010017311
##########
avro_test_helper/src/lib.rs:
##########
@@ -71,4 +75,5 @@ pub type TestResult = Result<(), TestError>;
/// Does nothing. Just loads the crate.
/// Should be used in the integration tests, because they do not use
[dev-dependencies]
/// and do not auto-load this crate.
-pub fn init() {}
+#[inline]
+pub const fn init() {}
Review Comment:
`init()` is documented as a way to “just load the crate” so ctors run in
integration tests. Making it `#[inline] pub const fn init() {}` increases the
chance the call gets inlined/optimized away, which can defeat the purpose of
forcing the crate to be linked/loaded for its side effects. Consider keeping
this as a non-`const` non-inlined function (e.g., remove `const`/`#[inline]`,
or use `#[inline(never)]`) so calling `init()` reliably pulls in the crate.
```suggestion
#[inline(never)]
pub fn init() {}
```
##########
avro_test_helper/src/logger.rs:
##########
@@ -53,47 +53,89 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages of this thread.
pub fn clear_log_messages() {
- LOG_MESSAGES.with(|msgs| match msgs.try_borrow_mut() {
- Ok(mut log_messages) => log_messages.clear(),
- Err(err) => panic!("Failed to clear log messages: {err:?}"),
- });
+ LOG_MESSAGES.with_borrow_mut(Vec::clear);
}
+/// Asserts that the message is not the last in the log for this thread.
+///
+/// # Panics
+/// Will panic if the provided message is an exact match for the last log
message.
+///
+/// # Example
+/// ```should_panic
+/// use apache_avro_test_helper::logger::assert_not_logged;
+///
+/// log::error!("Something went wrong");
+/// log::error!("Unexpected Error");
+///
+/// // This will not panic as it is not an exact match
+/// assert_not_logged("No Unexpected Error");
+///
+/// // This will not panic as it is not the last log message
+/// assert_not_logged("Something went wrong");
+///
+/// // This will panic
+/// assert_not_logged("Unexpected Error");
+/// ```
#[track_caller]
pub fn assert_not_logged(unexpected_message: &str) {
- LOG_MESSAGES.with(|msgs| match msgs.borrow().last() {
- Some(last_log) if last_log == unexpected_message => {
- panic!("The following log message should not have been logged:
'{unexpected_message}'")
- }
- _ => (),
+ LOG_MESSAGES.with_borrow(|msgs| {
+ let is_logged = msgs.iter().any(|msg| msg == unexpected_message);
+ assert!(
+ !is_logged,
+ "The following log message should not have been logged:
'{unexpected_message}'"
Review Comment:
`assert_not_logged` now checks whether the message appears anywhere in this
thread’s log (`iter().any(...)`), but the docstring and example still
describe/assume “only the last log message”. This makes the docs misleading
(and the example’s explanation is now wrong). Update the docs (and the example
narrative) to match the current semantics, or revert the implementation to a
`last()` check if that behavior was intended.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
