[FEATURE] Adding Realtime Log feature to CLI#81
[FEATURE] Adding Realtime Log feature to CLI#81antonio-amjr wants to merge 1 commit intoproject-chip:v2.15-cli-developfrom
Conversation
antonio-amjr
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a real-time log display feature for the CLI, utilizing the "rich" library to provide live feedback during specific test execution steps. The implementation includes a new RealtimeLogDisplay manager, integration with the websocket communication layer, dependency updates in pyproject.toml, and a suite of unit tests. Review feedback suggests several optimizations, including more robust timestamp handling, the removal of dead code, and performance improvements for the rendering logic.
|
|
||
| with self.lock: | ||
| # Format timestamp | ||
| if timestamp: |
There was a problem hiding this comment.
The check if timestamp: will evaluate to False if the timestamp is exactly 0.0. It is safer to use if timestamp is not None: to ensure that all valid timestamps are correctly processed.
| if timestamp: | |
| if timestamp is not None: |
| # if len(clean_message) > 180: | ||
| # clean_message = clean_message[:177] + "..." |
There was a problem hiding this comment.
These lines contain commented-out code. It is best practice to remove dead code before merging to maintain code cleanliness.
| if self.live_display and self.is_active: | ||
| try: | ||
| self.live_display.update(self._render_display()) | ||
| except Exception as e: | ||
| logger.debug(f"Error updating live display: {e}") |
There was a problem hiding this comment.
Calling live_display.update() on every log entry can be inefficient if logs are produced at a high frequency. Since Live is configured with refresh_per_second=4, you can improve performance by implementing the __rich__ method on this class (returning the result of _render_display()) and passing self to the Live constructor. This allows rich to handle the refresh rate automatically without flooding the terminal with updates for every single log message.
| "DEBUG": "dim cyan", | ||
| "INFO": "blue", | ||
| "WARNING": "yellow", | ||
| "ERROR": "red", | ||
| "CRITICAL": "bold red", | ||
| } | ||
| color = level_colors.get(level, "white") |
There was a problem hiding this comment.
The level_colors dictionary is recreated every time _get_level_style is called. For better performance, especially given this is called for every log record, consider moving this dictionary to a module-level or class-level constant.
| # Create title with step information | ||
| title = "Real-time Test Logs" | ||
| if self.current_test_case_id: | ||
| title = f"Real-time Test Logs - {self.current_test_case_id}" |
There was a problem hiding this comment.
The current_step_title is stored but not displayed in the panel. Including it would provide better context to the user about which specific step is currently executing.
# Create title with step information
parts = ["Real-time Test Logs"]
if self.current_test_case_id:
parts.append(self.current_test_case_id)
if self.current_step_title:
parts.append(self.current_step_title)
title = " - ".join(parts)1 participant
Fix: project-chip/certification-tool#944
WIP