ronantakizawa/codereview-bench huggingface dataset:
Repository: kubernetes/kubernetes
File: pkg/scheduler/util/assumecache/assume_cache.go (Go)

Code before:
	c.rwMutex.Lock()
	defer c.rwMutex.Unlock()

	c.eventHandlers = append(c.eventHandlers, handler)
	allObjs := c.listLocked(nil)
	for _, obj := range allObjs {
		c.eventQueue.WriteOne(func() {
			handler.OnAdd(obj, true)
		})
	}

	if c.handlerRegistration == nil {
		// No informer, so immediately synced.
		return syncedHandlerRegistration{}
	}

	return c.handlerRegistration
}

// emitEvents delivers all pending events that are in the queue, in the order
// in which they were stored there (FIFO). Only one goroutine at a time is
// delivering events, to ensure correct order.
func (c *AssumeCache) emitEvents() {
	c.rwMutex.Lock()
	for c.emittingEvents {
		// Wait for other caller of emitEvents to finish.
		// When it is done, it may or may not have drained
		// the events pushed by our caller.
		// We'll check below outselves.
		c.cond.Wait()
	}
	c.emittingEvents = true
	c.rwMutex.Unlock()

	defer func() {
		c.rwMutex.Lock()
		c.emittingEvents = false
		c.cond.Broadcast()
		c.rwMutex.Unlock()
	}()

	// When we get here, this instance of emitEvents is the active one.
	for {
		c.rwMutex.Lock()
		deliver, ok := c.eventQueue.ReadOne()
		c.rwMutex.Unlock()

		if !ok {
			return
		}
		func() {

Review comment (suggestion):
Is it possible there could be several?

```suggestion
		// Wait for other callers of emitEvents to finish.
```

Code after:
	for _, obj := range allObjs {
		c.eventQueue.WriteOne(func() {
			handler.OnAdd(obj, true)
		})
	}

	if c.handlerRegistration == nil {
		// No informer, so immediately synced.
		return syncedHandlerRegistration{}
	}

	return c.handlerRegistration
}

// emitEvents delivers all pending events that are in the queue, in the order
// in which they were stored there (FIFO). Only one goroutine at a time is
// delivering events, to ensure correct order.
func (c *AssumeCache) emitEvents() {
	c.rwMutex.Lock()
	for c.emittingEvents {
		// Wait for the active caller of emitEvents to finish.
		// When it is done, it may or may not have drained
		// the events pushed by our caller.
		// We'll check below ourselves.
		c.cond.Wait()
	}
	c.emittingEvents = true
	c.rwMutex.Unlock()

	defer func() {
		c.rwMutex.Lock()
		c.emittingEvents = false
		// Hand over the batton to one other goroutine, if there is one.
		// We don't need to wake up more than one because only one of
		// them would be able to grab the "emittingEvents" responsibility.
		c.cond.Signal()
		c.rwMutex.Unlock()
	}()

	// When we get here, this instance of emitEvents is the active one.
	for {
		c.rwMutex.Lock()
		deliver, ok := c.eventQueue.ReadOne()
		c.rwMutex.Unlock()

		if !ok {
			return
		}
		func() {
			defer utilruntime.HandleCrash()
			deliver()
Repository: kubernetes/kubernetes
File: pkg/kubelet/apis/podresources/server_v1_test.go (Go)

Code before:
				Containers: containers,
			},
		},
	}

	for _, tc := range []struct {
		desc             string
		dynamicResources []*podresourcesapi.DynamicResource
		expectedResponse *podresourcesapi.ListPodResourcesResponse
	}{
		{
			desc: "claim resource with all fields including ShareId",
			dynamicResources: []*podresourcesapi.DynamicResource{
				{
					ClaimName:      "gpu-claim",
					ClaimNamespace: "default",
					ClaimResources: []*podresourcesapi.ClaimResource{
						{
							CdiDevices: []*podresourcesapi.CDIDevice{
								{Name: "nvidia.com/gpu=0"},
								{Name: "nvidia.com/gpu=1"},
							},
							DriverName: "gpu.nvidia.com",
							PoolName:   "worker-1-gpu-pool",
							DeviceName: "gpu-device-0",
							ShareId:    ptr("shared-gpu-123"),
						},
					},
				},
			},
			expectedResponse: &podresourcesapi.ListPodResourcesResponse{
				PodResources: []*podresourcesapi.PodResources{
					{
						Name:      podName,
						Namespace: podNamespace,
						Containers: []*podresourcesapi.ContainerResources{
							{
								Name:    containerName,
								Devices: []*podresourcesapi.ContainerDevices{},
								CpuIds:  []int64{},
								Memory:  []*podresourcesapi.ContainerMemory{},
								DynamicResources: []*podresourcesapi.DynamicResource{
									{
										ClaimName:      "gpu-claim",
										ClaimNamespace: "default",
										ClaimResources: []*podresourcesapi.ClaimResource{
											{
												CdiDevices: []*podresourcesapi.CDIDevice{
													{Name: "nvidia.com/gpu=0"},
													{Name: "nvidia.com/gpu=1"},
												},

Review comment (suggestion):
you can use "k8s.io/utils/ptr" here instead of your own function:
```suggestion
							ShareId:    ptr.To("shared-gpu-123"),
```

Code after:
			Topology:   &podresourcesapi.TopologyInfo{Nodes: []*podresourcesapi.NUMANode{{ID: numaID}}},
		},
	}

	for _, tc := range []struct {
		desc       string
		pods       []*v1.Pod
		activePods []*v1.Pod
	}{
		{
			desc:       "no pods",
			pods:       []*v1.Pod{},
			activePods: []*v1.Pod{},
		},
		{
			desc: "no differences",
			pods: []*v1.Pod{
				makePod(1),
				makePod(2),
				makePod(3),
				makePod(4),
				makePod(5),
			},
			activePods: []*v1.Pod{
				makePod(1),
				makePod(2),
				makePod(3),
				makePod(4),
				makePod(5),
			},
		},
		{
			desc: "some terminated pods",
			pods: []*v1.Pod{
				makePod(1),
				makePod(2),
				makePod(3),
				makePod(4),
				makePod(5),
				makePod(6),
				makePod(7),
			},
			activePods: []*v1.Pod{
				makePod(1),
				makePod(3),
				makePod(4),
				makePod(5),
				makePod(6),
			},
		},
	} {
Repository: kubernetes/kubernetes
File: pkg/kubelet/apis/podresources/client_test.go (Go)

Code before:
	v1alpha1.UnimplementedPodResourcesListerServer
}

func (m *mockV1alpha1Server) List(context.Context, *v1alpha1.ListPodResourcesRequest) (*v1alpha1.ListPodResourcesResponse, error) {
	return &v1alpha1.ListPodResourcesResponse{}, nil
}

type mockV1Server struct {
	v1.UnimplementedPodResourcesListerServer
}

func (m *mockV1Server) List(context.Context, *v1.ListPodResourcesRequest) (*v1.ListPodResourcesResponse, error) {
	return &v1.ListPodResourcesResponse{}, nil
}

func startTestServer(t *testing.T, version string) (string, func(), error) {
	socketPath := filepath.Join(t.TempDir(), "podresources.sock")
	listener, err := net.Listen("unix", socketPath)
	if err != nil {
		return "", nil, err
	}

	server := grpc.NewServer()
	switch version {
	case "v1alpha1":
		v1alpha1.RegisterPodResourcesListerServer(server, &mockV1alpha1Server{})
	case "v1":
		v1.RegisterPodResourcesListerServer(server, &mockV1Server{})
	}

	go func() {
		if err := server.Serve(listener); err != nil {
			t.Logf("server stopped: %v", err)
		}
	}()

	return socketPath, server.Stop, nil
}


Review comment (suggestion):
Just wondering if this is enough for the server to start. Anyway, you're not testing its APIs.
```suggestion
		v1alpha1.RegisterPodResourcesListerServer(server, &v1.UnimplementedPodResourcesListerServer{})
```

Code after:
/*
Copyright The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

    http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package podresources

import (
	"testing"
	"time"

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"
	fakeremote "k8s.io/cri-client/pkg/fake"
)

func TestGetClient(t *testing.T) {
	testCases := map[string]func(string, time.Duration, int) (any, *grpc.ClientConn, error){
		"v1alpha1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1alpha1Client(socket, timeout, maxSize)
		},
		"v1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1Client(socket, timeout, maxSize)
		},
	}

	for version, getClientFn := range testCases {
		t.Run(version, func(t *testing.T) {
			socketPath, err := fakeremote.GenerateEndpoint()
			require.NoError(t, err)

			client, conn, err := getClientFn(socketPath, 10*time.Second, 1024*1024)
			require.NoError(t, err)
			require.NotNil(t, client)
			require.NoError(t, conn.Close())

			client, conn, err = getClientFn("invalid\x00endpoint", 100*time.Millisecond, 1024*1024)
			require.Error(t, err)
			assert.Nil(t, client)
			assert.Nil(t, conn)
		})
	}
}
Repository: kubernetes/kubernetes
File: pkg/kubelet/apis/podresources/client_test.go (Go)

Code before:
	"time"

	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"

	v1 "k8s.io/kubelet/pkg/apis/podresources/v1"
	"k8s.io/kubelet/pkg/apis/podresources/v1alpha1"
)

func TestGetClient(t *testing.T) {
	versions := []string{"v1alpha1", "v1"}

	for _, version := range versions {
		t.Run(version, func(t *testing.T) {
			socketPath, cleanup, err := startTestServer(t, version)
			require.NoError(t, err)
			defer cleanup()

			var client interface{}
			var conn *grpc.ClientConn
			switch version {
			case "v1alpha1":
				client, conn, err = GetV1alpha1Client(socketPath, 10*time.Second, 1024*1024)
			case "v1":
				client, conn, err = GetV1Client(socketPath, 10*time.Second, 1024*1024)
			}
			require.NoError(t, err)
			defer func() { _ = conn.Close() }()

			require.NotNil(t, client)
		})
	}
}

func TestGetClientError(t *testing.T) {
	versions := []string{"v1alpha1", "v1"}

	for _, version := range versions {
		t.Run(version, func(t *testing.T) {
			var client interface{}
			var conn *grpc.ClientConn
			var err error
			switch version {
			case "v1alpha1":
				client, conn, err = GetV1alpha1Client("unix:///invalid\x00path", 100*time.Millisecond, 1024*1024)
			case "v1":
				client, conn, err = GetV1Client("unix:///invalid\x00path", 100*time.Millisecond, 1024*1024)
			}
			require.Error(t, err)
			require.Nil(t, client)
			require.Nil(t, conn)

Review comment (refactor):
just pass a function pointer to the testcase? or use a helper struct and iterate over them (example: TestPolicyOptionsAvailable in cpumanager/policy_options_test.go)

Code after:
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"
	fakeremote "k8s.io/cri-client/pkg/fake"
)

func TestGetClient(t *testing.T) {
	testCases := map[string]func(string, time.Duration, int) (any, *grpc.ClientConn, error){
		"v1alpha1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1alpha1Client(socket, timeout, maxSize)
		},
		"v1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1Client(socket, timeout, maxSize)
		},
	}

	for version, getClientFn := range testCases {
		t.Run(version, func(t *testing.T) {
			socketPath, err := fakeremote.GenerateEndpoint()
			require.NoError(t, err)

			client, conn, err := getClientFn(socketPath, 10*time.Second, 1024*1024)
			require.NoError(t, err)
			require.NotNil(t, client)
			require.NoError(t, conn.Close())

			client, conn, err = getClientFn("invalid\x00endpoint", 100*time.Millisecond, 1024*1024)
			require.Error(t, err)
			assert.Nil(t, client)
			assert.Nil(t, conn)
		})
	}
}
Repository: kubernetes/kubernetes
File: pkg/kubelet/apis/podresources/client_test.go (Go)

Code before:
	"testing"
	"time"

	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"

	v1 "k8s.io/kubelet/pkg/apis/podresources/v1"
	"k8s.io/kubelet/pkg/apis/podresources/v1alpha1"
)

// clientTestCase defines a test case for client creation with different API versions.
// These unit tests focus on boundary conditions that are difficult to test in e2e:
// - Client connection logic with invalid paths
// - Timeout handling with short durations
// - Error scenarios that require specific network conditions
type clientTestCase struct {
	version     string
	getClientFn func(string, time.Duration, int) (interface{}, *grpc.ClientConn, error)
	registerFn  func(*grpc.Server)
}

func getClientTestCases() []clientTestCase {
	return []clientTestCase{
		{
			version: "v1alpha1",
			getClientFn: func(socket string, timeout time.Duration, maxSize int) (interface{}, *grpc.ClientConn, error) {
				return GetV1alpha1Client(socket, timeout, maxSize)
			},
			registerFn: func(server *grpc.Server) {
				v1alpha1.RegisterPodResourcesListerServer(server, &v1alpha1.UnimplementedPodResourcesListerServer{})
			},
		},
		{
			version: "v1",
			getClientFn: func(socket string, timeout time.Duration, maxSize int) (interface{}, *grpc.ClientConn, error) {
				return GetV1Client(socket, timeout, maxSize)
			},
			registerFn: func(server *grpc.Server) {
				v1.RegisterPodResourcesListerServer(server, &v1.UnimplementedPodResourcesListerServer{})
			},
		},
	}
}

func TestGetClient(t *testing.T) {
	testCases := getClientTestCases()

	for _, tc := range testCases {
		t.Run(tc.version, func(t *testing.T) {
			socketPath, cleanup, err := startTestServer(t, tc.registerFn)
			require.NoError(t, err)

Review comment (suggestion):
nit:
```suggestion
			getClientFn: func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
```

Code after:

	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"
	fakeremote "k8s.io/cri-client/pkg/fake"
)

func TestGetClient(t *testing.T) {
	testCases := map[string]func(string, time.Duration, int) (any, *grpc.ClientConn, error){
		"v1alpha1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1alpha1Client(socket, timeout, maxSize)
		},
		"v1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1Client(socket, timeout, maxSize)
		},
	}

	for version, getClientFn := range testCases {
		t.Run(version, func(t *testing.T) {
			socketPath, err := fakeremote.GenerateEndpoint()
			require.NoError(t, err)

			client, conn, err := getClientFn(socketPath, 10*time.Second, 1024*1024)
			require.NoError(t, err)
			require.NotNil(t, client)
			require.NoError(t, conn.Close())

			client, conn, err = getClientFn("invalid\x00endpoint", 100*time.Millisecond, 1024*1024)
			require.Error(t, err)
			assert.Nil(t, client)
			assert.Nil(t, conn)
		})
	}
}
Repository: kubernetes/kubernetes
File: pkg/kubelet/apis/podresources/client_test.go (Go)

Code before:
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"
)

type clientTestCase struct {
	getClientFn func(string, time.Duration, int) (any, *grpc.ClientConn, error)
}

func getClientTestCases() map[string]clientTestCase {
	return map[string]clientTestCase{
		"v1alpha1": {
			getClientFn: func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
				return GetV1alpha1Client(socket, timeout, maxSize)
			},
		},
		"v1": {
			getClientFn: func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
				return GetV1Client(socket, timeout, maxSize)
			},
		},
	}
}

func TestGetClient(t *testing.T) {
	testCases := getClientTestCases()

	for version, tc := range testCases {
		t.Run(version, func(t *testing.T) {
			socketPath := filepath.Join(t.TempDir(), "podresources.sock")

			client, conn, err := tc.getClientFn(socketPath, 10*time.Second, 1024*1024)
			require.NoError(t, err)
			require.NotNil(t, client)
			require.NoError(t, conn.Close())

			client, conn, err = tc.getClientFn("unix:///invalid\x00path", 100*time.Millisecond, 1024*1024)
			require.Error(t, err)
			assert.Nil(t, client)
			assert.Nil(t, conn)
		})
	}
}



Review comment (suggestion):
It would be if test cases are here, in the test.

BTW, if test case is just a function, you probably don't need to wrap it into structure, e.g. `testCases := map[string]func(string, time.Duration, int) (any, *grpc.ClientConn, error)` or something similar.

Code after:
	"github.com/stretchr/testify/require"
	"google.golang.org/grpc"
	fakeremote "k8s.io/cri-client/pkg/fake"
)

func TestGetClient(t *testing.T) {
	testCases := map[string]func(string, time.Duration, int) (any, *grpc.ClientConn, error){
		"v1alpha1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1alpha1Client(socket, timeout, maxSize)
		},
		"v1": func(socket string, timeout time.Duration, maxSize int) (any, *grpc.ClientConn, error) {
			return GetV1Client(socket, timeout, maxSize)
		},
	}

	for version, getClientFn := range testCases {
		t.Run(version, func(t *testing.T) {
			socketPath, err := fakeremote.GenerateEndpoint()
			require.NoError(t, err)

			client, conn, err := getClientFn(socketPath, 10*time.Second, 1024*1024)
			require.NoError(t, err)
			require.NotNil(t, client)
			require.NoError(t, conn.Close())

			client, conn, err = getClientFn("invalid\x00endpoint", 100*time.Millisecond, 1024*1024)
			require.Error(t, err)
			assert.Nil(t, client)
			assert.Nil(t, conn)
		})
	}
}
Repository: kubernetes/kubernetes
File: third_party/forked/golang/expansion/expand.go (Go)

Code before:
// tryReadVariableName attempts to read a variable name from the input
// string and returns the content read from the input, whether that content
// represents a variable name to perform mapping on, and the number of bytes
// consumed in the input string.
//
// The input string is assumed not to contain the initial operator.
func tryReadVariableName(input string) (string, bool, int) {
	switch input[0] {
	case operator:
		// Escaped operator; return it.
		return input[0:1], false, 1
	case referenceOpener:
		// Scan to expression closer
		for i := 1; i < len(input); i++ {
			if input[i] == referenceCloser {
				return input[1:i], true, i + 1
			}
		}

		// Incomplete reference; return it.
		return string(operator) + string(referenceOpener), false, 1
	default:
		// Not the beginning of an expression, ie, an operator
		// that doesn't begin an expression.  Return the operator
		// and the first rune in the string.
		r, size := utf8.DecodeRuneInString(input)
		return string(operator) + string(r), false, size
	}
}


Review comment (suggestion):
If we are going to do this "right" we should probably decode the rune before the switch (which also uses `input[0]`.  Switch on the rune, not the 0th byte.  Yes, it is FUNCTIONALLY safe (I think) but it's confuing and ambiguous.

Code after:
// tryReadVariableName attempts to read a variable name from the input
// string and returns the content read from the input, whether that content
// represents a variable name to perform mapping on, and the number of bytes
// consumed in the input string.
//
// The input string is assumed not to contain the initial operator.
func tryReadVariableName(input string) (string, bool, int) {
	r, size := utf8.DecodeRuneInString(input)
	switch r {
	case operator:
		// Escaped operator; return it.
		return input[0:size], false, size
	case referenceOpener:
		// Scan to expression closer
		for i := 1; i < len(input); i++ {
			if input[i] == referenceCloser {
				return input[1:i], true, i + 1
			}
		}

		// Incomplete reference; return it.
		return string(operator) + string(referenceOpener), false, 1
	default:
		// Not the beginning of an expression, ie, an operator
		// that doesn't begin an expression.  Return the operator
		// and the first rune in the string.

		return string(operator) + string(r), false, size
	}
}
Repository: kubernetes/kubernetes
File: test/images/node-perf/pytorch-wide-deep/train_wide_deep.py (Python)

Code before:
    print("-" * 50)

    start_time = time.time()

    for epoch in range(n_epochs):
        model.train()

        # Forward pass
        optimizer.zero_grad()
        outputs = model(train_num_t, train_cat_t)
        loss = criterion(outputs, train_labels_t)

        # Backward pass
        loss.backward()
        optimizer.step()

        if (epoch + 1) % 50 == 0:
            print(f"Epoch {epoch+1:3d}/{n_epochs}, Loss: {loss.item():.4f}")

    elapsed = time.time() - start_time

    # Evaluation
    model.eval()
    with torch.no_grad():
        outputs = model(test_num_t, test_cat_t)
        _, predicted = torch.max(outputs, 1)
        accuracy = (predicted == test_labels_t).float().mean().item()

    print("-" * 50)
    print(f"Training completed in {elapsed:.2f} seconds")
    print(f"Test accuracy: {accuracy:.4f}")


if __name__ == "__main__":
    train()


Review comment (suggestion):
You can probably do this directly:
```suggestion
        predicted = outputs.argmax(dim=1)
```

Code after:
    # We use full-batch training with more epochs for comparable total compute.
    n_epochs = 300
    batch_size = len(train_labels)  # Full batch for simpler CPU benchmarking

    print(f"Training for {n_epochs} epochs, batch_size={batch_size}")
    print("-" * 50)

    start_time = time.time()

    for epoch in range(n_epochs):
        model.train()

        # Forward pass
        optimizer.zero_grad()
        outputs = model(train_num_t, train_cat_t)
        loss = criterion(outputs, train_labels_t)

        # Backward pass
        loss.backward()
        optimizer.step()

        if (epoch + 1) % 50 == 0:
            print(f"Epoch {epoch+1:3d}/{n_epochs}, Loss: {loss.item():.4f}")

    elapsed = time.time() - start_time

    # Evaluation
    model.eval()
    with torch.no_grad():
        outputs = model(test_num_t, test_cat_t)
        predicted = outputs.argmax(dim=1)
        accuracy = (predicted == test_labels_t).float().mean().item()

    print("-" * 50)
    print(f"Training completed in {elapsed:.2f} seconds")
    print(f"Test accuracy: {accuracy:.4f}")


if __name__ == "__main__":
    train()