diff --git a/src/reportportal/ap.py b/src/reportportal/ap.py index 3d65980..93b208d 100644 --- a/src/reportportal/ap.py +++ b/src/reportportal/ap.py @@ -45,6 +45,10 @@ def create_main_parser() -> argparse.ArgumentParser: except PackageNotFoundError: pkg_version = 'unknown (not installed)' + # Get configuration defaults (config file + env vars + built-in defaults) + # Must be loaded before creating arguments that use these defaults + defaults = _get_config_defaults() + parser = argparse.ArgumentParser( prog='rptool', description='Unified command-line interface for ReportPortal tools', @@ -58,11 +62,17 @@ def create_main_parser() -> argparse.ArgumentParser: version=f'rptool {pkg_version}' ) + # Validation of config file log_level + valid_log_levels = {"DEBUG", "INFO", "WARNING", "ERROR"} + configured_log_level = str(defaults.get("log_level", "INFO")).upper() + if configured_log_level not in valid_log_levels: + raise ValueError(f'Invalid log level in config: {configured_log_level}. Must be one of {valid_log_levels}') + parser.add_argument( "--log-level", choices=["DEBUG", "INFO", "WARNING", "ERROR"], - default="INFO", - help="Set the logging level (default: INFO)" + default=configured_log_level, + help="Set the logging level (default: from config or INFO)" ) # Create subparsers for each command @@ -74,9 +84,6 @@ def create_main_parser() -> argparse.ArgumentParser: required=True ) - # Get configuration defaults (config file + env vars + built-in defaults) - defaults = _get_config_defaults() - # Adding subparsers' arguments subparsers_hanlers = [ _add_write_arguments, @@ -124,14 +131,14 @@ def _add_write_arguments(subparsers: argparse.ArgumentParser, defaults: dict) -> _add_common_rp_args(parser, defaults) parser.add_argument( - "--launch-name", + "--launch-name", help="Override Launch name that will be reported, otherwise filename will be used", default=defaults['rp_launch_name'] ) parser.add_argument( - "--launch-description", + "--launch-description", help="Custom head section to launch description, passthrough description will be added from the junit if available", - # The empty string from defaults is necessary to enable additional description to be added on .finish_launch() + # The empty string from defaults is necessary to enable additional description to be added on .finish_launch() default=defaults['rp_launch_description'], ) parser.add_argument( @@ -147,7 +154,7 @@ def _add_write_arguments(subparsers: argparse.ArgumentParser, defaults: dict) -> default=False ) parser.add_argument("junits", nargs='+', help="path to all junit results, multiple files will be reportes as one launch") - + def _add_query_arguments(subparsers: argparse.ArgumentParser, defaults: dict) -> None: """Add arguments for query command.""" @@ -235,13 +242,6 @@ def _add_trigger_arguments(subparsers: argparse.ArgumentParser, defaults: dict) ) _add_common_rp_args(parser, defaults) - parser.add_argument( - "--log-level", - choices=["DEBUG", "INFO", "WARNING", "ERROR"], - default=defaults.get("log_level", "INFO"), - help="Set the logging level (default: INFO)" - ) - def _add_summary_arguments(subparsers: argparse.ArgumentParser, defaults: dict) -> None: """Add arguments for summary command.""" @@ -255,12 +255,6 @@ def _add_summary_arguments(subparsers: argparse.ArgumentParser, defaults: dict) _add_common_rp_args(parser, defaults) - parser.add_argument( - "--log-level", - choices=["DEBUG", "INFO", "WARNING", "ERROR"], - default=defaults.get("log_level", "INFO"), - help="Set the logging level (default: INFO)" - ) parser.add_argument( "--attribute", action="append", diff --git a/src/reportportal/config.py b/src/reportportal/config.py index c6a35fa..e116094 100644 --- a/src/reportportal/config.py +++ b/src/reportportal/config.py @@ -37,26 +37,29 @@ def load_config_file() -> Dict[str, Any]: Returns: Dictionary with configuration values, empty dict if file doesn't exist - or can't be loaded + or is empty. + + Raises: + ValueError: When config file cannot be parsed properly. """ config_file = get_config_file_path() if not config_file.exists(): - logger.debug(f"Config file not found: {config_file}") + logger.debug("No config file present, using defaults") return {} try: with open(config_file, 'r') as f: config = yaml.safe_load(f) if config is None: - logger.debug(f"Config file is empty: {config_file}") + logger.debug("Config file empty") return {} - logger.debug(f"Loaded config from: {config_file}") + logger.info("Config file loaded successfully") return config except Exception as e: - logger.warning(f"Failed to load config file {config_file}: {e}") - return {} + # need to raise ValueError to indicate critical problem + raise ValueError(f"Error reading config file {config_file} {e}") def get_config_defaults() -> Dict[str, Any]: @@ -162,6 +165,7 @@ def get_effective_defaults() -> Dict[str, Any]: # Inject REQUESTS_CA_BUNDLE into environment if configured but not already set if merged.get("requests_ca_bundle") and not os.environ.get("REQUESTS_CA_BUNDLE"): os.environ["REQUESTS_CA_BUNDLE"] = merged["requests_ca_bundle"] - logger.debug(f"Set REQUESTS_CA_BUNDLE from config: {merged['requests_ca_bundle']}") + logger.debug("Set REQUESTS_CA_BUNDLE from config: {}", merged['requests_ca_bundle']) return merged + diff --git a/src/reportportal/rp_dispatcher.py b/src/reportportal/rp_dispatcher.py index 3ab9ebd..503d8f0 100644 --- a/src/reportportal/rp_dispatcher.py +++ b/src/reportportal/rp_dispatcher.py @@ -164,7 +164,19 @@ def main(argv: Optional[List[str]] = None) -> int: Returns: Exit code (0 for success, 1 for error) """ - parser = ap.create_main_parser() + # Remove default loguru handler immediately to prevent premature debug messages + # (e.g., during config file loading before log level is determined) + logger.remove() + # Setup intermittent WARNING logger for any configuration logs + # or set to environmnet variable LOG_LEVEL if defined + logger.add(sink=sys.stderr, level=os.environ.get('LOG_LEVEL', "").upper() or 'WARNING') + + try: + parser = ap.create_main_parser() + except ValueError as e: + logger.error('Improper configuration {}', str(e)) + return 1 + # Parse arguments try: @@ -172,8 +184,8 @@ def main(argv: Optional[List[str]] = None) -> int: except SystemExit as e: return e.code if e.code is not None else 1 - # setup logging handlers - logger.remove() # remove default one + # Setup logging handler with configured log level + logger.remove() logger.add(sink=sys.stderr, level=args.log_level) # Dispatch to appropriate command handler diff --git a/tests/unit/test_ap.py b/tests/unit/test_ap.py index fa34aa9..38d7ab2 100644 --- a/tests/unit/test_ap.py +++ b/tests/unit/test_ap.py @@ -4,6 +4,7 @@ import pytest import os +from pathlib import Path from unittest.mock import patch from reportportal.ap import ( @@ -321,11 +322,11 @@ def test_release_parser_all_options(self): parser = create_main_parser() args = parser.parse_args([ + '--log-level', 'DEBUG', 'summary', '--rp-project', 'test_project', '--rp-url', 'https://test.reportportal.com', '--rp-token', 'test_key', - '--log-level', 'DEBUG', '--attribute', 'kuadrant:v1.3.1', '--attribute', 'platform:aws', '--days', '7', @@ -373,7 +374,7 @@ def test_release_parser_log_levels(self): parser = create_main_parser() for level in ['DEBUG', 'INFO', 'WARNING', 'ERROR']: - args = parser.parse_args(['summary', '--attribute', 'kuadrant:v1.3.1', '--log-level', level]) + args = parser.parse_args(['--log-level', level, 'summary', '--attribute', 'kuadrant:v1.3.1']) assert args.log_level == level def test_release_parser_days_parameter(self): @@ -535,4 +536,136 @@ def test_multi_criteria_filtering(self): assert 'platform:gcp' in args.attribute assert 'component:controller' in args.attribute assert 'env:staging' in args.attribute - assert args.days == 7 \ No newline at end of file + assert args.days == 7 + + +@pytest.mark.unit +class TestLogLevelConfig: + """Test log level configuration (Issue #6).""" + + def test_log_level_from_config_file(self): + """Test that log_level from config file is used as default.""" + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + # Simulate config file with DEBUG log level + mock_load.return_value = {'log_level': 'DEBUG'} + parser = create_main_parser() + + # Parse without --log-level argument + args = parser.parse_args(['write', 'test.xml']) + + # Should use config file value + assert args.log_level == 'DEBUG' + + def test_log_level_cli_overrides_config(self): + """Test that CLI --log-level overrides config file.""" + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + # Config has DEBUG + mock_load.return_value = {'log_level': 'DEBUG'} + parser = create_main_parser() + + # CLI specifies ERROR + args = parser.parse_args(['--log-level', 'ERROR', 'write', 'test.xml']) + + # Should use CLI value + assert args.log_level == 'ERROR' + + def test_log_level_default_when_no_config(self): + """Test that INFO is used when no config is provided.""" + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + # No log_level in config + mock_load.return_value = {} + parser = create_main_parser() + + # Parse without --log-level argument + args = parser.parse_args(['write', 'test.xml']) + + # Should use built-in default (INFO) + assert args.log_level == 'INFO' + + def test_log_level_works_with_all_commands(self): + """Test that config log_level works for all commands.""" + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + mock_load.return_value = {'log_level': 'WARNING'} + parser = create_main_parser() + + # Test write command + args = parser.parse_args(['write', 'test.xml']) + assert args.log_level == 'WARNING' + + # Test query command + args = parser.parse_args(['query']) + assert args.log_level == 'WARNING' + + # Test trigger command + args = parser.parse_args(['trigger']) + assert args.log_level == 'WARNING' + + # Test summary command + args = parser.parse_args(['summary', '--attribute', 'test:v1']) + assert args.log_level == 'WARNING' + + def test_log_level_invalid_in_config(self): + """Test that invalid log level in config raises ValueError.""" + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + # Simulate config file with invalid log level + mock_load.return_value = {'log_level': 'VERBOSE'} + + # Creating parser should raise ValueError + with pytest.raises(ValueError) as exc_info: + create_main_parser() + + # Check error message contains the invalid value + error_msg = str(exc_info.value) + assert 'Invalid log level in config' in error_msg + assert 'VERBOSE' in error_msg + assert 'Must be one of' in error_msg + + def test_log_level_invalid_in_config_case_insensitive(self): + """Test that invalid log level works with case normalization.""" + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + # Lowercase 'trace' should also be rejected + mock_load.return_value = {'log_level': 'trace'} + + with pytest.raises(ValueError) as exc_info: + create_main_parser() + + error_msg = str(exc_info.value) + assert 'Invalid log level in config' in error_msg + # Should show uppercase version in error + assert 'TRACE' in error_msg + + def test_no_premature_debug_messages_during_config_load(self): + """Test that debug messages during config loading are suppressed until log level is set.""" + import io + from unittest.mock import patch + from reportportal.rp_dispatcher import main + + # Capture stderr to check for debug messages + captured_stderr = io.StringIO() + + with patch.dict(os.environ, {}, clear=True): + with patch('reportportal.config.load_config_file') as mock_load: + # Simulate config file exists and has values + mock_load.return_value = {'log_level': 'INFO', 'rp_url': 'http://test.com'} + + # Redirect stderr + with patch('sys.stderr', captured_stderr): + try: + # Run with INFO level (default from config) + # This will fail because we don't have valid args, but we just want to check logging + main(['write', 'test.xml']) + except SystemExit: + pass + + # Check that no "Loaded config from" or "Config file" debug messages appear + stderr_output = captured_stderr.getvalue() + assert "Loaded config from" not in stderr_output, \ + "Debug message 'Loaded config from' should not appear with INFO log level" + assert "Config file not found" not in stderr_output, \ + "Debug message 'Config file not found' should not appear with INFO log level" diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 33be313..ac1a1b2 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -28,24 +28,25 @@ def test_get_config_file_path_with_platformdirs(self): assert path.name == "settings.yaml" assert "rptool" in str(path) + class TestLoadConfigFile: """Test YAML config file loading.""" def test_load_config_file_not_exists(self): """Test loading when config file doesn't exist.""" - with patch('reportportal.config.get_config_file_path') as mock_path: + with patch("reportportal.config.get_config_file_path") as mock_path: mock_path.return_value = Path("/nonexistent/settings.yaml") config = load_config_file() assert config == {} def test_load_config_file_empty(self): """Test loading empty config file.""" - with tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) as f: + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: f.write("") temp_path = f.name try: - with patch('reportportal.config.get_config_file_path') as mock_path: + with patch("reportportal.config.get_config_file_path") as mock_path: mock_path.return_value = Path(temp_path) config = load_config_file() assert config == {} @@ -54,8 +55,9 @@ def test_load_config_file_empty(self): def test_load_config_file_valid(self): """Test loading valid config file.""" - with tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) as f: - f.write(""" + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: + f.write( + """ rp_url: "https://test.com" rp_project: "test_project" rp_token: "test_token" @@ -63,11 +65,12 @@ def test_load_config_file_valid(self): launch_name: "Test Launch" launch_description: "Test Description" log_level: "DEBUG" -""") +""" + ) temp_path = f.name try: - with patch('reportportal.config.get_config_file_path') as mock_path: + with patch("reportportal.config.get_config_file_path") as mock_path: mock_path.return_value = Path(temp_path) config = load_config_file() assert config["rp_url"] == "https://test.com" @@ -81,17 +84,21 @@ def test_load_config_file_valid(self): os.unlink(temp_path) def test_load_config_file_invalid_yaml(self): - """Test loading invalid YAML file.""" - with tempfile.NamedTemporaryFile(mode='w', suffix='.yaml', delete=False) as f: + """Test loading invalid YAML file raises ValueError.""" + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as f: f.write("invalid: yaml: content:") temp_path = f.name try: - with patch('reportportal.config.get_config_file_path') as mock_path: + with patch("reportportal.config.get_config_file_path") as mock_path: mock_path.return_value = Path(temp_path) - config = load_config_file() - # Should return empty dict on error - assert config == {} + # Should raise ValueError on error + with pytest.raises(ValueError) as exc_info: + load_config_file() + + # Check error message contains file path + error_msg = str(exc_info.value) + assert "Error reading config file" in error_msg finally: os.unlink(temp_path) @@ -101,7 +108,7 @@ class TestGetConfigDefaults: def test_get_config_defaults_builtin(self): """Test that built-in defaults are returned when no config file exists.""" - with patch('reportportal.config.load_config_file') as mock_load: + with patch("reportportal.config.load_config_file") as mock_load: mock_load.return_value = {} defaults = get_config_defaults() @@ -115,14 +122,14 @@ def test_get_config_defaults_builtin(self): def test_get_config_defaults_from_file(self): """Test that config file values override built-in defaults.""" - with patch('reportportal.config.load_config_file') as mock_load: + with patch("reportportal.config.load_config_file") as mock_load: mock_load.return_value = { "rp_url": "https://config-file.com", "rp_project": "file_project", "trigger_auto_analysis": True, "launch_name": "Test Launch", "launch_description": "Test Description", - "log_level": "DEBUG" + "log_level": "DEBUG", } defaults = get_config_defaults() @@ -147,7 +154,7 @@ def test_merge_with_env_vars_empty_env(self): "trigger_auto_analysis": False, "launch_name": None, "launch_description": "", - "log_level": "INFO" + "log_level": "INFO", } with patch.dict(os.environ, {}, clear=True): @@ -163,7 +170,7 @@ def test_merge_with_env_vars_override(self): "trigger_auto_analysis": False, "launch_name": "Config Launch", "launch_description": "Config Description", - "log_level": "INFO" + "log_level": "INFO", } env_vars = { @@ -172,7 +179,7 @@ def test_merge_with_env_vars_override(self): "TRIGGER_AUTO_ANALYSIS": "true", "RP_LAUNCH_NAME": "Env Launch", "RP_LAUNCH_DESCRIPTION": "Env Description", - "LOG_LEVEL": "DEBUG" + "LOG_LEVEL": "DEBUG", } with patch.dict(os.environ, env_vars, clear=True): @@ -198,7 +205,7 @@ def test_trigger_auto_analysis_true_values(self): "trigger_auto_analysis": False, "launch_name": None, "launch_description": "", - "log_level": "INFO" + "log_level": "INFO", } true_values = ["true", "True", "TRUE", "1", "yes", "YES", "on", "ON"] @@ -206,7 +213,9 @@ def test_trigger_auto_analysis_true_values(self): for val in true_values: with patch.dict(os.environ, {"TRIGGER_AUTO_ANALYSIS": val}, clear=True): merged = merge_with_env_vars(config) - assert merged["trigger_auto_analysis"] is True, f"Failed for value: {val}" + assert ( + merged["trigger_auto_analysis"] is True + ), f"Failed for value: {val}" def test_trigger_auto_analysis_false_values(self): """Test that various false string values are converted to boolean False.""" @@ -217,7 +226,7 @@ def test_trigger_auto_analysis_false_values(self): "trigger_auto_analysis": True, "launch_name": None, "launch_description": "", - "log_level": "INFO" + "log_level": "INFO", } false_values = ["false", "False", "FALSE", "0", "no", "NO", "off", "OFF"] @@ -225,7 +234,9 @@ def test_trigger_auto_analysis_false_values(self): for val in false_values: with patch.dict(os.environ, {"TRIGGER_AUTO_ANALYSIS": val}, clear=True): merged = merge_with_env_vars(config) - assert merged["trigger_auto_analysis"] is False, f"Failed for value: {val}" + assert ( + merged["trigger_auto_analysis"] is False + ), f"Failed for value: {val}" def test_trigger_auto_analysis_no_env(self): """Test that config value is preserved when env var is not set.""" @@ -236,7 +247,7 @@ def test_trigger_auto_analysis_no_env(self): "trigger_auto_analysis": True, "launch_name": None, "launch_description": "", - "log_level": "INFO" + "log_level": "INFO", } with patch.dict(os.environ, {}, clear=True): @@ -250,13 +261,13 @@ class TestGetEffectiveDefaults: def test_get_effective_defaults_priority(self): """Test that full priority chain works correctly.""" # Mock config file - with patch('reportportal.config.load_config_file') as mock_load: + with patch("reportportal.config.load_config_file") as mock_load: mock_load.return_value = { "rp_url": "https://config.com", "rp_project": "config_project", "trigger_auto_analysis": False, "launch_name": "Config Launch", - "log_level": "INFO" + "log_level": "INFO", } # Set env vars @@ -264,7 +275,7 @@ def test_get_effective_defaults_priority(self): "RP_URL": "https://env.com", "TRIGGER_AUTO_ANALYSIS": "1", "RP_LAUNCH_DESCRIPTION": "Env Description", - "LOG_LEVEL": "DEBUG" + "LOG_LEVEL": "DEBUG", } with patch.dict(os.environ, env_vars, clear=True): @@ -282,3 +293,4 @@ def test_get_effective_defaults_priority(self): # Built-in default when neither config nor ENV assert defaults["rp_token"] is None + diff --git a/tests/unit/test_rp_dispatcher_write.py b/tests/unit/test_rp_dispatcher_write.py index 7399aa6..cb75c45 100644 --- a/tests/unit/test_rp_dispatcher_write.py +++ b/tests/unit/test_rp_dispatcher_write.py @@ -197,8 +197,12 @@ def test_run_write_command_with_log_level(self, mock_rpwriter): class TestWriteCommandIntegration: """Test write command integration with main dispatcher.""" - def test_write_command_registered(self): + @patch('reportportal.config.load_config_file') + def test_write_command_registered(self, mock_load_config): """Test that write command is registered in dispatcher.""" + # Mock config file to isolate test from user's environment + mock_load_config.return_value = {} + parser = create_main_parser() # Parse write command help to verify it exists @@ -208,8 +212,12 @@ def test_write_command_registered(self): # --help should exit with code 0 assert exc_info.value.code == 0 - def test_write_command_arguments(self): + @patch('reportportal.config.load_config_file') + def test_write_command_arguments(self, mock_load_config): """Test parsing write command arguments.""" + # Mock config file to isolate test from user's environment + mock_load_config.return_value = {} + parser = create_main_parser() args = parser.parse_args([ '--log-level', 'DEBUG', @@ -231,8 +239,12 @@ def test_write_command_arguments(self): assert args.trigger_auto_analysis is True assert args.junits == ['test.xml'] - def test_write_command_missing_junit_file(self): + @patch('reportportal.config.load_config_file') + def test_write_command_missing_junit_file(self, mock_load_config): """Test that missing JUnit file raises error.""" + # Mock config file to isolate test from user's environment + mock_load_config.return_value = {} + parser = create_main_parser() with pytest.raises(SystemExit) as exc_info: