search

endclothing / prometheus_client_php

Prometheus instrumentation library for PHP applications
https://prometheus.io/docs/concepts/metric_types/
Apache License 2.0
150 stars 77 forks source link

Test suite produces warnings #26

Open martinssipenko opened 4 years ago

martinssipenko commented 4 years ago

I noticed that after merging #25 there are some tests that produce warnings. By looking at the whole test suite closed I noticed that abstract classes are being used in tests which I think makes the test suite hard to understand. It also causes some tests cases not being executed, more particularly the ones that use @dataProvider.

I'd like to propose to refactor the test suite so that abstract classes are not used anymore. This however means that we have duplicate code (copy all test cases into each adapters test class) or use a test method per adapter type and move real assertions to protected methods.

@NoelDavies whats you view on this? I'm happy to submit a example PR if that would help.

NoelDavies commented 4 years ago

That'd be brilliant. I didn't notice them. Good spot

martinssipenko commented 4 years ago

@NoelDavies check out #27 would you be ok with such approach?

martinssipenko commented 4 years ago

I'm wondering whats the idea behind BlackBoxTest, and why does it increment metrics 1100 times?

martinssipenko commented 4 years ago

@NoelDavies I closed the initial PR and opened a new one - #28, it turned out the issue was in PHPUnit bug that got fixed in latest release.

NoelDavies commented 4 years ago

I added a review. Once those are looked at we'll push up :D