Skip to content

Add tests for logging utilities#377

Open
zhongkaifu wants to merge 1 commit intomainfrom
codex/add-tests-for-logging-utility-functions
Open

Add tests for logging utilities#377
zhongkaifu wants to merge 1 commit intomainfrom
codex/add-tests-for-logging-utility-functions

Conversation

@zhongkaifu
Copy link
Copy Markdown
Owner

Motivation

  • Improve coverage for logging utilities that manage run and trace context and produce structured JSON logs.
  • Ensure log_event includes workflow_run_id, trace_id, span_id, and payload overrides for node_id/action_id.
  • Validate log_llm_message correctly serializes object-style messages including tool_calls[].function.name and arguments.

Description

  • Add tests/test_logging_utils.py containing test_configure_run_logging_sets_and_resets_run_id, test_use_trace_context_sets_current_and_resets, test_log_event_includes_context_and_payload_override, and test_log_llm_message_serializes_tool_calls.
  • Tests use capsys to capture stdout JSONL output and tmp_path to write temporary log files.
  • Tests exercise configure_run_logging, use_trace_context, log_event, and log_llm_message, and verify payload merging and trace context propagation.

Testing

  • No automated tests were executed as part of this change.

Codex Task

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +46 to +47
assert record["node_id"] == "node-1"
assert record["action_id"] == "action-1"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Align node_id/action_id assertions with log_event behavior

These assertions assume log_event will copy node_id/action_id from the payload when the explicit args are omitted, but log_event currently initializes those keys to None and then uses record.setdefault(...), which does not overwrite existing keys. As a result, the emitted JSON line will keep node_id/action_id as null and this test will fail unless the production code is updated to override or the test passes node_id/action_id explicitly. Consider adjusting the test expectations or the logging logic to match the intended behavior.

Useful? React with 👍 / 👎.

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