Skip to content

Add nx_log! usage examples and unit tests for the log macro - #93

Open
lntutor wants to merge 1 commit into
GianIac:mainfrom
lntutor:test/nx-log-formatting-67
Open

Add nx_log! usage examples and unit tests for the log macro#93
lntutor wants to merge 1 commit into
GianIac:mainfrom
lntutor:test/nx-log-formatting-67

Conversation

@lntutor

@lntutor lntutor commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • document literal, formatted, and multi-argument nx_log! usage
  • capture log messages in unit tests without invoking the wasm host ABI
  • assert the exact output produced by each formatting form

Verification

  • /Users/loi/.cargo/bin/cargo test -p nx-sdk
  • /Users/loi/.cargo/bin/cargo doc -p nx-sdk --no-deps
  • /Users/loi/.cargo/bin/cargo fmt --all -- --check
  • /Users/loi/.cargo/bin/cargo clippy -p nx-sdk --all-targets -- -D warnings
  • git diff --check

Closes #67

@lntutor

lntutor commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Current head de6cac0d0403099a16b0f63deb80b4769f95ad6c is mergeable and the visible CI surface on this exact SHA is fully green: cross-platform Check, Clippy, and Test jobs on Ubuntu, macOS, and Windows; Format; Build WASM Examples; CLI Multi-process Smoke; Benchmark Comparator Tests; Ubuntu Profiling Artifacts; Benchmark Regression Gate; and the aggregate CI Success check all completed successfully. There are no review threads or review-body comments open on the current PR state, so if this nx_log formatting test coverage still matches the intended direction, it looks ready for maintainer review/merge when convenient.

@GianIac GianIac left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for this PR.
Good coverage of the three formatting patterns and the doctest gating with #[cfg(target_arch = "wasm32")] is a nice touch.

  • PR has conflicts with main (likely from #90 adding #[inline] to log::log). Please rebase and keep the #[inline] on the #[cfg(not(test))] version.

Question

  • The issue asked for "docs + tests, no public API changes", but this replaces log::log with a fully separate #[cfg(test)] in-memory implementation. It works, but it's more invasive than needed and cascades into the #[expect(dead_code)] on host_log_v2.
  • Would you consider a lighter approach? e.g. testing only the macro expansion via a small #[cfg(test)] fn fmt_message(args: fmt::Arguments) -> String and asserting on that, without swapping out log::log itself. That keeps log.rs single-implementation and closer to the issue scope.
  • If you prefer to keep the current approach, that's OK too just add a short comment at the top of log.rs explaining why there are two log::log definitions.

Nits (optional)

  • Consider pub(crate) on take_test_messages so future modules can assert on emitted logs.
  • The assert_logged_message helper is nice, worth keeping either way.

Once rebased and we've agreed on the design direction, happy to merge.

@GianIac GianIac changed the title test(sdk): cover nx_log formatting Add nx_log! usage examples and unit tests for the log macro Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add nx_log! usage examples and unit tests for the log macro

2 participants