[WIP] Add testutils package #423

brancz · 2018-07-03T11:34:48Z

This is mostly a copy and paste from the testutils package that we have originally extracted from the node-exporter test code (I think). I opened #422 about this and @beorn7 suggested me to open a PR and we start discussing. I haven't added documentation yet in case we want to change any of this. If we are happy with this I am more than happy to add more documentation.

Note that this primarily revolves around testing (custom) collectors, it is less useful for standard Counter/Gauge/Histogram/Summary metrics, unless the tested entity is a wrapping object around those (I suppose it could also be useful for single metrics but I see it being most useful for collectors).

Also related: #58

Signed-off-by: Frederic Branczyk fbranczyk@gmail.com

@beorn7 @brian-brazil @grobie

Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>

beorn7 · 2018-07-06T13:23:56Z

I'll look at this ASAP. @grobie also wants to compare this with his various test implementations he did.
We both are somewhat busy, though, but we'll keep this on our respective plates.

grobie · 2018-07-06T13:34:43Z

"wants" ;-) But I'll try to give it a look over the weekend.

beorn7 · 2018-07-06T21:59:43Z

FTR: Travis test was flaky, I restarted it, now it is green.

beorn7 · 2018-07-06T22:01:49Z

testutils/testutils.go

+// GatherAndCompare retrieves all metrics exposed by a collector and compares it
+// to an expected output in the Prometheus text exposition format.
+// metricNames allows only comparing the given metrics. All are compared if it's nil.
+func GatherAndCompare(c prometheus.Collector, expected string, metricNames ...string) error {


Yes, that's exactly what I had in mind for this package.

beorn7 · 2018-07-06T22:03:59Z

testutils/testutils.go

+
+// The below sorting code is copied form the Prometheus client library modulo the added
+// label pair sorting.
+// https://github.com/prometheus/client_golang/blob/ea6e1db4cb8127eeb0b6954f7320363e5451820f/prometheus/registry.go#L642-L684


Which raises the question if we should move this code to an internal package…

beorn7 · 2018-07-06T22:06:21Z

testutils/testutils.go

+
+	"github.com/prometheus/client_golang/prometheus"
+	dto "github.com/prometheus/client_model/go"
+	"github.com/prometheus/common/expfmt"


Nit: Traditional Prometheus grouping here would be:

import ( "bytes" "fmt" "reflect" "sort" "strings" "github.com/prometheus/common/expfmt" dto "github.com/prometheus/client_model/go" "github.com/prometheus/client_golang/prometheus" )

beorn7 · 2018-07-06T22:21:23Z

As in the comments (besides the nits), this is what I had in mind as one of the helpers in this upcoming package. So yes, full agreement we should have this here.

Suggested course of action:

Let's wait for @grobie to chime in whether this is what he wants, too.
Improve code-level details (started with a few nits from my side already, see comments).
Merge this.
I will then add the single-value retrieval helper suggested in Improve testability #58 in a follow-up PR.

(Reading the following is optional: Very fundamentally, I think this single-value retrieval is not the right (tm) approach to testing, but we can/should still provide it here for now. The "gather all the metric and compare it to a golden set of metrics" is a nice end-to-end test. For unit-testing, the only thing I want to test is if, for example, an Inc() call has happened on a Counter. However, retrieving the counter value before and after the Inc call to verify the increment by one involves the whole counter machinery (which I don't want to test) and doesn't play well with concurrency. It would be better to use test mocks of metric types that directly report if the call has happened and do nothing else. However, with the current state of client_golang, the interfaces are less clear than they could be, and not everything is an interface that one could implement with test mocks. Thus, this part will follow later, cf. #230, and for now I'll provide that single-value retrieval helper, but at least it is nicely locked away in this package, and it can be deprecated without breaking anything in the main packages.)

beorn7

Some more minor remarks after a more detailed look. Otherwise, this looks very good to me. Let's give @grobie time to respond over the weekend.

I don't think you need to invest extra effort into documentation at this point. As said, I'll add that single-value extraction helper to this package in a separate PR, and then I can write a package doc comment to explain how to do the testing.

beorn7 · 2018-07-07T15:42:24Z

testutils/testutils.go

+	expected = removeUnusedWhitespace(expected)
+
+	reg := prometheus.NewPedanticRegistry()
+	if err := reg.Register(c); err != nil {


How about leaving these two lines to the caller? I.e. make GatherAndCompare accept a Gatherer rather than a Collector. This would also let the name of the function appear more consistent.

With this approach, a caller could act on the registry level, which allows testing even if metrics are coming from a library that does not allow access to individual collectors, but a registry with all the collectors already registered is readily available. Also, the caller can decide if a pedantic registry should be used or not.

If a caller wants to test a single Collector, it's easy to wrap GatherAndCompare with these two lines. Or wo could even provide a CollectAndCompare.

As a concrete example, right now you couldn't test many Collectors because the "cheat" by providing a dummy Desc, which the pedantic registry would flag. (#47 will allow to implement those Collectors cleanly, and I'll work in this issue this evening (coincidentally).)

👍 to using a gatherer.

beorn7 · 2018-07-07T15:45:30Z

testutils/testutils.go

+	}
+
+	// The Prometheus metrics representation parser expects an empty line at the
+	// end otherwise fails with an unexpected EOF error.


That's misleading. It expects the last line to end on a \n, but that line does not need to be empty.

grobie

This is a great start! I'd wish for a more generic interface. Shorthands can be provided with additional functions if desired.

grobie · 2018-07-09T10:33:57Z

testutils/testutils.go

+// GatherAndCompare retrieves all metrics exposed by a collector and compares it
+// to an expected output in the Prometheus text exposition format.
+// metricNames allows only comparing the given metrics. All are compared if it's nil.
+func GatherAndCompare(c prometheus.Collector, expected string, metricNames ...string) error {


The interface and implementation couple two independent concerns: verifying the given collector and formatting a custom input format. Instead of this coupling and assuming all users will format their expectation in this special way, using the io.Reader interface would leave it to the user how to provide the expectation. For example, this allows projects with extensive end to end tests to simply provide a File reader to such fixtures.

The current custom sanitizer implementation in removeUnusedWhitespace can be provided as another public function which will satisfy the reader interface.

It certainly makes sense to use an io.Reader instead of a string as input.

I don't quite follow the line of argument with the removeUnusedWhitespace, though. In fact, now that I have looked more closely, I got the suspicion the removeUnusedWhitespace is not even needed. The provided text for "expected" is parsed with the text format parses. (Thus, the parser should eat all the spurious whitespace.) For the diff output, we then re-create the text format from the protobuf representation and get a sanitized output anyway.

Even better if we don't need it.

I played with the tests, and it really looks to me we can just remove removeUnusedWhitespace. :o)

grobie · 2018-07-09T10:36:18Z

testutils/testutils.go

+	expected = removeUnusedWhitespace(expected)
+
+	reg := prometheus.NewPedanticRegistry()
+	if err := reg.Register(c); err != nil {


👍 to using a gatherer.

beorn7 · 2018-07-09T11:49:44Z

Another point: Since these testutils are only for testing the exposition client, I'd make it a sub-package of client_golang/prometheus rather than client_golang directly.

beorn7 · 2018-07-13T11:13:22Z

@brancz will you be able to address the comments given? I think the changes will be quite straight forward, and then we can merge this. If you are busy right now, I can take over from here.

brancz · 2018-07-13T13:47:42Z

Yeah I have a big deadline that is keeping me busy at least for today and next week, feel free to take over! 🙂

beorn7 · 2018-07-19T09:35:50Z

I was a bit optimistic. Preparations for my grand cycle tour took precedence, and now I'm on said cycle tour, with not much time for anything else. @brancz if you find time, please go ahead to work on this. Should I find time before that, I'll let you know here.

brancz · 2018-07-19T11:46:46Z

I'm still busy as well, but might be able to get to it next week.

beorn7 · 2018-08-22T22:27:18Z

Superseded by #445 .

Add testutils package

5d3b089

Signed-off-by: Frederic Branczyk <fbranczyk@gmail.com>

beorn7 reviewed Jul 6, 2018

View reviewed changes

beorn7 requested changes Jul 7, 2018

View reviewed changes

grobie reviewed Jul 9, 2018

View reviewed changes

beorn7 mentioned this pull request Aug 22, 2018

Add testutil package #445

Merged

beorn7 closed this Aug 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add testutils package #423

[WIP] Add testutils package #423

brancz commented Jul 3, 2018

beorn7 commented Jul 6, 2018

grobie commented Jul 6, 2018

beorn7 commented Jul 6, 2018

beorn7 Jul 6, 2018

beorn7 Jul 6, 2018

beorn7 Jul 6, 2018

beorn7 commented Jul 6, 2018

beorn7 left a comment

beorn7 Jul 7, 2018

beorn7 Jul 7, 2018

grobie Jul 9, 2018 •

edited

Loading

beorn7 Jul 7, 2018

grobie left a comment

grobie Jul 9, 2018

beorn7 Jul 9, 2018

grobie Jul 9, 2018

beorn7 Jul 9, 2018

grobie Jul 9, 2018 •

edited

Loading

beorn7 commented Jul 9, 2018

beorn7 commented Jul 13, 2018

brancz commented Jul 13, 2018

beorn7 commented Jul 19, 2018

brancz commented Jul 19, 2018

beorn7 commented Aug 22, 2018

[WIP] Add testutils package #423

[WIP] Add testutils package #423

Conversation

brancz commented Jul 3, 2018

beorn7 commented Jul 6, 2018

grobie commented Jul 6, 2018

beorn7 commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Jul 6, 2018

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grobie Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

beorn7 commented Jul 9, 2018

beorn7 commented Jul 13, 2018

brancz commented Jul 13, 2018

beorn7 commented Jul 19, 2018

brancz commented Jul 19, 2018

beorn7 commented Aug 22, 2018

grobie Jul 9, 2018 •

edited

Loading

grobie Jul 9, 2018 •

edited

Loading