Skip to content

Add log capture assertions #1962

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented May 17, 2025

Summary

  • validate logging output in test_logging.py

Testing

  • python -m pytest -k test_logging -q (fails: No module named pytest)

@jslee02 jslee02 force-pushed the codex/add-assertions-for-log-output-in-tests branch from 31fc67e to ac16abc Compare May 18, 2025 00:30
@jslee02 jslee02 requested a review from Copilot May 18, 2025 00:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds assertions to validate logging output in tests by capturing log messages with capsys.

  • Updated test_basics to use capsys for capturing logs and asserting expected log messages.
  • Updated test_arguments to capture logs and verify the formatted logging output.

@jslee02 jslee02 force-pushed the codex/add-assertions-for-log-output-in-tests branch from ac16abc to 595aeeb Compare May 18, 2025 00:58
@jslee02 jslee02 requested a review from Copilot May 18, 2025 01:00
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

The purpose of this PR is to validate logging output by adding log capture assertions in the unit tests.

  • Introduces a helper function, assert_logged, to check log outputs.
  • Updates test_basics and test_arguments to use capsys for capturing and asserting log output.

@jslee02 jslee02 force-pushed the codex/add-assertions-for-log-output-in-tests branch from 595aeeb to 47ea050 Compare May 18, 2025 01:06
@jslee02 jslee02 requested a review from Copilot May 18, 2025 01:07
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces logging output validation in the unit tests by adding log capture assertions.

  • A helper function (assert_logged) is added to encapsulate the log assertion logic.
  • The test functions (test_basics and test_arguments) have been updated to use capsys and the new assertion helper.

@jslee02 jslee02 force-pushed the codex/add-assertions-for-log-output-in-tests branch from 47ea050 to f23f15e Compare May 18, 2025 01:23
@jslee02 jslee02 requested a review from Copilot May 18, 2025 01:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds log capture assertions to validate logging output by introducing a helper function.

  • Added a new helper function assert_logged that verifies log messages appear in exactly one of stdout or stderr.
  • Updated test_basics and test_arguments to use assert_logged for log validation.

@jslee02 jslee02 marked this pull request as ready for review May 18, 2025 01:48
@jslee02 jslee02 force-pushed the codex/add-assertions-for-log-output-in-tests branch from f23f15e to c45f797 Compare May 18, 2025 03:17
@jslee02 jslee02 requested a review from Copilot May 18, 2025 03:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds a reusable helper for capturing and asserting log messages in existing tests.

  • Introduce assert_logged to centralize log output validation.
  • Update test_basics and test_arguments to use the new helper and include capsys.
Comments suppressed due to low confidence (1)

python/tests/unit/common/test_logging.py:34

  • [nitpick] For improved readability and consistency, switch to an f-string: dart.common.info(f"Log with param '{1}' and '{val}'").
dart.common.info("Log with param '{}' and '{}'".format(1, val))

Comment on lines +5 to +13
def assert_logged(capsys, *messages: str) -> None:
"""Read captured logs and ensure *messages* appear in the output."""
captured = capsys.readouterr()
output = captured.out + captured.err
for msg in messages:
assert msg in output, f"{msg!r} not found in captured output"


def test_basics(capsys):
Copy link
Preview

Copilot AI May 18, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using pytest's caplog fixture instead of capsys for logging assertions, which provides richer log record inspection and severity filtering.

Suggested change
def assert_logged(capsys, *messages: str) -> None:
"""Read captured logs and ensure *messages* appear in the output."""
captured = capsys.readouterr()
output = captured.out + captured.err
for msg in messages:
assert msg in output, f"{msg!r} not found in captured output"
def test_basics(capsys):
def assert_logged(caplog, *messages: str) -> None:
"""Ensure *messages* appear in the captured logs."""
log_output = " ".join(record.message for record in caplog.records)
for msg in messages:
assert msg in log_output, f"{msg!r} not found in captured logs"
def test_basics(caplog):

Copilot uses AI. Check for mistakes.

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant