Skip to content

Commit 7124c94

Browse files
committed
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.
1 parent 441af3a commit 7124c94

10 files changed

Lines changed: 64 additions & 9 deletions

File tree

Lib/profiling/sampling/binary_collector.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ def export(self, filename=None):
9494
filename: Ignored (binary files are written incrementally)
9595
"""
9696
self._writer.finalize()
97+
return True
9798

9899
@property
99100
def total_samples(self):

Lib/profiling/sampling/cli.py

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -789,11 +789,12 @@ def progress_callback(current, total):
789789
args.outfile
790790
or _generate_output_filename(args.format, os.getpid())
791791
)
792-
collector.export(filename)
792+
export_ok = collector.export(filename)
793793

794794
# Auto-open browser for HTML output if --browser flag is set
795795
if (
796-
args.format in (
796+
export_ok
797+
and args.format in (
797798
'flamegraph', 'diff_flamegraph', 'heatmap'
798799
)
799800
and getattr(args, 'browser', False)
@@ -840,10 +841,14 @@ def _handle_output(collector, args, pid, mode):
840841
filename = os.path.join(args.outfile, _generate_output_filename(args.format, pid))
841842
else:
842843
filename = args.outfile or _generate_output_filename(args.format, pid)
843-
collector.export(filename)
844+
export_ok = collector.export(filename)
844845

845846
# Auto-open browser for HTML output if --browser flag is set
846-
if args.format in ('flamegraph', 'diff_flamegraph', 'heatmap') and getattr(args, 'browser', False):
847+
if (
848+
export_ok
849+
and args.format in ('flamegraph', 'diff_flamegraph', 'heatmap')
850+
and getattr(args, 'browser', False)
851+
):
847852
_open_in_browser(filename)
848853

849854

Lib/profiling/sampling/collector.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,11 @@ def collect_failed_sample(self):
163163

164164
@abstractmethod
165165
def export(self, filename):
166-
"""Export collected data to a file."""
166+
"""Export collected data.
167+
168+
Returns:
169+
bool: True if output was generated, False if there was no data to export.
170+
"""
167171

168172
@staticmethod
169173
def _filter_internal_frames(frames):

Lib/profiling/sampling/gecko_collector.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,7 @@ def spin():
699699
print(
700700
f"Open in Firefox Profiler: https://profiler.firefox.com/"
701701
)
702+
return True
702703

703704
def _build_marker_schema(self):
704705
"""Build marker schema definitions for Firefox Profiler."""

Lib/profiling/sampling/heatmap_collector.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -598,7 +598,7 @@ def export(self, output_path):
598598
"""
599599
if not self.file_samples:
600600
print("Warning: No heatmap data to export")
601-
return
601+
return False
602602

603603
try:
604604
output_dir = self._prepare_output_directory(output_path)
@@ -612,6 +612,7 @@ def export(self, output_path):
612612
self._generate_index_html(output_dir / 'index.html', file_stats)
613613

614614
self._print_export_summary(output_dir, file_stats)
615+
return True
615616

616617
except Exception as e:
617618
print(f"Error: Failed to export heatmap: {e}")

Lib/profiling/sampling/jsonl_collector.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,7 @@ def export(self, filename):
165165
)
166166
self._write_message(output, self._build_end_record())
167167
print(f"JSONL profile written to {filename}")
168+
return True
168169

169170
def _build_meta_record(self):
170171
record = {

Lib/profiling/sampling/pstats_collector.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ def collect(self, stack_frames, timestamps_us=None):
6363
def export(self, filename):
6464
self.create_stats()
6565
self._dump_stats(filename)
66+
return True
6667

6768
def _dump_stats(self, file):
6869
stats_with_marker = dict(self.stats)

Lib/profiling/sampling/stack_collector.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ def export(self, filename):
6464
for stack, count in lines:
6565
f.write(f"{stack} {count}\n")
6666
print(f"Collapsed stack output written to {filename}")
67+
return True
6768

6869

6970
class FlamegraphCollector(StackTraceCollector):
@@ -161,14 +162,15 @@ def export(self, filename):
161162
print(
162163
"Warning: No functions found in profiling data. Check if sampling captured any data."
163164
)
164-
return
165+
return False
165166

166167
html_content = self._create_flamegraph_html(flamegraph_data)
167168

168169
with open(filename, "w", encoding="utf-8") as f:
169170
f.write(html_content)
170171

171172
print(f"Flamegraph saved to: {filename}")
173+
return True
172174

173175
@staticmethod
174176
@functools.lru_cache(maxsize=None)

Lib/test/test_profiling/test_sampling_profiler/test_cli.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import sys
88
import tempfile
99
import unittest
10+
from types import SimpleNamespace
1011
from unittest import mock
1112

1213
try:
@@ -26,6 +27,7 @@
2627
FORMAT_EXTENSIONS,
2728
_create_collector,
2829
_generate_output_filename,
30+
_handle_output,
2931
main,
3032
)
3133
from profiling.sampling.constants import (
@@ -727,6 +729,26 @@ def test_async_aware_flag_defaults_to_running(self):
727729
call_kwargs = mock_sample.call_args[1]
728730
self.assertEqual(call_kwargs.get("async_aware"), "running")
729731

732+
def test_handle_output_browser_not_opened_when_export_fails(self):
733+
for format_type in ("flamegraph", "diff_flamegraph", "heatmap"):
734+
with self.subTest(format=format_type):
735+
collector = mock.MagicMock()
736+
collector.export.return_value = False
737+
args = SimpleNamespace(
738+
format=format_type,
739+
outfile="profile.html",
740+
browser=True,
741+
)
742+
743+
with (
744+
mock.patch("profiling.sampling.cli.os.path.isdir", return_value=False),
745+
mock.patch("profiling.sampling.cli._open_in_browser") as mock_open,
746+
):
747+
_handle_output(collector, args, pid=12345, mode=0)
748+
749+
collector.export.assert_called_once_with("profile.html")
750+
mock_open.assert_not_called()
751+
730752
def test_async_aware_with_async_mode_all(self):
731753
"""Test --async-aware with --async-mode all."""
732754
test_args = ["profiling.sampling.cli", "attach", "12345", "--async-aware", "--async-mode", "all"]

Lib/test/test_profiling/test_sampling_profiler/test_collectors.py

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -502,9 +502,10 @@ def test_flamegraph_collector_export(self):
502502

503503
# Export flamegraph
504504
with captured_stdout(), captured_stderr():
505-
collector.export(flamegraph_out.name)
505+
export_ok = collector.export(flamegraph_out.name)
506506

507507
# Verify file was created and contains valid data
508+
self.assertTrue(export_ok)
508509
self.assertTrue(os.path.exists(flamegraph_out.name))
509510
self.assertGreater(os.path.getsize(flamegraph_out.name), 0)
510511

@@ -523,6 +524,21 @@ def test_flamegraph_collector_export(self):
523524
self.assertIn('"value":', content)
524525
self.assertIn('"children":', content)
525526

527+
def test_flamegraph_collector_empty_export_fails(self):
528+
"""Test empty flamegraph export reports no output."""
529+
flamegraph_out = tempfile.NamedTemporaryFile(
530+
suffix=".html", delete=False
531+
)
532+
self.addCleanup(close_and_unlink, flamegraph_out)
533+
534+
collector = FlamegraphCollector(1000)
535+
536+
with captured_stdout(), captured_stderr():
537+
export_ok = collector.export(flamegraph_out.name)
538+
539+
self.assertFalse(export_ok)
540+
self.assertEqual(os.path.getsize(flamegraph_out.name), 0)
541+
526542
def test_gecko_collector_basic(self):
527543
"""Test basic GeckoCollector functionality."""
528544
collector = GeckoCollector(1000)
@@ -1552,8 +1568,9 @@ def test_diff_flamegraph_export(self):
15521568
self.addCleanup(close_and_unlink, flamegraph_out)
15531569

15541570
with captured_stdout(), captured_stderr():
1555-
diff.export(flamegraph_out.name)
1571+
export_ok = diff.export(flamegraph_out.name)
15561572

1573+
self.assertTrue(export_ok)
15571574
self.assertTrue(os.path.exists(flamegraph_out.name))
15581575
self.assertGreater(os.path.getsize(flamegraph_out.name), 0)
15591576

0 commit comments

Comments
 (0)