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.

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

Already on GitHub? Sign in to your account

Added testutil.CollectAndCount #703

Merged
merged 1 commit into from Jan 9, 2020
Merged

Added testutil.CollectAndCount #703

merged 1 commit into from Jan 9, 2020

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Jan 9, 2020

馃憢

I would like to propose additional function to testutil.go which will count metrics in a given Collector. The example usage can be seen here: https://github.com/thanos-io/thanos/blob/d758432436718ff6cd0d9136bc53653f4ec5cf47/pkg/compact/compact_e2e_test.go#L179

I also read the package documentation and I think I can agree that mock would be preferred for all of those test assertions IF such mock would be ready to use somewhere AND it would be easy to inject those mocked counter, gauges etc. The latter might be even impossible given the *Vec metrics are not interfaces e.g:

func NewCounterVec(opts CounterOpts, labelNames []string) *CounterVec {

Overall I think, with this small function testutil gives all you need to unit test your instrumentation without mocks, except maybe registering part, which is easier to mock.

Signed-off-by: Bartlomiej Plotka bwplotka@gmail.com

@bwplotka bwplotka requested a review from beorn7 January 9, 2020 10:20
Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

I guess we can do this.

Note that vectors are planned to get "interfaced" in v2 to allow the mocking you'd need here.

Just some notes about naming and doc comments.

prometheus/testutil/testutil.go Outdated Show resolved Hide resolved
prometheus/testutil/testutil.go Outdated Show resolved Hide resolved
prometheus/testutil/testutil.go Outdated Show resolved Hide resolved
@bwplotka
Copy link
Member Author

bwplotka commented Jan 9, 2020

Thanks for speedy review @beorn7 - all applied.

Side note: With interfaces, mock might be doable indeed. Do you think we should host such a prepared mock as well in v2 then? I think it would be useful.

@bwplotka bwplotka requested a review from beorn7 January 9, 2020 11:18
@bwplotka bwplotka changed the title Added testutil.MetricCount Added testutil.ColllectAndCount Jan 9, 2020
@bwplotka bwplotka changed the title Added testutil.ColllectAndCount Added testutil.CollectAndCount Jan 9, 2020
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
@beorn7
Copy link
Member

beorn7 commented Jan 9, 2020

Do you think we should host such a prepared mock as well in v2 then? I think it would be useful.

That's the plan, at least vaguely.

@beorn7 beorn7 merged commit 803ef2a into master Jan 9, 2020
@bwplotka bwplotka deleted the testutil-metric-count branch January 9, 2020 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants