Skip to content

Improve debug logging and add tests that it is not evaluated when disabled #26

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

Merged

Conversation

simonjbeaumont
Copy link
Collaborator

Motivation

The transport code has some debug logging, which is compile-time omitted in release builds, and only enabled in tests.

A number of tests produce an enormous amount of output (e.g. the streaming tests that pull 10GB through 10MB buffers), which are unwieldy in CI and locally.

Additionally, we found that the logging produced by these tests had interleaved output, both because of the mixed use of print and debug in tests and the lack of locking between multiple threads

Finally, even when running in debug mode with debug logging disabled, the function arguments to the debug function were still evaluated unnecessarily.

Modifications

  • Update the debug function to log to standard error (cf. standard out).
  • Pass an autoclosure as argument to debug so that it is only evaluated when logging is enabled.
  • Add a lock around the use of FileHandle.standardError.
  • Replace all stray uses of print in tests with debug.

Result

  • No debug logging should appear in tests by default (you can enable them locally when reproducing failures).
  • When enabled, log lines should not be all jumbled up.
  • When disabled, no string interpolation is performed.

Test Plan

  • Added tests that check that the log message is not interpolated when disabled.

@simonjbeaumont simonjbeaumont force-pushed the debug-logging-improvements branch from 1a90ef0 to 2d98cc5 Compare November 23, 2023 11:42
@simonjbeaumont simonjbeaumont marked this pull request as ready for review November 23, 2023 11:44
@simonjbeaumont simonjbeaumont added the 🔨 semver/patch No public API change. label Nov 23, 2023
@simonjbeaumont simonjbeaumont merged commit aee2c63 into apple:main Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants