-
Notifications
You must be signed in to change notification settings - Fork 170
add functionality to enable color logging for ros2launch messages #945
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
andreas-loeffler
wants to merge
1
commit into
ros2:rolling
Choose a base branch
from
andreas-loeffler:feature/enable_color_logging
base: rolling
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+154
−3
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,24 @@ | |
| import pytest | ||
|
|
||
|
|
||
| def contains_ansicode(str_to_check: str) -> bool: | ||
| """ | ||
| Check if given string contains an ANSI code sequence. | ||
|
|
||
| :param str_to_check: Given string to check for ANSI codes. | ||
| :return: True if the given string contains any ANSI code sequence, | ||
| False otherwise. | ||
| """ | ||
| ansi_regex = re.compile(r""" | ||
| \x1b # literal ESC | ||
| \[ # literal [ | ||
| [;\d]* # zero or more digits or semicolons | ||
| [A-Za-z] # a letter | ||
| """, re.VERBOSE) | ||
|
|
||
| return bool(ansi_regex.search(str_to_check)) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def log_dir(tmpdir_factory): | ||
| """Test fixture that generates a temporary directory for log files.""" | ||
|
|
@@ -97,6 +115,111 @@ def test_output_loggers_bad_configuration(log_dir): | |
| ] | ||
|
|
||
|
|
||
| def test_colorized_output_with_tty(capsys, monkeypatch, mock_clean_env): | ||
| """ | ||
| Test color output with TTY. | ||
|
|
||
| Colors should be enabled when stdout is set to a TTY and | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the exact behavior I'm proposing we consider inverting. |
||
| RCUTILS_COLORIZED_OUTPUT is unset. | ||
| """ | ||
| monkeypatch.setattr('sys.stdout.isatty', lambda: True) | ||
| monkeypatch.delenv('RCUTILS_COLORIZED_OUTPUT', raising=False) | ||
|
|
||
| launch.logging.reset() | ||
| logger = launch.logging.get_logger() | ||
| logger.setLevel(logging.INFO) | ||
| logger.info('Test message with TTY') | ||
|
|
||
| capture = capsys.readouterr() | ||
| assert contains_ansicode(capture.out), \ | ||
| 'Expected ANSI color codes in output with TTY' | ||
|
|
||
|
|
||
| def test_colorized_output_disabled_by_env(capsys, monkeypatch, mock_clean_env): | ||
| """ | ||
| Test color output disabled by env var. | ||
|
|
||
| Colors should be disabled when stdout is a TTY but | ||
| RCUTILS_COLORIZED_OUTPUT=0. | ||
| """ | ||
| monkeypatch.setattr('sys.stdout.isatty', lambda: True) | ||
| monkeypatch.setenv('RCUTILS_COLORIZED_OUTPUT', '0') | ||
|
|
||
| launch.logging.reset() | ||
| logger = launch.logging.get_logger() | ||
| logger.setLevel(logging.INFO) | ||
| logger.info('Test message with colors disabled') | ||
|
|
||
| capture = capsys.readouterr() | ||
| assert not contains_ansicode(capture.out), \ | ||
| 'Expected no ANSI color codes when disabled by env var' | ||
|
|
||
|
|
||
| def test_colorized_output_forced_by_env(capsys, monkeypatch, mock_clean_env): | ||
| """ | ||
| Test color output forced by env var in non TTY. | ||
|
|
||
| Colors should be forced on when RCUTILS_COLORIZED_OUTPUT=1 | ||
| even without TTY. | ||
| """ | ||
| monkeypatch.setattr('sys.stdout.isatty', lambda: False) | ||
| monkeypatch.setenv('RCUTILS_COLORIZED_OUTPUT', '1') | ||
|
|
||
| launch.logging.reset() | ||
| logger = launch.logging.get_logger() | ||
| logger.setLevel(logging.INFO) | ||
| logger.info('Test message with forced colors') | ||
|
|
||
| capture = capsys.readouterr() | ||
| assert contains_ansicode(capture.out), \ | ||
| 'Expected ANSI color codes when forced by env var' | ||
|
|
||
|
|
||
| def test_file_logs_not_colorized(log_dir, monkeypatch, mock_clean_env): | ||
| """ | ||
| Test that file logs are not colorized. | ||
|
|
||
| File logs should never contain ANSI color codes even if | ||
| RCUTILS_COLORIZED_OUTPUT=1. | ||
| """ | ||
| monkeypatch.setenv('RCUTILS_COLORIZED_OUTPUT', '1') | ||
|
|
||
| launch.logging.reset() | ||
| launch.logging.launch_config.log_dir = log_dir | ||
| logger = launch.logging.get_logger() | ||
| logger.setLevel(logging.INFO) | ||
| logger.info('Test message for file logging') | ||
|
|
||
| log_file = pathlib.Path(log_dir) / 'launch.log' | ||
| assert log_file.exists(), 'Log file should exist' | ||
| file_contents = log_file.read_text() | ||
|
|
||
| assert not contains_ansicode(file_contents), \ | ||
| 'File logs should never contain ANSI color codes' | ||
| assert 'Test message for file logging' in file_contents, \ | ||
| 'Message should be in log file' | ||
|
|
||
|
|
||
| def test_colorized_output_no_tty(capsys, monkeypatch, mock_clean_env): | ||
| """ | ||
| Test color output without a TTY and no env var. | ||
|
|
||
| Colors should be disabled when stdout is not a TTY and | ||
| RCUTILS_COLORIZED_OUTPUT is unset. | ||
| """ | ||
| monkeypatch.setattr('sys.stdout.isatty', lambda: False) | ||
| monkeypatch.delenv('RCUTILS_COLORIZED_OUTPUT', raising=False) | ||
|
|
||
| launch.logging.reset() | ||
| logger = launch.logging.get_logger() | ||
| logger.setLevel(logging.INFO) | ||
| logger.info('Test message without TTY') | ||
|
|
||
| capture = capsys.readouterr() | ||
| assert not contains_ansicode(capture.out), \ | ||
| 'Expected no ANSI color codes without TTY' | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('config,checks,main_log_file_name', params) | ||
| def test_output_loggers_configuration( | ||
| capsys, log_dir, config, checks, main_log_file_name, mock_clean_env | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the
RCUTILS_COLORIZED_OUTPUTenvironment variable is not set it looks like the terminal will have colors added by default. My only fear is that some ROS user might be relying on log parsing that might not be expecting the escape sequences used to add the color. Should we perhaps leave the default behavior as no colors, and have the user opt-in to the new behavior?What do you think @mjcarroll?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree with this.
besides, scheme won't match rcutils.
ColoredFormatteris constructed without an explicit log_colors=, so it uses colorlog's defaults, notably INFO=green, DEBUG=cyan, those are not consistent with rcutils convention.so launch's own messages will be colored differently from the node output they're interleaved with.
I'd ask for an explicit log_colors mapping that mirrors rcutils for visual consistency.