client_ruby
client_ruby copied to clipboard
Refactor specs for metric implementations
So I thought I'd finally have a go at the refactor I described in the massive comment added to spec/examples/metric_example.rb
in 9f7603bef92dd5f382bb47c509454feef957dd00.
I spent the first chunk of my time on this PR exploring how to draw the lines on what gets tested where. Specifically, I wanted to come up with principles for testing common metric behaviour (especially when the behaviour is provided by the Prometheus::Client::Metric
base class). The principles I landed on were:
- If a behaviour is common to all metrics, and can be tested with code that doesn't depend on the specifics of a given metric class, put the tests in
spec/examples/metric_example.rb
. - If a behaviour is common to all metrics, and the implementation is in
Prometheus::Client::Metric
, but it can't be tested with code that doesn't depend on the specifics of a given metric class, put the tests inspec/prometheus/client/metric_spec.rb
. - If a metric class overrides behaviour provided by
Prometheus::Client::Metric
, then it should cover its implementation in its own tests.
Naturally any metric-specific behaviours (e.g. the fact that Prometheus::Client::Gauge
implements set
whereas Prometheus::Client::Counter
implements increment
) should be covered in their respective files.
Roughly speaking, these are the opinions I formed while doing this refactor which led me to those principles:
- Contract-style testing (via shared examples in this case) is nice, but immediately becomes way worse to use if you have to parameterise those tests. Going further down this route rather than what I did in
metric_spec.rb
would mean passing in arguments like "a lambda that knows how to increment this particular metric type when given a set of labels", and I think that very quickly leads to an incomprehensible mess. - Repeatedly testing behaviour provided by the
Prometheus::Client::Metric
base class is tedious, and you can see that in our various reactions to it over the years (either heavy copy-paste and complaining in a comment like 9f7603bef92dd5f382bb47c509454feef957dd00, or picking an arbitrary metric to test like #227). Explicitly having a place to test common behaviour feels like a decent answer to this problem, and a fake metric implementation that only exists in the tests seems like a nice way to do that. - If you're overriding something from
Prometheus::Client::Metric
it's on you to test that - even if what you're doing is basically the same (this is what led me to duplicate the file corruption test intohistogram_spec.rb
).
The rest of my time spent on this PR was implementing the many (fairly mechanical) changes you can see in the diff.
I think the most interesting thing to discuss is the principles, but I'm also happy to chat about specific parts of the diff (especially to help inform any opinions we come up with and any ways in which we want to tweak those principles).
I'm also open to the possibility that this is all a bad idea, and we should make some much smaller changes (or none at all). If we end up there, I'll close this at make a new PR deleting that massive comment I added in 9f7603bef92dd5f382bb47c509454feef957dd00. At least we'll have this PR as a reference for an avenue we explored.
If we do end up merging this in some form, I'll amend the commit message with more details of what's going on.