Make the AlreadyRegisteredError useful for wrapped registries #607
Conversation
|
This fixes #605 |
prometheus/registry_test.go
Outdated
| t.Error("expected original collector but got new one") | ||
| } | ||
| } else { | ||
| t.Error("unexpected error:", err) |
There was a problem hiding this comment.
Why are you using t.Error here. I think this is never expected so we could just use fatal?
There was a problem hiding this comment.
It's definitely fatal as in "the test is broken". But I only use t.Fatal in tests if the rest of the test wouldn't make sense to run anymore. However, if the rest of the test can still in principle work and tell me about other broken things, I use t.Error so that it continue to runs.
In this case, the Error is the last thing in the (sub-)test, so the difference doesn't really matter. It's more that I personally default to t.Error for the reasons above.
prometheus/registry_test.go
Outdated
| if are.ExistingCollector != original { | ||
| t.Error("expected original collector but got something else") | ||
| } | ||
| if are.ExistingCollector == equalButNotSame { |
There was a problem hiding this comment.
I think I would just have that included in the error above
t.Errorf("expected original collector but got %v", are.ExiststingCollector)(or even use Fatalf)
There was a problem hiding this comment.
The %v representation of a collector can look a bit daunting. As this is an == test, you'll see from the line where the test fails what precise collector was returned. My take is that this is slightly clearer than dumping a possibly complex text representation to the test output (which might have the right hint for the qualified reader, but will probably require to look at the test code anyway).
prometheus/registry_test.go
Outdated
| } else { | ||
| t.Error("unexpected error:", err) | ||
| } | ||
| if are, ok := err.(prometheus.AlreadyRegisteredError); ok { |
There was a problem hiding this comment.
This if/else block is almost the same for all test maybe add a function to check it, something like func checkAlreadyRegisteredError(t *testing.T, err error, original, equalButNotSame Collector) {...}
There was a problem hiding this comment.
The whole test could (or should) be transformed into a table based one, filling in the registries used for registering, re-registering, and the CounterVecs to register and reregister.
In general, I don't mind code duplication in tests if it makes things very explicit. But perhaps at this point, the table approach is even easier to grok.
I'll work on it...
There was a problem hiding this comment.
The test is now table-driven.
Signed-off-by: beorn7 <beorn@grafana.com>
@lukedirtwalker please have a look. This should fix your use case.