-
Notifications
You must be signed in to change notification settings - Fork 79
Conversation
Codecov Report
@@ Coverage Diff @@
## master #224 +/- ##
==========================================
+ Coverage 73.02% 73.39% +0.37%
==========================================
Files 15 15
Lines 1609 1609
==========================================
+ Hits 1175 1181 +6
+ Misses 355 351 -4
+ Partials 79 77 -2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor nits, otherwise LGTM.
metrics_proto_api_test.go
Outdated
@@ -85,6 +86,74 @@ func TestVariousCasesFromFile(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestMetricsWithPrefix(t *testing.T) { | |||
server, addr, doneFn := createFakeServer(t) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: looks like these few lines appear multiple times. How about having a func createFakeServerAndConn
that returns both the server and connection? The returned doneFn
can incorporate conn.Close()
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
|
||
tc := readTestCaseFromFiles(t, "SingleMetric") | ||
|
||
prefixes := []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: can also test "external.googleapis.com/prometheus/".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
- also modify createFakeServer to return server and connectio
metrics_proto_api_test.go
Outdated
stop := func() { | ||
srv.Stop() | ||
_ = ln.Close() | ||
conn.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think conn
needs to be closed first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. fixed it.
added test for