From dc6bb2ac1f2bab9dcc8befb6d7f8615224a302ac Mon Sep 17 00:00:00 2001 From: Maggie Nolan Date: Thu, 28 Jun 2018 09:50:55 -0700 Subject: [PATCH] address comments --- internal/report/report.go | 6 +- internal/report/report_test.go | 76 ++++++------------- .../report/testdata/source_neg_samples.dot | 17 ----- .../report/testdata/source_neg_samples.rpt | 34 +++++++++ 4 files changed, 59 insertions(+), 74 deletions(-) delete mode 100644 internal/report/testdata/source_neg_samples.dot create mode 100644 internal/report/testdata/source_neg_samples.rpt diff --git a/internal/report/report.go b/internal/report/report.go index 2cd8ce57f..ca0272ef0 100644 --- a/internal/report/report.go +++ b/internal/report/report.go @@ -84,7 +84,7 @@ type Options struct { // Generate generates a report as directed by the Report. func Generate(w io.Writer, rpt *Report, obj plugin.ObjTool, ui plugin.UI) error { if rpt.unexpNegSamples { - ui.PrintErr("Negative sample values appeared in the profile.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead.") + ui.PrintErr("Profile has negative sample values.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead.") } o := rpt.options @@ -1207,7 +1207,7 @@ func New(prof *profile.Profile, o *Options) *Report { return measurement.ScaledLabel(v, o.SampleUnit, o.OutputUnit) } total, unexpNegSamples := computeTotal(prof, o.SampleValue, o.SampleMeanDivisor) - return &Report{prof, total, o, format, unexpNegSamples} + return &Report{prof, total, unexpNegSamples, o, format} } // NewDefault builds a new report indexing the last sample value @@ -1268,9 +1268,9 @@ func computeTotal(prof *profile.Profile, value, meanDiv func(v []int64) int64) ( type Report struct { prof *profile.Profile total int64 + unexpNegSamples bool options *Options formatValue func(int64) string - unexpNegSamples bool } // Total returns the total number of samples in a report. diff --git a/internal/report/report_test.go b/internal/report/report_test.go index 04fd220e7..d1c68db1a 100644 --- a/internal/report/report_test.go +++ b/internal/report/report_test.go @@ -39,25 +39,31 @@ func TestSource(t *testing.T) { sampleValue1 := func(v []int64) int64 { return v[1] } - + negProfile := testProfile.Copy() + negProfile.Sample = negProfile.Sample[:2] + for _, sample := range negProfile.Sample { + for i, v := range sample.Value { + sample.Value[i] = -v + } + } for _, tc := range []testcase{ { rpt: New( - testProfile1.Copy(), + testProfile.Copy(), &Options{ OutputFormat: List, Symbol: regexp.MustCompile(`.`), TrimPath: "/some/path", SampleValue: sampleValue1, - SampleUnit: testProfile1.SampleType[1].Unit, + SampleUnit: testProfile.SampleType[1].Unit, }, ), want: path + "source.rpt", }, { rpt: New( - testProfile1.Copy(), + testProfile.Copy(), &Options{ OutputFormat: Dot, CallTree: true, @@ -65,32 +71,29 @@ func TestSource(t *testing.T) { TrimPath: "/some/path", SampleValue: sampleValue1, - SampleUnit: testProfile1.SampleType[1].Unit, + SampleUnit: testProfile.SampleType[1].Unit, }, ), want: path + "source.dot", }, { rpt: New( - testProfile2.Copy(), + negProfile.Copy(), &Options{ - OutputFormat: Dot, - CallTree: true, + OutputFormat: List, Symbol: regexp.MustCompile(`.`), - TrimPath: "/some/path", - - SampleValue: sampleValue1, - SampleUnit: testProfile2.SampleType[1].Unit, + SampleValue: sampleValue1, + SampleUnit: negProfile.SampleType[1].Unit, }, ), wantNegSampleErr: true, - want: path + "source_neg_samples.dot", + want: path + "source_neg_samples.rpt", }, } { var b bytes.Buffer ui := &proftest.TestUI{T: t} if tc.wantNegSampleErr { - ui.AllowRx = "Negative sample values appeared in the profile.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead." + ui.AllowRx = "Profile has negative sample values.\nIf using the -base flag to compare profiles, consider using the -diff_base flag instead." } if err := Generate(&b, tc.rpt, &binutils.Binutils{}, ui); err != nil { t.Fatalf("%s: %v", tc.want, err) @@ -203,7 +206,7 @@ var testL = []*profile.Location{ }, } -var testProfile1 = &profile.Profile{ +var testProfile = &profile.Profile{ PeriodType: &profile.ValueType{Type: "cpu", Unit: "millisecond"}, Period: 10, DurationNanos: 10e9, @@ -238,41 +241,6 @@ var testProfile1 = &profile.Profile{ Mapping: testM, } -var testProfile2 = &profile.Profile{ - PeriodType: &profile.ValueType{Type: "cpu", Unit: "millisecond"}, - Period: 10, - DurationNanos: 10e9, - SampleType: []*profile.ValueType{ - {Type: "samples", Unit: "count"}, - {Type: "cpu", Unit: "cycles"}, - }, - Sample: []*profile.Sample{ - { - Location: []*profile.Location{testL[0]}, - Value: []int64{1, 1}, - }, - { - Location: []*profile.Location{testL[2], testL[1], testL[0]}, - Value: []int64{1, 10}, - }, - { - Location: []*profile.Location{testL[4], testL[2], testL[0]}, - Value: []int64{1, -100}, - }, - { - Location: []*profile.Location{testL[3], testL[0]}, - Value: []int64{1, 1000}, - }, - { - Location: []*profile.Location{testL[4], testL[3], testL[0]}, - Value: []int64{1, 10000}, - }, - }, - Location: testL, - Function: testF, - Mapping: testM, -} - func TestDisambiguation(t *testing.T) { parent1 := &graph.Node{Info: graph.NodeInfo{Name: "parent1"}} parent2 := &graph.Node{Info: graph.NodeInfo{Name: "parent2"}} @@ -356,7 +324,7 @@ func TestLegendActiveFilters(t *testing.T) { } func TestComputeTotal(t *testing.T) { - p1 := testProfile1.Copy() + p1 := testProfile.Copy() p1.Sample = []*profile.Sample{ { Location: []*profile.Location{testL[0]}, @@ -372,7 +340,7 @@ func TestComputeTotal(t *testing.T) { }, } - p2 := testProfile1.Copy() + p2 := testProfile.Copy() p2.Sample = []*profile.Sample{ { Location: []*profile.Location{testL[0]}, @@ -388,7 +356,7 @@ func TestComputeTotal(t *testing.T) { }, } - p3 := testProfile1.Copy() + p3 := testProfile.Copy() p3.Sample = []*profile.Sample{ { Location: []*profile.Location{testL[0]}, @@ -473,7 +441,7 @@ func TestComputeTotal(t *testing.T) { t.Errorf("got total %d, want %v", gotTotal, tc.wantTotal) } if gotUnexpNegSamples != tc.wantUnexpNegSamples { - t.Errorf("got unexpected negative samples %v, want %v", gotUnexpNegSamples, tc.wantUnexpNegSamples) + t.Errorf("got unexpected negative samples %v, want %v", gotUnexpNegSamples, tc.wantUnexpNegSamples) } }) } diff --git a/internal/report/testdata/source_neg_samples.dot b/internal/report/testdata/source_neg_samples.dot deleted file mode 100644 index ad37ef749..000000000 --- a/internal/report/testdata/source_neg_samples.dot +++ /dev/null @@ -1,17 +0,0 @@ -digraph "unnamed" { -node [style=filled fillcolor="#f8f8f8"] -subgraph cluster_L { "Duration: 10s, Total samples = 11111 " [shape=box fontsize=16 label="Duration: 10s, Total samples = 11111 \lShowing nodes accounting for 10911, 98.20% of 11111 total\l"] } -N1 [label="tee\nsource2:8\n10000 (90.00%)" id="node1" fontsize=24 shape=box tooltip="tee testdata/source2:8 (10000)" color="#b20500" fillcolor="#edd6d5"] -N2 [label="main\nsource1:2\n1 (0.009%)\nof 10911 (98.20%)" id="node2" fontsize=9 shape=box tooltip="main testdata/source1:2 (10911)" color="#b20000" fillcolor="#edd5d5"] -N3 [label="tee\nsource2:2\n1000 (9.00%)\nof 11000 (99.00%)" id="node3" fontsize=14 shape=box tooltip="tee testdata/source2:2 (11000)" color="#b20000" fillcolor="#edd5d5"] -N4 [label="tee\nsource2:8\n-100 (0.9%)" id="node4" fontsize=10 shape=box tooltip="tee testdata/source2:8 (-100)" color="#b0b2aa" fillcolor="#ecedec"] -N5 [label="bar\nsource1:10\n10 (0.09%)" id="node5" fontsize=9 shape=box tooltip="bar testdata/source1:10 (10)" color="#b2b2b1" fillcolor="#ededed"] -N6 [label="bar\nsource1:10\n0 of -100 (0.9%)" id="node6" fontsize=8 shape=box tooltip="bar testdata/source1:10 (-100)" color="#b0b2aa" fillcolor="#ecedec"] -N7 [label="foo\nsource1:4\n0 of 10 (0.09%)" id="node7" fontsize=8 shape=box tooltip="foo testdata/source1:4 (10)" color="#b2b2b1" fillcolor="#ededed"] -N2 -> N3 [label=" 11000" weight=100 penwidth=5 color="#b20000" tooltip="main testdata/source1:2 -> tee testdata/source2:2 (11000)" labeltooltip="main testdata/source1:2 -> tee testdata/source2:2 (11000)"] -N3 -> N1 [label=" 10000" weight=91 penwidth=5 color="#b20500" tooltip="tee testdata/source2:2 -> tee testdata/source2:8 (10000)" labeltooltip="tee testdata/source2:2 -> tee testdata/source2:8 (10000)"] -N6 -> N4 [label=" -100" color="#b0b2aa" tooltip="bar testdata/source1:10 -> tee testdata/source2:8 (-100)" labeltooltip="bar testdata/source1:10 -> tee testdata/source2:8 (-100)"] -N2 -> N6 [label=" -100" color="#b0b2aa" tooltip="main testdata/source1:2 -> bar testdata/source1:10 (-100)" labeltooltip="main testdata/source1:2 -> bar testdata/source1:10 (-100)"] -N7 -> N5 [label=" 10" color="#b2b2b1" tooltip="foo testdata/source1:4 -> bar testdata/source1:10 (10)" labeltooltip="foo testdata/source1:4 -> bar testdata/source1:10 (10)"] -N2 -> N7 [label=" 10" color="#b2b2b1" tooltip="main testdata/source1:2 -> foo testdata/source1:4 (10)" labeltooltip="main testdata/source1:2 -> foo testdata/source1:4 (10)"] -} diff --git a/internal/report/testdata/source_neg_samples.rpt b/internal/report/testdata/source_neg_samples.rpt new file mode 100644 index 000000000..71379f641 --- /dev/null +++ b/internal/report/testdata/source_neg_samples.rpt @@ -0,0 +1,34 @@ +Total: 11 +ROUTINE ======================== bar in testdata/source1 + -10 -10 (flat, cum) 90.91% of Total + . . 5:source1 line 5; + . . 6:source1 line 6; + . . 7:source1 line 7; + . . 8:source1 line 8; + . . 9:source1 line 9; + -10 -10 10:source1 line 10; + . . 11:source1 line 11; + . . 12:source1 line 12; + . . 13:source1 line 13; + . . 14:source1 line 14; + . . 15:source1 line 15; +ROUTINE ======================== foo in testdata/source1 + 0 -10 (flat, cum) 90.91% of Total + . . 1:source1 line 1; + . . 2:source1 line 2; + . . 3:source1 line 3; + . -10 4:source1 line 4; + . . 5:source1 line 5; + . . 6:source1 line 6; + . . 7:source1 line 7; + . . 8:source1 line 8; + . . 9:source1 line 9; +ROUTINE ======================== main in testdata/source1 + -1 -11 (flat, cum) 100% of Total + . . 1:source1 line 1; + -1 -11 2:source1 line 2; + . . 3:source1 line 3; + . . 4:source1 line 4; + . . 5:source1 line 5; + . . 6:source1 line 6; + . . 7:source1 line 7;