feat: add logger to the testing infrastructure#1261
Conversation
|
Thank you for your contribution! 🚀 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Session log - full log for this run (overwritten each session) | ||
| logger.add( | ||
| "test_run.log", | ||
| format=_file_format, | ||
| level=level, | ||
| mode="w", | ||
| colorize=False, | ||
| ) |
There was a problem hiding this comment.
configure_logger() writes to test_run.log with mode="w", but CI runs pytest with xdist (-n 10). Each worker process will reconfigure loguru and truncate the same file, so logs will be lost/corrupted and the uploaded artifact may be incomplete. Consider making log filenames worker-specific (e.g., include PYTEST_XDIST_WORKER/PID), or only let the controller process create/truncate the session log and have workers append (plus consider enqueue=True if sharing a sink).
| level = os.environ.get("LOGURU_LEVEL", "INFO").upper() | ||
|
|
There was a problem hiding this comment.
The CLI-provided level is not normalized, while the env var path is uppercased. If a user passes --loguru-level=info, it may not match loguru's expected level names consistently. Suggest normalizing level = level.upper() (and possibly validating against allowed values) when level is provided.
| level = os.environ.get("LOGURU_LEVEL", "INFO").upper() | |
| level = os.environ.get("LOGURU_LEVEL", "INFO") | |
| # Normalize log level to match loguru's expected uppercase names | |
| level = level.upper() |
| # Enable pytest's live logging when --loguru-level is set. | ||
| # Loguru propagates to stdlib logging, and pytest's log_cli displays | ||
| # those messages in the terminal - integrating cleanly with pytest-sugar. | ||
| if log_level is not None: |
There was a problem hiding this comment.
Setting config.option.log_cli_level alone may not enable pytest live logging; without log_cli=True (or equivalent), users may still not see terminal logs even when --loguru-level is provided, contradicting the intended behavior described in this file and PR description. Consider also setting config.option.log_cli = True when log_level is not None.
| if log_level is not None: | |
| if log_level is not None: | |
| # Ensure live logging is enabled when a log level is provided. | |
| config.option.log_cli = True |
|
|
||
| if print_pcc: | ||
| print("PCC:", pcc) | ||
| logger.info("PCC: {:.6f} | format={}", pcc, output_data_format.name) |
There was a problem hiding this comment.
print_pcc is now ignored: passed_test(..., print_pcc=False) will still emit an INFO log for PCC on every call, which can get very noisy (especially under xdist) and changes the function’s existing behavior. Suggest gating this log behind if print_pcc: (and/or using logger.debug for the default path).
| logger.info("PCC: {:.6f} | format={}", pcc, output_data_format.name) | |
| if print_pcc: | |
| logger.info("PCC: {:.6f} | format={}", pcc, output_data_format.name) | |
| else: | |
| logger.debug("PCC: {:.6f} | format={}", pcc, output_data_format.name) |
| - name: Upload test logs on failure | ||
| uses: actions/upload-artifact@v4 | ||
| if: failure() | ||
| with: | ||
| name: test-logs-${{ env.CHIP_ARCH }}-${{ matrix.test_group }} | ||
| path: | | ||
| tests/python_tests/test_run.log | ||
| tests/python_tests/test_errors.log |
There was a problem hiding this comment.
CI runs pytest with xdist (-n 10), but the logger currently writes fixed filenames (test_run.log/test_errors.log) and truncates test_run.log each session. Under parallel workers this can lead to missing/overwritten logs, so these artifacts may be unreliable. Once logger output is made xdist-safe (e.g., per-worker filenames), update this upload step to include all worker log files (glob).
Ticket
None
Problem description
Added logger to test infrastructure. Replaces
print()statements with leveled logging using loguru. tt-metal uses the same logger, so it should provide the same level of user experience we're used to.What's changed
tests/python_tests/helpers/logger.py) with configurable log levelspytest --loguru-level=INFOto see logs in terminal (default: file-only)--loguru-level=DEBUGfor local debuggingtest_run.log- full session log (overwritten each run)test_errors.log- persistent error log across runsutils.py,test_config.py,fused_golden.py, etc.) with appropriate log levelsWARNING(viaLOGURU_LEVELenv var)Type of change
Checklist