From c9e483e4650abafde1323a3c023e4cdd0749606e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1szl=C3=B3=20Kiss=20Koll=C3=A1r?= Date: Thu, 14 May 2026 13:47:48 +0100 Subject: [PATCH] Fix browser open after empty export Make sampling exporters report whether output was generated, and only open HTML reports in the browser after a successful export. --- Lib/profiling/sampling/binary_collector.py | 1 + Lib/profiling/sampling/cli.py | 13 +++++++---- Lib/profiling/sampling/collector.py | 6 ++++- Lib/profiling/sampling/gecko_collector.py | 1 + Lib/profiling/sampling/heatmap_collector.py | 3 ++- Lib/profiling/sampling/jsonl_collector.py | 1 + Lib/profiling/sampling/pstats_collector.py | 1 + Lib/profiling/sampling/stack_collector.py | 4 +++- .../test_sampling_profiler/test_cli.py | 22 +++++++++++++++++++ .../test_sampling_profiler/test_collectors.py | 21 ++++++++++++++++-- 10 files changed, 64 insertions(+), 9 deletions(-) diff --git a/Lib/profiling/sampling/binary_collector.py b/Lib/profiling/sampling/binary_collector.py index 64afe632fae175..afbbc829269067 100644 --- a/Lib/profiling/sampling/binary_collector.py +++ b/Lib/profiling/sampling/binary_collector.py @@ -94,6 +94,7 @@ def export(self, filename=None): filename: Ignored (binary files are written incrementally) """ self._writer.finalize() + return True @property def total_samples(self): diff --git a/Lib/profiling/sampling/cli.py b/Lib/profiling/sampling/cli.py index a5d9573ae6b6dd..4d48034d864458 100644 --- a/Lib/profiling/sampling/cli.py +++ b/Lib/profiling/sampling/cli.py @@ -789,11 +789,12 @@ def progress_callback(current, total): args.outfile or _generate_output_filename(args.format, os.getpid()) ) - collector.export(filename) + export_ok = collector.export(filename) # Auto-open browser for HTML output if --browser flag is set if ( - args.format in ( + export_ok + and args.format in ( 'flamegraph', 'diff_flamegraph', 'heatmap' ) and getattr(args, 'browser', False) @@ -840,10 +841,14 @@ def _handle_output(collector, args, pid, mode): filename = os.path.join(args.outfile, _generate_output_filename(args.format, pid)) else: filename = args.outfile or _generate_output_filename(args.format, pid) - collector.export(filename) + export_ok = collector.export(filename) # Auto-open browser for HTML output if --browser flag is set - if args.format in ('flamegraph', 'diff_flamegraph', 'heatmap') and getattr(args, 'browser', False): + if ( + export_ok + and args.format in ('flamegraph', 'diff_flamegraph', 'heatmap') + and getattr(args, 'browser', False) + ): _open_in_browser(filename) diff --git a/Lib/profiling/sampling/collector.py b/Lib/profiling/sampling/collector.py index 81ec6344ebdea4..95389327320a4c 100644 --- a/Lib/profiling/sampling/collector.py +++ b/Lib/profiling/sampling/collector.py @@ -161,7 +161,11 @@ def collect_failed_sample(self): @abstractmethod def export(self, filename): - """Export collected data to a file.""" + """Export collected data. + + Returns: + bool: True if output was generated, False if there was no data to export. + """ @staticmethod def _filter_internal_frames(frames): diff --git a/Lib/profiling/sampling/gecko_collector.py b/Lib/profiling/sampling/gecko_collector.py index 8986194268b3ce..3cca44b8ff6100 100644 --- a/Lib/profiling/sampling/gecko_collector.py +++ b/Lib/profiling/sampling/gecko_collector.py @@ -697,6 +697,7 @@ def spin(): print( f"Open in Firefox Profiler: https://profiler.firefox.com/" ) + return True def _build_marker_schema(self): """Build marker schema definitions for Firefox Profiler.""" diff --git a/Lib/profiling/sampling/heatmap_collector.py b/Lib/profiling/sampling/heatmap_collector.py index 5c36d78f5535e7..e296207c340d43 100644 --- a/Lib/profiling/sampling/heatmap_collector.py +++ b/Lib/profiling/sampling/heatmap_collector.py @@ -597,7 +597,7 @@ def export(self, output_path): """ if not self.file_samples: print("Warning: No heatmap data to export") - return + return False try: output_dir = self._prepare_output_directory(output_path) @@ -611,6 +611,7 @@ def export(self, output_path): self._generate_index_html(output_dir / 'index.html', file_stats) self._print_export_summary(output_dir, file_stats) + return True except Exception as e: print(f"Error: Failed to export heatmap: {e}") diff --git a/Lib/profiling/sampling/jsonl_collector.py b/Lib/profiling/sampling/jsonl_collector.py index 7d26129b80de86..e2eaecf1ac15eb 100644 --- a/Lib/profiling/sampling/jsonl_collector.py +++ b/Lib/profiling/sampling/jsonl_collector.py @@ -164,6 +164,7 @@ def export(self, filename): self._iter_final_agg_entries(), ) self._write_message(output, self._build_end_record()) + return True def _build_meta_record(self): record = { diff --git a/Lib/profiling/sampling/pstats_collector.py b/Lib/profiling/sampling/pstats_collector.py index 50500296c15acc..4a78a6971668c2 100644 --- a/Lib/profiling/sampling/pstats_collector.py +++ b/Lib/profiling/sampling/pstats_collector.py @@ -61,6 +61,7 @@ def collect(self, stack_frames, timestamps_us=None): def export(self, filename): self.create_stats() self._dump_stats(filename) + return True def _dump_stats(self, file): stats_with_marker = dict(self.stats) diff --git a/Lib/profiling/sampling/stack_collector.py b/Lib/profiling/sampling/stack_collector.py index 60df026ed76a6c..4a08c1da31691d 100644 --- a/Lib/profiling/sampling/stack_collector.py +++ b/Lib/profiling/sampling/stack_collector.py @@ -62,6 +62,7 @@ def export(self, filename): for stack, count in lines: f.write(f"{stack} {count}\n") print(f"Collapsed stack output written to {filename}") + return True class FlamegraphCollector(StackTraceCollector): @@ -159,7 +160,7 @@ def export(self, filename): print( "Warning: No functions found in profiling data. Check if sampling captured any data." ) - return + return False html_content = self._create_flamegraph_html(flamegraph_data) @@ -167,6 +168,7 @@ def export(self, filename): f.write(html_content) print(f"Flamegraph saved to: {filename}") + return True @staticmethod @functools.lru_cache(maxsize=None) diff --git a/Lib/test/test_profiling/test_sampling_profiler/test_cli.py b/Lib/test/test_profiling/test_sampling_profiler/test_cli.py index 9c0734ac804e1b..379b7814cfd944 100644 --- a/Lib/test/test_profiling/test_sampling_profiler/test_cli.py +++ b/Lib/test/test_profiling/test_sampling_profiler/test_cli.py @@ -7,6 +7,7 @@ import sys import tempfile import unittest +from types import SimpleNamespace from unittest import mock try: @@ -26,6 +27,7 @@ FORMAT_EXTENSIONS, _create_collector, _generate_output_filename, + _handle_output, main, ) from profiling.sampling.constants import ( @@ -727,6 +729,26 @@ def test_async_aware_flag_defaults_to_running(self): call_kwargs = mock_sample.call_args[1] self.assertEqual(call_kwargs.get("async_aware"), "running") + def test_handle_output_browser_not_opened_when_export_fails(self): + for format_type in ("flamegraph", "diff_flamegraph", "heatmap"): + with self.subTest(format=format_type): + collector = mock.MagicMock() + collector.export.return_value = False + args = SimpleNamespace( + format=format_type, + outfile="profile.html", + browser=True, + ) + + with ( + mock.patch("profiling.sampling.cli.os.path.isdir", return_value=False), + mock.patch("profiling.sampling.cli._open_in_browser") as mock_open, + ): + _handle_output(collector, args, pid=12345, mode=0) + + collector.export.assert_called_once_with("profile.html") + mock_open.assert_not_called() + def test_async_aware_with_async_mode_all(self): """Test --async-aware with --async-mode all.""" test_args = ["profiling.sampling.cli", "attach", "12345", "--async-aware", "--async-mode", "all"] diff --git a/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py b/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py index 390a1479fdd297..1e67e722418c06 100644 --- a/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py +++ b/Lib/test/test_profiling/test_sampling_profiler/test_collectors.py @@ -502,9 +502,10 @@ def test_flamegraph_collector_export(self): # Export flamegraph with captured_stdout(), captured_stderr(): - collector.export(flamegraph_out.name) + export_ok = collector.export(flamegraph_out.name) # Verify file was created and contains valid data + self.assertTrue(export_ok) self.assertTrue(os.path.exists(flamegraph_out.name)) self.assertGreater(os.path.getsize(flamegraph_out.name), 0) @@ -523,6 +524,21 @@ def test_flamegraph_collector_export(self): self.assertIn('"value":', content) self.assertIn('"children":', content) + def test_flamegraph_collector_empty_export_fails(self): + """Test empty flamegraph export reports no output.""" + flamegraph_out = tempfile.NamedTemporaryFile( + suffix=".html", delete=False + ) + self.addCleanup(close_and_unlink, flamegraph_out) + + collector = FlamegraphCollector(1000) + + with captured_stdout(), captured_stderr(): + export_ok = collector.export(flamegraph_out.name) + + self.assertFalse(export_ok) + self.assertEqual(os.path.getsize(flamegraph_out.name), 0) + def test_gecko_collector_basic(self): """Test basic GeckoCollector functionality.""" collector = GeckoCollector(1000) @@ -1552,8 +1568,9 @@ def test_diff_flamegraph_export(self): self.addCleanup(close_and_unlink, flamegraph_out) with captured_stdout(), captured_stderr(): - diff.export(flamegraph_out.name) + export_ok = diff.export(flamegraph_out.name) + self.assertTrue(export_ok) self.assertTrue(os.path.exists(flamegraph_out.name)) self.assertGreater(os.path.getsize(flamegraph_out.name), 0)