Skip to content

testutil.GatherAndCompare is error-prone #1351

Open
@machine424

Description

@machine424

After #1143, the function testutil.GatherAndCompare:

// GatherAndCompare gathers all metrics from the provided Gatherer and compares
// it to an expected output read from the provided Reader in the Prometheus text
// exposition format. If any metricNames are provided, only metrics with those
// names are compared.
func GatherAndCompare(g prometheus.Gatherer, expected io.Reader, metricNames ...string) error {
return TransactionalGatherAndCompare(prometheus.ToTransactionalGatherer(g), expected, metricNames...)
}

// TransactionalGatherAndCompare gathers all metrics from the provided Gatherer and compares
// it to an expected output read from the provided Reader in the Prometheus text
// exposition format. If any metricNames are provided, only metrics with those
// names are compared.
func TransactionalGatherAndCompare(g prometheus.TransactionalGatherer, expected io.Reader, metricNames ...string) error {
got, done, err := g.Gather()
defer done()
if err != nil {
return fmt.Errorf("gathering metrics failed: %w", err)
}
wanted, err := convertReaderToMetricFamily(expected)
if err != nil {
return err
}
return compareMetricFamilies(got, wanted, metricNames...)
}

// compareMetricFamilies would compare 2 slices of metric families, and optionally filters both of
// them to the `metricNames` provided.
func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...string) error {
if metricNames != nil {
got = filterMetrics(got, metricNames)
expected = filterMetrics(expected, metricNames)
}
return compare(got, expected)
}

became error-prone, as we filter the expected metrics in compareMetricFamilies as well now, if we make a mistake in one of the arguments expected or metricNames, the function will no longer return an error.

If e.g. we make a mistake in the name of the metric (provide a metric that is not exported because of a typo or re-using a variable containing the wrong metric name), the function will return nil.

Or e.g. if we provide an expected with the wrong metrics to a faulty code, we could never be aware of that.

I think expected lost its safeguard/source of truth utility.

I expect the user/tester to have full control on expected, I don't see why one couldn't provide the exact series they expect.

Plus having "dynamic" expected makes the tests hard to grasp and debug. No one needs tests with too many logic/intelligence that needs to be tested as well :) Way beneficial when they are more explicit even if this result in a little bit more code.

Looking at the code that was justifying the change:

https://github.com/fluxninja/aperture/blob/416d9ceaff9c1b2c4c238210c7c067dcf0a65567/pkg/otelcollector/metricsprocessor/processor_test.go#L148-L168

it's still not making use of the change yet, but I think they can easily split their expected and pass it as a map or sth to their test function...

Discovered this while adding a test for kubernetes etcd metrics: kubernetes/kubernetes#120827. I copied/pasted a test function and the test was passing because I forgot to change the metric name.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions