Skip to content
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

Enhance XCM Debugging with Log Capture in Unit Tests #7594

Open
wants to merge 37 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
624c15a
Assert on captured logs on XCM Simulator
raymondkfcheung Feb 13, 2025
513b177
Remove tests for DispatchError
raymondkfcheung Feb 17, 2025
489247b
Revert Cargo.toml
raymondkfcheung Feb 17, 2025
b52a1e6
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 17, 2025
ce94bff
Add log_capture
raymondkfcheung Feb 17, 2025
9bd8d15
Add docs for LogCapture
raymondkfcheung Feb 17, 2025
58f22d5
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 18, 2025
7db41c4
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 18, 2025
bd00fb3
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 19, 2025
6500072
Add tracing-test
raymondkfcheung Feb 19, 2025
c4572f2
Add changes
raymondkfcheung Feb 19, 2025
286cd2f
Add changes
raymondkfcheung Feb 19, 2025
b8aa377
Revert changes
raymondkfcheung Feb 19, 2025
ca1da9b
Move LogCapture to sp_tracing
raymondkfcheung Feb 19, 2025
1e82b9e
Remove unused imports
raymondkfcheung Feb 19, 2025
bcb56d5
Add use sp_tracing back
raymondkfcheung Feb 19, 2025
617fced
Simplify log capture
raymondkfcheung Feb 19, 2025
4a37d17
Remove unused imports
raymondkfcheung Feb 19, 2025
7dd0846
Add doc tests
raymondkfcheung Feb 19, 2025
17ff9ee
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 19, 2025
4ff842f
Update from raymondkfcheung running command 'prdoc --audience runtime…
github-actions[bot] Feb 20, 2025
5c44734
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 20, 2025
24c09af
Rename to test_log_capture
raymondkfcheung Feb 20, 2025
c5c4050
Update docs
raymondkfcheung Feb 20, 2025
e8c40c0
Update tests
raymondkfcheung Feb 20, 2025
ab31f4b
Update PR doc
raymondkfcheung Feb 20, 2025
f0d0d56
Update tests
raymondkfcheung Feb 20, 2025
595de4c
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 20, 2025
fed1747
Change default max_level to DEBUG
raymondkfcheung Feb 20, 2025
5d50f0f
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 20, 2025
01d8548
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 20, 2025
d93c6e1
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 20, 2025
654e004
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 21, 2025
e709419
Update from raymondkfcheung running command 'fmt'
github-actions[bot] Feb 21, 2025
2411944
Update PR doc
raymondkfcheung Feb 21, 2025
d1fc37f
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 21, 2025
4aff24f
Merge branch 'master' into ray-assert-xcm-logs
raymondkfcheung Feb 21, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,7 @@ tracing-core = { version = "0.1.32", default-features = false }
tracing-futures = { version = "0.2.4" }
tracing-log = { version = "0.2.0" }
tracing-subscriber = { version = "0.3.18" }
tracing-test = { version = "0.2.5", default-features = false }
tracking-allocator = { path = "polkadot/node/tracking-allocator", default-features = false, package = "staging-tracking-allocator" }
trie-bench = { version = "0.39.0" }
trie-db = { version = "0.29.1", default-features = false }
Expand Down
74 changes: 74 additions & 0 deletions polkadot/xcm/xcm-simulator/example/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,28 @@ fn reserve_transfer() {
relay_chain::Balances::free_balance(&child_account_id(1)),
INITIAL_BALANCE + withdraw_amount
);
// Ensure expected events were emitted
let events = relay_chain::System::events();
let attempted_count = events
.iter()
.filter(|e| {
matches!(
e.event,
relay_chain::RuntimeEvent::XcmPallet(pallet_xcm::Event::Attempted { .. })
)
})
.count();
let sent_count = events
.iter()
.filter(|e| {
matches!(
e.event,
relay_chain::RuntimeEvent::XcmPallet(pallet_xcm::Event::Sent { .. })
)
})
.count();
Comment on lines +141 to +158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we plan to count the number of logs in other places as well it would be worth deduplicating this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

assert_eq!(attempted_count, 1, "Expected one XcmPallet::Attempted event");
assert_eq!(sent_count, 1, "Expected one XcmPallet::Sent event");
});

ParaA::execute_with(|| {
Expand All @@ -147,6 +169,58 @@ fn reserve_transfer() {
});
}

#[test]
fn reserve_transfer_with_error() {
use sp_tracing::{test_log_capture, tracing::Level};

// Reset the test network
MockNet::reset();

// Execute XCM Transfer and Capture Logs
test_log_capture::capture_with_max_level(Level::ERROR, || {
let invalid_dest = Box::new(Parachain(9999).into());
let withdraw_amount = 123;

Relay::execute_with(|| {
let result = RelayChainPalletXcm::limited_reserve_transfer_assets(
relay_chain::RuntimeOrigin::signed(ALICE),
invalid_dest,
Box::new(AccountId32 { network: None, id: ALICE.into() }.into()),
Box::new((Here, withdraw_amount).into()),
0,
Unlimited,
);

// Ensure an error occurred
assert!(result.is_err(), "Expected an error due to invalid destination");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Could we move the assert!(test_log_capture::logs_contain("XCM validate_send failed")); after this line ? I think it would make sense to check if the logged error is the one that we expected right here, after we know that we got an error.


// Verify that XcmPallet::Attempted was NOT emitted (rollback happened)
let events = relay_chain::System::events();
let xcm_attempted_emitted = events.iter().any(|e| {
matches!(
e.event,
relay_chain::RuntimeEvent::XcmPallet(pallet_xcm::Event::Attempted { .. })
)
});
assert!(
!xcm_attempted_emitted,
"Expected no XcmPallet::Attempted event due to rollback, but it was emitted"
);
});

// Ensure no balance change due to the error
ParaA::execute_with(|| {
assert_eq!(
pallet_balances::Pallet::<parachain::Runtime>::free_balance(&ALICE),
INITIAL_BALANCE
);
});
});

// Assertions on Captured Logs
assert!(test_log_capture::logs_contain("XCM validate_send failed"));
}

#[test]
fn remote_locking_and_unlocking() {
MockNet::reset();
Expand Down
12 changes: 12 additions & 0 deletions prdoc/pr_7594.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
title: Improve XCM Debugging by Capturing Logs in Unit Tests
doc:
- audience: Runtime Dev
description: |-
This PR introduces a lightweight log-capturing mechanism for XCM unit tests, making it easier to troubleshoot failures when needed.
crates:
- name: pallet-xcm
bump: patch
- name: xcm-simulator-example
bump: patch
- name: sp-tracing
bump: patch
2 changes: 2 additions & 0 deletions substrate/primitives/tracing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ tracing-subscriber = { workspace = true, optional = true, features = [
"time",
"tracing-log",
] }
tracing-test = { workspace = true, optional = true }

[features]
default = ["std"]
Expand All @@ -38,5 +39,6 @@ std = [
"tracing-core/std",
"tracing-subscriber",
"tracing/std",
"tracing-test",
"with-tracing",
]
110 changes: 110 additions & 0 deletions substrate/primitives/tracing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,3 +255,113 @@ macro_rules! enter_span {
$crate::enter_span!($crate::span!($lvl, $name))
};
}

/// `tracing-test` can be used for general logging capture but doesn't support logging within
/// scoped functions like `Relay::execute_with`.
///
/// # Example
///
/// ```
/// use tracing_test::traced_test;
/// use tracing::info;
///
/// #[traced_test]
/// fn test_logging() {
/// info!("This is a test log message");
/// assert!(logs_contain("test log message"));
/// }
/// ```
///
/// For more details, see [`tracing-test`](https://crates.io/crates/tracing-test).
#[cfg(feature = "std")]
pub use tracing_test;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this inside the test_log_capture mod. Also I wouldn't add examples related to this. There should be examples in the tracing-test crate.

Copy link
Contributor Author

@raymondkfcheung raymondkfcheung Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. Maybe we can remove tracing-test and implement our own log capture logic, as discussed here.


/// A module for capturing and asserting logs in tests.
///
/// This module provides utilities for capturing logs during test execution
/// and verifying expected log output.
///
/// # Example
///
/// ```
/// use sp_tracing::test_log_capture;
///
/// test_log_capture::capture(|| {
/// tracing::info!("This is a test log message");
/// });
///
/// assert!(test_log_capture::logs_contain("test log message"));
/// ```
#[cfg(feature = "std")]
pub mod test_log_capture {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would guard this under a test feature as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

use tracing::subscriber;
use tracing_core::LevelFilter;
use tracing_test::internal::{global_buf, MockWriter};

/// Runs a test block with logging enabled and captures logs for assertions.
///
/// This function sets up a `tracing_subscriber` that redirects logs
/// to an internal buffer, which can later be queried.
///
/// # Example
///
/// ```
/// use sp_tracing::test_log_capture;
///
/// test_log_capture::capture(|| {
/// tracing::warn!("Captured warning message");
/// });
///
/// assert!(test_log_capture::logs_contain("Captured warning message"));
/// ```
pub fn capture<F: FnOnce()>(f: F) {
capture_with_max_level(LevelFilter::DEBUG, f);
}

/// Runs a test block with logging enabled and captures logs for assertions
/// while allowing the maximum log level to be specified.
///
/// # Example
///
/// ```
/// use sp_tracing::test_log_capture;
/// use sp_tracing::tracing::Level;
///
/// test_log_capture::capture_with_max_level(Level::INFO, || {
/// tracing::info!("This will be captured at INFO level");
/// tracing::debug!("This will be captured at DEBUG level");
/// });
///
/// assert!(test_log_capture::logs_contain("INFO level"));
/// assert!(!test_log_capture::logs_contain("DEBUG level"));
Comment on lines +330 to +336
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played with this a bit and in my experiments indeed it works when I do tracing::info, but it doesn't work with log::info. I think a lot of the logs are emitted using log:: methods.

Maybe I'm doing something wrong. But just saying in case it's worth looking into.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven’t tested with log, but tracing seems to be the preferred way to log messages. To keep it simple, we can focus on tracing for now.

/// ```
pub fn capture_with_max_level<F: FnOnce()>(max_level: impl Into<LevelFilter>, f: F) {
let log_capture = MockWriter::new(&global_buf());
Comment on lines +338 to +339
Copy link
Contributor

@serban300 serban300 Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think it would be a bit more flexible if we avoided using a closure and if we avoided using a global buffer.

I would do something like:

	pub fn init_buffer_logger(
		max_level: impl Into<LevelFilter>,
	) -> (DefaultGuard, Arc<Mutex<Vec<u8>>>) {
		let buf = Arc::new(Mutex::new(Vec::new()));
		let subscriber = tracing_subscriber::fmt()
			.with_max_level(max_level)
			.with_writer(MockMakeWriter::new(buf.clone()))
			.finish();
		(subscriber::set_default(subscriber), buf)
	}

(not sure about the naming)

The current MockWriter wouldn't work with this approach. We would have to use the MockWriter from tracing-subscriber or something similar. But it's really small and this way we could remove the tracing-test dependency as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this approach won’t work unless we implement our own Writer, similar to what we did here:
log_capture_test.rs#L13.

I'm okay with removing tracing-test completely if we go with this approach.

let subscriber = tracing_subscriber::fmt()
.with_max_level(max_level)
.with_writer(log_capture)
.finish();
subscriber::with_default(subscriber, f);
}

/// Checks whether a captured log message contains a given substring.
///
/// This function retrieves all captured logs and checks if the provided
/// string is present.
///
/// # Example
///
/// ```
/// use sp_tracing::test_log_capture;
///
/// test_log_capture::capture(|| {
/// tracing::debug!("Debug log for testing");
/// });
///
/// assert!(test_log_capture::logs_contain("Debug log"));
/// ```
pub fn logs_contain(value: &str) -> bool {
let logs = String::from_utf8(global_buf().lock().unwrap().clone()).unwrap();
logs.contains(value)
}
}
Loading