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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

estats: remove dependency on testing package #7579

Merged
merged 2 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions experimental/stats/metricregistry.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ package stats

import (
"maps"
"testing"

"google.golang.org/grpc/grpclog"
"google.golang.org/grpc/internal"
Expand Down Expand Up @@ -250,9 +249,9 @@ func RegisterInt64Gauge(descriptor MetricDescriptor) *Int64GaugeHandle {
}

// snapshotMetricsRegistryForTesting snapshots the global data of the metrics
// registry. Registers a cleanup function on the provided testing.T that sets
// the metrics registry to its original state. Only called in testing functions.
func snapshotMetricsRegistryForTesting(t *testing.T) {
// registry. Returns a cleanup function that sets the metrics registry to its
// original state.
func snapshotMetricsRegistryForTesting() func() {
Copy link
Member

Choose a reason for hiding this comment

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

If this isn't exported anyway, then can't we simply move it into an _test.go file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't do that since internal.SnapshotMetricRegistryForTesting is set to this unexported function (in an init function) and the variable in internal package is used by other tests and therefore needs to be set.

oldDefaultMetrics := DefaultMetrics
oldRegisteredMetrics := registeredMetrics
oldMetricsRegistry := metricsRegistry
Expand All @@ -262,9 +261,9 @@ func snapshotMetricsRegistryForTesting(t *testing.T) {
maps.Copy(registeredMetrics, registeredMetrics)
maps.Copy(metricsRegistry, metricsRegistry)

t.Cleanup(func() {
return func() {
DefaultMetrics = oldDefaultMetrics
registeredMetrics = oldRegisteredMetrics
metricsRegistry = oldMetricsRegistry
})
}
}
12 changes: 9 additions & 3 deletions experimental/stats/metricregistry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ func Test(t *testing.T) {
// TestPanic tests that registering two metrics with the same name across any
// type of metric triggers a panic.
func (s) TestPanic(t *testing.T) {
snapshotMetricsRegistryForTesting(t)
cleanup := snapshotMetricsRegistryForTesting()
defer cleanup()

want := "metric simple counter already registered"
defer func() {
if r := recover(); !strings.Contains(fmt.Sprint(r), want) {
Expand All @@ -64,7 +66,9 @@ func (s) TestPanic(t *testing.T) {
// this tests the interactions between the metrics recorder and the metrics
// registry.
func (s) TestMetricRegistry(t *testing.T) {
snapshotMetricsRegistryForTesting(t)
cleanup := snapshotMetricsRegistryForTesting()
defer cleanup()

intCountHandle1 := RegisterInt64Count(MetricDescriptor{
Name: "simple counter",
Description: "sum of all emissions from tests",
Expand Down Expand Up @@ -141,7 +145,9 @@ func (s) TestMetricRegistry(t *testing.T) {
// metric registry. A component (simulated by test) should be able to record on
// the different registered int count metrics.
func TestNumerousIntCounts(t *testing.T) {
snapshotMetricsRegistryForTesting(t)
cleanup := snapshotMetricsRegistryForTesting()
defer cleanup()

intCountHandle1 := RegisterInt64Count(MetricDescriptor{
Name: "int counter",
Description: "sum of all emissions from tests",
Expand Down
7 changes: 3 additions & 4 deletions internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,10 +217,9 @@ var (
SetConnectedAddress any // func(scs *SubConnState, addr resolver.Address)

// SnapshotMetricRegistryForTesting snapshots the global data of the metric
// registry. Registers a cleanup function on the provided testing.T that
// sets the metric registry to its original state. Only called in testing
// functions.
SnapshotMetricRegistryForTesting any // func(t *testing.T)
// registry. Returns a cleanup function that sets the metric registry to its
// original state. Only called in testing functions.
SnapshotMetricRegistryForTesting func() func()

// SetDefaultBufferPoolForTesting updates the default buffer pool, for
// testing purposes.
Expand Down
8 changes: 6 additions & 2 deletions internal/stats/metrics_recorder_list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,9 @@ type recordingLoadBalancer struct {
// test then asserts that the recorded metrics show up on both configured stats
// handlers.
func (s) TestMetricsRecorderList(t *testing.T) {
internal.SnapshotMetricRegistryForTesting.(func(t *testing.T))(t)
cleanup := internal.SnapshotMetricRegistryForTesting()
defer cleanup()

mr := manual.NewBuilderWithScheme("test-metrics-recorder-list")
defer mr.Close()

Expand Down Expand Up @@ -212,7 +214,9 @@ func (s) TestMetricsRecorderList(t *testing.T) {
// TestMetricRecorderListPanic tests that the metrics recorder list panics if
// received the wrong number of labels for a particular metric.
func (s) TestMetricRecorderListPanic(t *testing.T) {
internal.SnapshotMetricRegistryForTesting.(func(t *testing.T))(t)
cleanup := internal.SnapshotMetricRegistryForTesting()
defer cleanup()

intCountHandle := estats.RegisterInt64Count(estats.MetricDescriptor{
Name: "simple counter",
Description: "sum of all emissions from tests",
Expand Down
4 changes: 3 additions & 1 deletion stats/opentelemetry/metricsregistry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,9 @@ func newServerStatsHandler(options MetricsOptions) metricsRecorderForTest {
// the expected metrics emissions, which includes default metrics and optional
// label assertions.
func (s) TestMetricsRegistryMetrics(t *testing.T) {
internal.SnapshotMetricRegistryForTesting.(func(t *testing.T))(t)
cleanup := internal.SnapshotMetricRegistryForTesting()
defer cleanup()

intCountHandle1 := estats.RegisterInt64Count(estats.MetricDescriptor{
Name: "int-counter-1",
Description: "Sum of calls from test",
Expand Down
Loading