Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

smart diff to testutil.GatherAndCompare #998

Merged
merged 9 commits into from
Apr 13, 2022
Merged

smart diff to testutil.GatherAndCompare #998

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
94022f3
added smart diff to testutil.GatherAndCompare
sourikghosh Mar 7, 2022
066306b
v2 testuitls added for better diff
sourikghosh Mar 17, 2022
b33c848
replaced CollectAndCompare to CollectAndCompareV2 in testutil_test
sourikghosh Mar 17, 2022
cf6a226
renamed methods from v2 to withT
sourikghosh Mar 17, 2022
7729983
replaced testify with custom diff func
sourikghosh Mar 18, 2022
fc11393
Merge branch 'main' into better-diff-for-test
sourikghosh Mar 31, 2022
39fc465
difflib GetUnifiedDiffString added with test
sourikghosh Mar 31, 2022
b33c45f
license meta data added to file
sourikghosh Mar 31, 2022
b890818
moved difflib to internal
sourikghosh Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ require (
github.com/prometheus/client_model v0.2.0
github.com/prometheus/common v0.32.1
github.com/prometheus/procfs v0.7.3
github.com/stretchr/testify v1.4.0
golang.org/x/sys v0.0.0-20220114195835-da31bd327af9
google.golang.org/protobuf v1.26.0
)
Expand Down
70 changes: 69 additions & 1 deletion prometheus/testutil/testutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,10 @@ import (
"bytes"
"fmt"
"io"
"testing"

"github.com/prometheus/common/expfmt"
"github.com/stretchr/testify/require"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. is there a way to avoid this package? For a reason we wanted to make sure it's not used, so we are not leaking it.

Since this is not testing code, every project that imports client_golang needs to import testify.... I think we want to copy the diff function and maintain here.

See how we did that here https://github.com/efficientgo/tools/blob/main/core/pkg/testutil/testutil.go#L101

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree. I think I was a little hasty to review this, and I haven't noticed testify is not there already 🙈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess because it's already a transitive dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I agree too, this makes sense
but the above diff function has a dep on
https://github.com/efficientgo/tools/blob/fe763185946be83b20da626605319733bd7f97cb/core/pkg/testutil/testutil.go#L16 which is not maintained should I transfer the module implementation over here or just simply import it.


dto "github.com/prometheus/client_model/go"

Expand Down Expand Up @@ -215,13 +217,79 @@ func compare(got, want []*dto.MetricFamily) error {
if wantBuf.String() != gotBuf.String() {
return fmt.Errorf(`
metric output does not match expectation; want:
%s
got:
%s`, wantBuf.String(), gotBuf.String())

}
return nil
}

// CollectAndCompareWithT is similar to CollectAndCompare except it takes *testing.T object
// and shouldFail Flag to pass down to GatherAndCompareV2.
func CollectAndCompareWithT(t *testing.T, c prometheus.Collector, expected io.Reader, shouldFail bool, metricNames ...string) error {
reg := prometheus.NewPedanticRegistry()
if err := reg.Register(c); err != nil {
return fmt.Errorf("registering collector failed: %s", err)
}
return GatherAndCompareWithT(t, reg, expected, shouldFail, metricNames...)
}

// GatherAndCompareWithT is similiar to GatherAndCompare except it takes t *testing.T and shouldFail flag
// and calls TransactionalGatherAndCompareV2.
func GatherAndCompareWithT(t *testing.T, g prometheus.Gatherer, expected io.Reader, shouldFail bool, metricNames ...string) error {
return TransactionalGatherAndCompareWithT(t, prometheus.ToTransactionalGatherer(g), expected, shouldFail, metricNames...)
}

// TransactionalGatherAndCompareWithT is similiar to TransactionalGatherAndCompare except
// it takes t *testing.T and shouldFail flag and calls compareV2 for better diff.
func TransactionalGatherAndCompareWithT(t *testing.T, g prometheus.TransactionalGatherer, expected io.Reader, shouldFail bool, metricNames ...string) error {
got, done, err := g.Gather()
defer done()
if err != nil {
return fmt.Errorf("gathering metrics failed: %s", err)
}
if metricNames != nil {
got = filterMetrics(got, metricNames)
}
var tp expfmt.TextParser
wantRaw, err := tp.TextToMetricFamilies(expected)
if err != nil {
return fmt.Errorf("parsing expected metrics failed: %s", err)
}
want := internal.NormalizeMetricFamilies(wantRaw)

return compareWithT(t, got, want, shouldFail)
}

// compareWithT accepts *testing.T object and uses require package to provide
// a better diff between got and want.
func compareWithT(t *testing.T, got, want []*dto.MetricFamily, shouldFail bool) error {
var gotBuf, wantBuf bytes.Buffer
enc := expfmt.NewEncoder(&gotBuf, expfmt.FmtText)
for _, mf := range got {
if err := enc.Encode(mf); err != nil {
return fmt.Errorf("encoding gathered metrics failed: %s", err)
}
}
enc = expfmt.NewEncoder(&wantBuf, expfmt.FmtText)
for _, mf := range want {
if err := enc.Encode(mf); err != nil {
return fmt.Errorf("encoding expected metrics failed: %s", err)
}
}

if shouldFail && wantBuf.String() != gotBuf.String() {
return fmt.Errorf(`
metric output does not match expectation; want:

%s
got:

%s`, wantBuf.String(), gotBuf.String())

}

require.Equal(t, wantBuf.String(), gotBuf.String())
return nil
}

Expand Down
15 changes: 7 additions & 8 deletions prometheus/testutil/testutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"testing"

"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/require"
)

type untypedCollector struct{}
Expand Down Expand Up @@ -138,7 +139,7 @@ func TestCollectAndCompare(t *testing.T) {
some_total{ label1 = "value1" } 1
`

if err := CollectAndCompare(c, strings.NewReader(metadata+expected), "some_total"); err != nil {
if err := CollectAndCompareWithT(t, c, strings.NewReader(metadata+expected), false, "some_total"); err != nil {
t.Errorf("unexpected collecting result:\n%s", err)
}
}
Expand All @@ -160,7 +161,7 @@ func TestCollectAndCompareNoLabel(t *testing.T) {
some_total 1
`

if err := CollectAndCompare(c, strings.NewReader(metadata+expected), "some_total"); err != nil {
if err := CollectAndCompareWithT(t, c, strings.NewReader(metadata+expected), false, "some_total"); err != nil {
t.Errorf("unexpected collecting result:\n%s", err)
}
}
Expand Down Expand Up @@ -232,7 +233,7 @@ func TestCollectAndCompareHistogram(t *testing.T) {
}

t.Run(input.name, func(t *testing.T) {
if err := CollectAndCompare(input.c, strings.NewReader(input.metadata+input.expect)); err != nil {
if err := CollectAndCompareWithT(t, input.c, strings.NewReader(input.metadata+input.expect), false); err != nil {
t.Errorf("unexpected collecting result:\n%s", err)
}
})
Expand All @@ -259,7 +260,7 @@ func TestNoMetricFilter(t *testing.T) {
some_total{label1="value1"} 1
`

if err := CollectAndCompare(c, strings.NewReader(metadata+expected)); err != nil {
if err := CollectAndCompareWithT(t, c, strings.NewReader(metadata+expected), false); err != nil {
t.Errorf("unexpected collecting result:\n%s", err)
}
}
Expand Down Expand Up @@ -297,14 +298,12 @@ got:
some_total{label1="value1"} 1
`

err := CollectAndCompare(c, strings.NewReader(metadata+expected))
err := CollectAndCompareWithT(t, c, strings.NewReader(metadata+expected), true)
if err == nil {
t.Error("Expected error, got no error.")
}

if err.Error() != expectedError {
t.Errorf("Expected\n%#+v\nGot:\n%#+v", expectedError, err.Error())
}
require.EqualErrorf(t, err, expectedError, "Expected\n%#+v\nGot:\n%#+v", expectedError, err.Error())
}

func TestCollectAndCount(t *testing.T) {
Expand Down