From 7a88a2c2e96483c8c197b5100d75477f0adf1de9 Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Mon, 8 Sep 2025 13:57:36 -0500 Subject: [PATCH 1/8] add display_results option to deliver and --hide-results flag to cli --- servicex/app/main.py | 10 ++++-- servicex/servicex_client.py | 62 +++++++++++++++++++++++++++++++++++-- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/servicex/app/main.py b/servicex/app/main.py index b82448ef..c3b87be0 100644 --- a/servicex/app/main.py +++ b/servicex/app/main.py @@ -49,6 +49,11 @@ ignore_cache_opt = typer.Option( None, "--ignore-cache", help="Ignore local cache and always submit to ServiceX" ) +hide_results_opt = typer.Option( + False, + "--hide-results", + help="Exclude printing results to the console", +) def show_version(show: bool): @@ -75,19 +80,20 @@ def deliver( config_path: Optional[str] = config_file_option, spec_file: str = spec_file_arg, ignore_cache: Optional[bool] = ignore_cache_opt, + hide_results: bool = hide_results_opt, ): """ Deliver a file to the ServiceX cache. """ print(f"Delivering {spec_file} to ServiceX cache") - results = servicex_client.deliver( + servicex_client.deliver( spec_file, servicex_name=backend, config_path=config_path, ignore_local_cache=ignore_cache, + display_results=not hide_results, ) - rich.print(results) if __name__ == "__main__": diff --git a/servicex/servicex_client.py b/servicex/servicex_client.py index f9e66600..535050ab 100644 --- a/servicex/servicex_client.py +++ b/servicex/servicex_client.py @@ -52,6 +52,8 @@ from collections.abc import Sequence, Coroutine from enum import Enum import traceback +from rich.console import Console +from rich.table import Table T = TypeVar("T") logger = logging.getLogger(__name__) @@ -233,6 +235,44 @@ def _output_handler( return out_dict +def _display_results(out_dict): + """Display the delivery results using rich styling.""" + console = Console() + + console.print("\n[bold green]✓ ServiceX Delivery Complete![/bold green]\n") + + table = Table( + title="Delivered Files", show_header=True, header_style="bold magenta" + ) + table.add_column("Sample", style="cyan", no_wrap=True) + table.add_column("File Count", justify="right", style="green") + table.add_column("Files", style="dim") + + total_files = 0 + for sample_name, files in out_dict.items(): + if isinstance(files, GuardList) and files.valid(): + file_list = list(files) + file_count = len(file_list) + total_files += file_count + + # Show first few files with ellipsis if many + if file_count <= 3: + files_display = "\n".join(str(f) for f in file_list) + else: + files_display = "\n".join(str(f) for f in file_list[:2]) + files_display += f"\n... and {file_count - 2} more files" + + table.add_row(sample_name, str(file_count), files_display) + else: + # Handle error case + table.add_row( + sample_name, "[red]Error[/red]", "[red]Failed to retrieve files[/red]" + ) + + console.print(table) + console.print(f"\n[bold blue]Total files delivered: {total_files}[/bold blue]\n") + + async def deliver_async( spec: Union[ServiceXSpec, Mapping[str, Any], str, Path], config_path: Optional[str] = None, @@ -241,6 +281,7 @@ async def deliver_async( fail_if_incomplete: bool = True, ignore_local_cache: bool = False, progress_bar: ProgressBarFormat = ProgressBarFormat.default, + display_results: bool = True, concurrency: int = 10, ): r""" @@ -263,6 +304,8 @@ async def deliver_async( will have its own progress bars; :py:const:`ProgressBarFormat.compact` gives one summary progress bar for all transformations; :py:const:`ProgressBarFormat.none` switches off progress bars completely. + :param display_results: Specifies whether the results should be displayed to the console. + Defaults to True. :param concurrency: specify how many downloads to run in parallel (default is 8). :return: A dictionary mapping the name of each :py:class:`Sample` to a :py:class:`.GuardList` with the file names or URLs for the outputs. @@ -291,17 +334,30 @@ async def deliver_async( else: raise ValueError(f"Invalid value {progress_bar} for progress_bar provided") + if config.General.Delivery not in [ + General.DeliveryEnum.URLs, + General.DeliveryEnum.LocalCache, + ]: + raise ValueError( + f"unexpected value for config.general.Delivery: {config.General.Delivery}" + ) + if config.General.Delivery == General.DeliveryEnum.URLs: results = await group.as_signed_urls_async( return_exceptions=return_exceptions, **progress_options ) - return _output_handler(config, datasets, results) - elif config.General.Delivery == General.DeliveryEnum.LocalCache: + else: results = await group.as_files_async( return_exceptions=return_exceptions, **progress_options ) - return _output_handler(config, datasets, results) + + output_dict = _output_handler(config, datasets, results) + + if display_results: + _display_results(output_dict) + + return output_dict deliver = make_sync(deliver_async) From 33bbea235f6c7dda1a1d660aa1f93a0d4a81c5f6 Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Tue, 9 Sep 2025 14:01:19 -0500 Subject: [PATCH 2/8] fix breaking test --- tests/app/test_app.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/app/test_app.py b/tests/app/test_app.py index 76ced5c5..bc0a3c45 100644 --- a/tests/app/test_app.py +++ b/tests/app/test_app.py @@ -45,6 +45,10 @@ def test_deliver(script_runner): assert result.returncode == 0 result_rows = result.stdout.split("\n") assert result_rows[0] == "Delivering foo.yaml to ServiceX cache" - assert ( - result_rows[1] == "{'UprootRaw_YAML': ['/tmp/foo.root', '/tmp/bar.root']}" + mock_servicex_client.deliver.assert_called_once_with( + "foo.yaml", + servicex_name=None, + config_path=None, + ignore_local_cache=None, + display_results=True, ) From e70586bdcb15a5d6d0180620d08c9a9a589af13b Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Tue, 9 Sep 2025 15:38:01 -0500 Subject: [PATCH 3/8] add new test to improve code coverage --- tests/app/test_app.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/app/test_app.py b/tests/app/test_app.py index bc0a3c45..6a8cdd07 100644 --- a/tests/app/test_app.py +++ b/tests/app/test_app.py @@ -52,3 +52,24 @@ def test_deliver(script_runner): ignore_local_cache=None, display_results=True, ) + + +def test_deliver_hide_results(script_runner): + with patch("servicex.app.main.servicex_client") as mock_servicex_client: + mock_servicex_client.deliver = Mock( + return_value={"UprootRaw_YAML": ["/tmp/foo.root", "/tmp/bar.root"]} + ) + result = script_runner.run( + ["servicex", "deliver", "foo.yaml", "--hide-results"] + ) + assert result.returncode == 0 + result_rows = result.stdout.split("\n") + assert result_rows[0] == "Delivering foo.yaml to ServiceX cache" + # Verify that servicex_client.deliver was called with display_results=False + mock_servicex_client.deliver.assert_called_once_with( + "foo.yaml", + servicex_name=None, + config_path=None, + ignore_local_cache=None, + display_results=False, + ) From 36b32b61f8a3ba389eea25063e60a23cc9a94de4 Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Tue, 9 Sep 2025 15:49:22 -0500 Subject: [PATCH 4/8] add new test --- servicex/servicex_client.py | 5 +- tests/test_display_results.py | 222 ++++++++++++++++++++++++++++++++++ 2 files changed, 225 insertions(+), 2 deletions(-) create mode 100644 tests/test_display_results.py diff --git a/servicex/servicex_client.py b/servicex/servicex_client.py index 535050ab..5b5079c6 100644 --- a/servicex/servicex_client.py +++ b/servicex/servicex_client.py @@ -52,7 +52,6 @@ from collections.abc import Sequence, Coroutine from enum import Enum import traceback -from rich.console import Console from rich.table import Table T = TypeVar("T") @@ -237,7 +236,9 @@ def _output_handler( def _display_results(out_dict): """Display the delivery results using rich styling.""" - console = Console() + from rich import get_console + + console = get_console() console.print("\n[bold green]✓ ServiceX Delivery Complete![/bold green]\n") diff --git a/tests/test_display_results.py b/tests/test_display_results.py new file mode 100644 index 00000000..d306fbb5 --- /dev/null +++ b/tests/test_display_results.py @@ -0,0 +1,222 @@ +# Copyright (c) 2024, IRIS-HEP +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# +# * Redistributions of source code must retain the above copyright notice, this +# list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# +# * Neither the name of the copyright holder nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +from unittest.mock import Mock, patch + +from servicex.servicex_client import GuardList, _display_results + + +def test_display_results_with_valid_files(): + """Test _display_results with valid GuardList containing files.""" + # Create actual GuardList with valid files + files = ["/tmp/file1.root", "/tmp/file2.root"] + guard_list = GuardList(files) + + out_dict = {"UprootRaw_YAML": guard_list} + + with patch("rich.get_console") as mock_get_console: + mock_console = Mock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table") as mock_table_class: + mock_table = Mock() + mock_table_class.return_value = mock_table + + _display_results(out_dict) + + # Verify get_console was called + mock_get_console.assert_called_once() + + # Verify console.print was called for completion message + assert ( + mock_console.print.call_count >= 2 + ) # At least completion message and table + + # Verify Table was created with proper parameters + mock_table_class.assert_called_once_with( + title="Delivered Files", show_header=True, header_style="bold magenta" + ) + + # Verify table columns were added + expected_calls = [ + (("Sample",), {"style": "cyan", "no_wrap": True}), + (("File Count",), {"justify": "right", "style": "green"}), + (("Files",), {"style": "dim"}), + ] + for expected_args, expected_kwargs in expected_calls: + mock_table.add_column.assert_any_call(*expected_args, **expected_kwargs) + + # Verify table row was added + mock_table.add_row.assert_called_once_with( + "UprootRaw_YAML", "2", "/tmp/file1.root\n/tmp/file2.root" + ) + + +def test_display_results_with_many_files(): + """Test _display_results with more than 3 files (ellipsis case).""" + files_list = [f"/tmp/file{i}.root" for i in range(1, 6)] # 5 files + guard_list = GuardList(files_list) + + out_dict = {"Sample1": guard_list} + + with patch("rich.get_console") as mock_get_console: + mock_console = Mock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table") as mock_table_class: + mock_table = Mock() + mock_table_class.return_value = mock_table + + _display_results(out_dict) + + # Verify table row was added with ellipsis + expected_files_display = ( + "/tmp/file1.root\n/tmp/file2.root\n... and 3 more files" + ) + mock_table.add_row.assert_called_once_with( + "Sample1", "5", expected_files_display + ) + + +def test_display_results_with_invalid_files(): + """Test _display_results with invalid GuardList (error case).""" + # Create GuardList with an exception to make it invalid + guard_list = GuardList(ValueError("Sample error")) + + out_dict = {"FailedSample": guard_list} + + with patch("rich.get_console") as mock_get_console: + mock_console = Mock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table") as mock_table_class: + mock_table = Mock() + mock_table_class.return_value = mock_table + + _display_results(out_dict) + + # Verify error row was added + mock_table.add_row.assert_called_once_with( + "FailedSample", + "[red]Error[/red]", + "[red]Failed to retrieve files[/red]", + ) + + +def test_display_results_with_mixed_samples(): + """Test _display_results with both valid and invalid samples.""" + # Valid sample + valid_guard_list = GuardList(["/tmp/valid.root"]) + + # Invalid sample + invalid_guard_list = GuardList(ValueError("Invalid sample")) + + out_dict = {"ValidSample": valid_guard_list, "InvalidSample": invalid_guard_list} + + with patch("rich.get_console") as mock_get_console: + mock_console = Mock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table") as mock_table_class: + mock_table = Mock() + mock_table_class.return_value = mock_table + + _display_results(out_dict) + + # Verify both rows were added + assert mock_table.add_row.call_count == 2 + + # Check that both types of calls were made + calls = mock_table.add_row.call_args_list + call_args = [call[0] for call in calls] + + # Should have one valid and one error call + assert ("ValidSample", "1", "/tmp/valid.root") in call_args + assert ( + "InvalidSample", + "[red]Error[/red]", + "[red]Failed to retrieve files[/red]", + ) in call_args + + +def test_display_results_total_files_calculation(): + """Test that total files count is calculated correctly.""" + # Sample 1: 2 files + guard_list1 = GuardList(["/tmp/file1.root", "/tmp/file2.root"]) + + # Sample 2: 3 files + guard_list2 = GuardList(["/tmp/file3.root", "/tmp/file4.root", "/tmp/file5.root"]) + + # Sample 3: Invalid (should not count) + guard_list3 = GuardList(ValueError("Invalid sample")) + + out_dict = {"Sample1": guard_list1, "Sample2": guard_list2, "Sample3": guard_list3} + + with patch("rich.get_console") as mock_get_console: + mock_console = Mock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table"): + _display_results(out_dict) + + # Check that the total files message includes the correct count (2 + 3 = 5) + print_calls = mock_console.print.call_args_list + total_message_call = None + for call in print_calls: + if call[0] and "Total files delivered: 5" in str(call[0][0]): + total_message_call = call + break + + assert ( + total_message_call is not None + ), f"Expected total files message not found in calls: {print_calls}" + + +def test_display_results_console_print_calls(): + """Test that all expected console.print calls are made.""" + guard_list = GuardList(["/tmp/file.root"]) + + out_dict = {"Sample": guard_list} + + with patch("rich.get_console") as mock_get_console: + mock_console = Mock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table") as mock_table_class: + mock_table = Mock() + mock_table_class.return_value = mock_table + + _display_results(out_dict) + + # Should have exactly 3 print calls: + # 1. Completion message + # 2. Table + # 3. Total files message + assert mock_console.print.call_count == 3 + + # Verify console.print was called with table + mock_console.print.assert_any_call(mock_table) From f7eb21d2680449db7ae589b61cc49a13369299ef Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Tue, 9 Sep 2025 15:55:16 -0500 Subject: [PATCH 5/8] update test --- tests/app/test_app.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/app/test_app.py b/tests/app/test_app.py index 6a8cdd07..76531dec 100644 --- a/tests/app/test_app.py +++ b/tests/app/test_app.py @@ -1,8 +1,3 @@ -# Copyright (c) 2024, IRIS-HEP -# All rights reserved. -# -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are met: # # * Redistributions of source code must retain the above copyright notice, this # list of conditions and the following disclaimer. @@ -25,6 +20,7 @@ # CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, # OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + from unittest.mock import Mock, patch From 269559079d884d72c86e98c8359a3a3ea725304e Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Tue, 9 Sep 2025 16:13:30 -0500 Subject: [PATCH 6/8] add coverage for ValueError --- servicex/servicex_client.py | 21 +-- tests/test_display_results.py | 259 +++++++--------------------------- 2 files changed, 66 insertions(+), 214 deletions(-) diff --git a/servicex/servicex_client.py b/servicex/servicex_client.py index 5b5079c6..9ce45d8f 100644 --- a/servicex/servicex_client.py +++ b/servicex/servicex_client.py @@ -234,6 +234,18 @@ def _output_handler( return out_dict +def _get_progress_options(progress_bar: ProgressBarFormat) -> dict: + """Get progress options based on progress bar format.""" + if progress_bar == ProgressBarFormat.expanded: + return {} + elif progress_bar == ProgressBarFormat.compact: + return {"overall_progress": True} + elif progress_bar == ProgressBarFormat.none: + return {"display_progress": False} + else: + raise ValueError(f"Invalid value {progress_bar} for progress_bar provided") + + def _display_results(out_dict): """Display the delivery results using rich styling.""" from rich import get_console @@ -326,14 +338,7 @@ async def deliver_async( group = DatasetGroup(datasets) - if progress_bar == ProgressBarFormat.expanded: - progress_options = {} - elif progress_bar == ProgressBarFormat.compact: - progress_options = {"overall_progress": True} - elif progress_bar == ProgressBarFormat.none: - progress_options = {"display_progress": False} - else: - raise ValueError(f"Invalid value {progress_bar} for progress_bar provided") + progress_options = _get_progress_options(progress_bar) if config.General.Delivery not in [ General.DeliveryEnum.URLs, diff --git a/tests/test_display_results.py b/tests/test_display_results.py index d306fbb5..9b57db19 100644 --- a/tests/test_display_results.py +++ b/tests/test_display_results.py @@ -1,180 +1,27 @@ # Copyright (c) 2024, IRIS-HEP # All rights reserved. # -# Redistribution and use in source and binary forms, with or without -# modification, are permitted provided that the following conditions are met: -# -# * Redistributions of source code must retain the above copyright notice, this -# list of conditions and the following disclaimer. -# -# * Redistributions in binary form must reproduce the above copyright notice, -# this list of conditions and the following disclaimer in the documentation -# and/or other materials provided with the distribution. -# -# * Neither the name of the copyright holder nor the names of its -# contributors may be used to endorse or promote products derived from -# this software without specific prior written permission. -# -# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" -# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE -# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE -# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE -# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL -# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR -# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER -# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, -# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +# Simplified tests focusing on essential coverage only from unittest.mock import Mock, patch +import pytest -from servicex.servicex_client import GuardList, _display_results - - -def test_display_results_with_valid_files(): - """Test _display_results with valid GuardList containing files.""" - # Create actual GuardList with valid files - files = ["/tmp/file1.root", "/tmp/file2.root"] - guard_list = GuardList(files) - - out_dict = {"UprootRaw_YAML": guard_list} - - with patch("rich.get_console") as mock_get_console: - mock_console = Mock() - mock_get_console.return_value = mock_console - - with patch("servicex.servicex_client.Table") as mock_table_class: - mock_table = Mock() - mock_table_class.return_value = mock_table - - _display_results(out_dict) - - # Verify get_console was called - mock_get_console.assert_called_once() - - # Verify console.print was called for completion message - assert ( - mock_console.print.call_count >= 2 - ) # At least completion message and table - - # Verify Table was created with proper parameters - mock_table_class.assert_called_once_with( - title="Delivered Files", show_header=True, header_style="bold magenta" - ) - - # Verify table columns were added - expected_calls = [ - (("Sample",), {"style": "cyan", "no_wrap": True}), - (("File Count",), {"justify": "right", "style": "green"}), - (("Files",), {"style": "dim"}), - ] - for expected_args, expected_kwargs in expected_calls: - mock_table.add_column.assert_any_call(*expected_args, **expected_kwargs) - - # Verify table row was added - mock_table.add_row.assert_called_once_with( - "UprootRaw_YAML", "2", "/tmp/file1.root\n/tmp/file2.root" - ) - - -def test_display_results_with_many_files(): - """Test _display_results with more than 3 files (ellipsis case).""" - files_list = [f"/tmp/file{i}.root" for i in range(1, 6)] # 5 files - guard_list = GuardList(files_list) - - out_dict = {"Sample1": guard_list} - - with patch("rich.get_console") as mock_get_console: - mock_console = Mock() - mock_get_console.return_value = mock_console - - with patch("servicex.servicex_client.Table") as mock_table_class: - mock_table = Mock() - mock_table_class.return_value = mock_table - - _display_results(out_dict) - - # Verify table row was added with ellipsis - expected_files_display = ( - "/tmp/file1.root\n/tmp/file2.root\n... and 3 more files" - ) - mock_table.add_row.assert_called_once_with( - "Sample1", "5", expected_files_display - ) - - -def test_display_results_with_invalid_files(): - """Test _display_results with invalid GuardList (error case).""" - # Create GuardList with an exception to make it invalid - guard_list = GuardList(ValueError("Sample error")) - - out_dict = {"FailedSample": guard_list} - - with patch("rich.get_console") as mock_get_console: - mock_console = Mock() - mock_get_console.return_value = mock_console - - with patch("servicex.servicex_client.Table") as mock_table_class: - mock_table = Mock() - mock_table_class.return_value = mock_table - - _display_results(out_dict) - - # Verify error row was added - mock_table.add_row.assert_called_once_with( - "FailedSample", - "[red]Error[/red]", - "[red]Failed to retrieve files[/red]", - ) - - -def test_display_results_with_mixed_samples(): - """Test _display_results with both valid and invalid samples.""" - # Valid sample - valid_guard_list = GuardList(["/tmp/valid.root"]) - - # Invalid sample - invalid_guard_list = GuardList(ValueError("Invalid sample")) - - out_dict = {"ValidSample": valid_guard_list, "InvalidSample": invalid_guard_list} - - with patch("rich.get_console") as mock_get_console: - mock_console = Mock() - mock_get_console.return_value = mock_console - - with patch("servicex.servicex_client.Table") as mock_table_class: - mock_table = Mock() - mock_table_class.return_value = mock_table - - _display_results(out_dict) - - # Verify both rows were added - assert mock_table.add_row.call_count == 2 +from servicex.servicex_client import ( + GuardList, + _display_results, + _get_progress_options, + ProgressBarFormat, + ServiceXClient, +) - # Check that both types of calls were made - calls = mock_table.add_row.call_args_list - call_args = [call[0] for call in calls] - # Should have one valid and one error call - assert ("ValidSample", "1", "/tmp/valid.root") in call_args - assert ( - "InvalidSample", - "[red]Error[/red]", - "[red]Failed to retrieve files[/red]", - ) in call_args +def test_display_results_basic(): + """Test _display_results basic functionality - covers most code paths.""" + # Test with valid files (covers main path) + valid_files = GuardList(["/tmp/file1.root", "/tmp/file2.root"]) + # Test with error case (covers error path) + error_files = GuardList(ValueError("Test error")) - -def test_display_results_total_files_calculation(): - """Test that total files count is calculated correctly.""" - # Sample 1: 2 files - guard_list1 = GuardList(["/tmp/file1.root", "/tmp/file2.root"]) - - # Sample 2: 3 files - guard_list2 = GuardList(["/tmp/file3.root", "/tmp/file4.root", "/tmp/file5.root"]) - - # Sample 3: Invalid (should not count) - guard_list3 = GuardList(ValueError("Invalid sample")) - - out_dict = {"Sample1": guard_list1, "Sample2": guard_list2, "Sample3": guard_list3} + out_dict = {"ValidSample": valid_files, "ErrorSample": error_files} with patch("rich.get_console") as mock_get_console: mock_console = Mock() @@ -183,40 +30,40 @@ def test_display_results_total_files_calculation(): with patch("servicex.servicex_client.Table"): _display_results(out_dict) - # Check that the total files message includes the correct count (2 + 3 = 5) - print_calls = mock_console.print.call_args_list - total_message_call = None - for call in print_calls: - if call[0] and "Total files delivered: 5" in str(call[0][0]): - total_message_call = call - break - - assert ( - total_message_call is not None - ), f"Expected total files message not found in calls: {print_calls}" - - -def test_display_results_console_print_calls(): - """Test that all expected console.print calls are made.""" - guard_list = GuardList(["/tmp/file.root"]) - - out_dict = {"Sample": guard_list} - - with patch("rich.get_console") as mock_get_console: - mock_console = Mock() - mock_get_console.return_value = mock_console - - with patch("servicex.servicex_client.Table") as mock_table_class: - mock_table = Mock() - mock_table_class.return_value = mock_table - - _display_results(out_dict) - - # Should have exactly 3 print calls: - # 1. Completion message - # 2. Table - # 3. Total files message - assert mock_console.print.call_count == 3 - - # Verify console.print was called with table - mock_console.print.assert_any_call(mock_table) + # Just verify it was called - don't over-test internal details + mock_get_console.assert_called_once() + assert mock_console.print.call_count >= 2 # At least completion + total + + +def test_essential_valueerrors(): + """Test the most important ValueError cases in one simple test.""" + # Test progress options + assert _get_progress_options(ProgressBarFormat.expanded) == {} + with pytest.raises(ValueError, match="Invalid value"): + _get_progress_options("invalid") + + # Test ServiceX client errors - simplest possible + with pytest.raises(ValueError, match="Only specify backend or url"): + with patch("servicex.servicex_client.Configuration") as mock_config_class: + mock_config = Mock() + mock_config.endpoint_dict.return_value = {} + mock_config.default_endpoint = None + mock_config_class.read.return_value = mock_config + ServiceXClient(backend="test", url="http://test.com") + + +def test_guardlist_basics(): + """Test GuardList basic functionality.""" + # Valid case + valid_list = GuardList([1, 2, 3]) + assert len(valid_list) == 3 + assert valid_list[0] == 1 + assert valid_list.valid() + + # Error case + from servicex.servicex_client import ReturnValueException + + error_list = GuardList(ValueError("error")) + assert not error_list.valid() + with pytest.raises(ReturnValueException): + _ = error_list[0] From 110f2301167f2671ac19735ffb837762a5f03dd5 Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Wed, 10 Sep 2025 11:16:14 -0500 Subject: [PATCH 7/8] add tests to correct code coverage --- tests/test_servicex_client.py | 64 +++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/tests/test_servicex_client.py b/tests/test_servicex_client.py index 3df5ffde..444d7ea3 100644 --- a/tests/test_servicex_client.py +++ b/tests/test_servicex_client.py @@ -149,3 +149,67 @@ def test_invalid_backend_raises_error_with_filename(): ServiceXClient(backend="badname", config_path=config_file) assert f"Backend badname not defined in {expected} file" in str(err.value) + + +def test_display_results_with_many_files(): + from servicex.servicex_client import _display_results, GuardList + from unittest.mock import patch, MagicMock + + # Mock GuardList with more than 3 files to trigger lines 275-276 + mock_guard_list = MagicMock(spec=GuardList) + mock_guard_list.valid.return_value = True + mock_guard_list.__iter__.return_value = iter( + [ + "file1.parquet", + "file2.parquet", + "file3.parquet", + "file4.parquet", + "file5.parquet", + ] + ) + + out_dict = {"sample1": mock_guard_list} + + with patch("rich.get_console") as mock_get_console: + mock_console = MagicMock() + mock_get_console.return_value = mock_console + + with patch("servicex.servicex_client.Table") as mock_table: + mock_table_instance = MagicMock() + mock_table.return_value = mock_table_instance + + _display_results(out_dict) + + # Verify that add_row was called with the truncated file list + mock_table_instance.add_row.assert_called_once() + call_args = mock_table_instance.add_row.call_args[0] + assert call_args[0] == "sample1" + assert call_args[1] == "5" + assert "... and 3 more files" in call_args[2] + + +@pytest.mark.asyncio +async def test_deliver_async_invalid_delivery_config(): + from servicex.servicex_client import deliver_async + from unittest.mock import patch, MagicMock + + # Mock the config loading to return invalid delivery type + with patch("servicex.servicex_client._load_ServiceXSpec") as mock_load_spec: + with patch("servicex.servicex_client._build_datasets") as mock_build_datasets: + with patch("servicex.minio_adapter.init_s3_config"): + mock_config = MagicMock() + mock_config.General.Delivery = ( + "INVALID_DELIVERY" # Invalid delivery type + ) + mock_config.General.IgnoreLocalCache = False + mock_config.Sample = [] + mock_load_spec.return_value = mock_config + mock_build_datasets.return_value = [] + + with pytest.raises(ValueError) as exc_info: + await deliver_async("test_spec.yaml") + + assert ( + "unexpected value for config.general.Delivery: INVALID_DELIVERY" + in str(exc_info.value) + ) From 550cb800635c8c0ff277f227b01c647436f6e7bf Mon Sep 17 00:00:00 2001 From: Matt Shirley Date: Wed, 10 Sep 2025 13:45:02 -0500 Subject: [PATCH 8/8] align license text --- tests/app/test_app.py | 5 +++++ tests/test_display_results.py | 27 +++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/tests/app/test_app.py b/tests/app/test_app.py index 76531dec..3af68c8b 100644 --- a/tests/app/test_app.py +++ b/tests/app/test_app.py @@ -1,3 +1,8 @@ +# Copyright (c) 2024, IRIS-HEP +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: # # * Redistributions of source code must retain the above copyright notice, this # list of conditions and the following disclaimer. diff --git a/tests/test_display_results.py b/tests/test_display_results.py index 9b57db19..d8201920 100644 --- a/tests/test_display_results.py +++ b/tests/test_display_results.py @@ -1,7 +1,30 @@ -# Copyright (c) 2024, IRIS-HEP +# Copyright (c) 2022, IRIS-HEP # All rights reserved. # -# Simplified tests focusing on essential coverage only +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# +# * Redistributions of source code must retain the above copyright notice, this +# list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright notice, +# this list of conditions and the following disclaimer in the documentation +# and/or other materials provided with the distribution. +# +# * Neither the name of the copyright holder nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER +# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. from unittest.mock import Mock, patch import pytest