ronantakizawa/codereview-bench huggingface dataset:
Repository: hashicorp/nomad
File: command/agent/node_identity_endpoint_test.go (Go)

Code before:
			badMethods := []string{
				http.MethodConnect,
				http.MethodDelete,
				http.MethodGet,
				http.MethodHead,
				http.MethodOptions,
				http.MethodPatch,
				http.MethodTrace,
			}

			for _, method := range badMethods {
				req, err := http.NewRequest(method, "/v1/client/identity/renew", nil)
				must.NoError(t, err)

				_, err = s.Server.NodeIdentityRenewRequest(respW, req)
				must.ErrorContains(t, err, "Invalid method")

				codedErr, ok := err.(HTTPCodedError)
				must.True(t, ok)
				must.Eq(t, http.StatusMethodNotAllowed, codedErr.Code())
				must.Eq(t, ErrInvalidMethod, codedErr.Error())
			}
		})
	})

	t.Run("400 no node body", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			reqObj := structs.NodeIdentityRenewReq{
				NodeID: uuid.Generate(),
				QueryOptions: structs.QueryOptions{
					Region: s.config().Region,
				},
			}

			buf := encodeReq(reqObj)

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(http.MethodPost, "/v1/client/identity/renew", buf)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityRenewRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("400 no node query param", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			respW := httptest.NewRecorder()

Review comment (suggestion):
```suggestion
	t.Run("400 body with unknown node", func(t *testing.T) {
```

Code after:
			badMethods := []string{
				http.MethodConnect,
				http.MethodDelete,
				http.MethodGet,
				http.MethodHead,
				http.MethodOptions,
				http.MethodPatch,
				http.MethodTrace,
			}

			for _, method := range badMethods {
				req, err := http.NewRequest(method, "/v1/client/identity/renew", nil)
				must.NoError(t, err)

				_, err = s.Server.NodeIdentityRenewRequest(respW, req)
				must.ErrorContains(t, err, "Invalid method")

				codedErr, ok := err.(HTTPCodedError)
				must.True(t, ok)
				must.Eq(t, http.StatusMethodNotAllowed, codedErr.Code())
				must.Eq(t, ErrInvalidMethod, codedErr.Error())
			}
		})
	})

	t.Run("400 body with unknown node", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			reqObj := structs.NodeIdentityRenewReq{
				NodeID: uuid.Generate(),
				QueryOptions: structs.QueryOptions{
					Region: s.config().Region,
				},
			}

			buf := encodeReq(reqObj)

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(http.MethodPost, "/v1/client/identity/renew", buf)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityRenewRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("400 query param with unknown node", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			respW := httptest.NewRecorder()
Repository: hashicorp/nomad
File: command/agent/node_identity_endpoint_test.go (Go)

Code before:
		})
	})

	t.Run("400 no node body", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			reqObj := structs.NodeIdentityRenewReq{
				NodeID: uuid.Generate(),
				QueryOptions: structs.QueryOptions{
					Region: s.config().Region,
				},
			}

			buf := encodeReq(reqObj)

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(http.MethodPost, "/v1/client/identity/renew", buf)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityRenewRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("400 no node query param", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(
				http.MethodPost,
				"/v1/client/identity/renew?node_id="+uuid.Generate(),
				nil,
			)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityRenewRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("200 ok body", func(t *testing.T) {

		// Enable the client, so we have something to renew.
		configFn := func(c *Config) { c.Client.Enabled = true }

		httpTest(t, configFn, func(s *TestAgent) {

			testutil.WaitForClient(t, s.RPC, s.client.NodeID(), s.config().Region)


Review comment (suggestion):
```suggestion
	t.Run("400 query param with unknown node", func(t *testing.T) {
```

Code after:
		})
	})

	t.Run("400 body with unknown node", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			reqObj := structs.NodeIdentityRenewReq{
				NodeID: uuid.Generate(),
				QueryOptions: structs.QueryOptions{
					Region: s.config().Region,
				},
			}

			buf := encodeReq(reqObj)

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(http.MethodPost, "/v1/client/identity/renew", buf)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityRenewRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("400 query param with unknown node", func(t *testing.T) {
		httpTest(t, nil, func(s *TestAgent) {

			respW := httptest.NewRecorder()

			req, err := http.NewRequest(
				http.MethodPost,
				"/v1/client/identity/renew?node_id="+uuid.Generate(),
				nil,
			)
			must.NoError(t, err)

			_, err = s.Server.NodeIdentityRenewRequest(respW, req)
			must.ErrorContains(t, err, "Unknown node")
		})
	})

	t.Run("200 ok body", func(t *testing.T) {

		// Enable the client, so we have something to renew.
		configFn := func(c *Config) { c.Client.Enabled = true }

		httpTest(t, configFn, func(s *TestAgent) {

			testutil.WaitForClient(t, s.RPC, s.client.NodeID(), s.config().Region)
Repository: hashicorp/nomad
File: e2e/scheduler_system/systemsched_test.go (Go)

Code before:
			must.NoError(t, err)
		}
	})

	// Assert all jobs still running
	allocs = job.Allocs()
	must.SliceNotEmpty(t, allocs)

	allocForDisabledNode := make(map[string]*api.AllocationListStub)

	for _, alloc := range allocs {
		if alloc.NodeID == disabledNodeID {
			allocForDisabledNode[alloc.ID] = alloc
		}
	}

	// Filter down to only our latest running alloc
	for _, alloc := range allocForDisabledNode {
		must.Eq(t, uint64(0), alloc.JobVersion)
		if alloc.ClientStatus == structs.AllocClientStatusComplete {
			// remove the old complete alloc from map
			delete(allocForDisabledNode, alloc.ID)
		}
	}
	must.MapNotEmpty(t, allocForDisabledNode)
	must.MapLen(t, 1, allocForDisabledNode)

	// Update job
	job2, cleanup2 := jobs3.Submit(t,
		"./input/system_job1.nomad",
		jobs3.WaitComplete("group"),
		jobs3.DisableRandomJobID(),
	)
	t.Cleanup(cleanup2)

	// Get updated allocations
	allocs = job2.Allocs()
	must.SliceNotEmpty(t, allocs)

	var foundPreviousAlloc bool
	for _, dAlloc := range allocForDisabledNode {
		for _, alloc := range allocs {
			if alloc.ID == dAlloc.ID {
				foundPreviousAlloc = true
				must.Eq(t, uint64(0), alloc.JobVersion)
			} else if alloc.ClientStatus == structs.AllocClientStatusRunning {
				// Ensure allocs running on non disabled node are
				// newer version
				must.Eq(t, uint64(1), alloc.JobVersion)
			}
		}

Review comment (suggestion):
I'm not sure I understand what we're testing here. This is a system job so I'd expect each node has exactly one alloc for the job on it. We already know which alloc it is because we picked `alloc[0].NodeID` to get the ID of the disabled node. And I wouldn't expect any allocs to be complete because they're all still running.

Code after:
			must.NoError(t, err)
		}
	})

	// Assert all jobs still running
	allocs = job.Allocs()
	must.SliceNotEmpty(t, allocs)

	allocForDisabledNode := make(map[string]*api.AllocationListStub)
	for _, alloc := range allocs {
		if alloc.NodeID == disabledNodeID {
			allocForDisabledNode[alloc.ID] = alloc
		}
	}

	// Update job
	job2, cleanup2 := jobs3.Submit(t,
		"./input/system_job1.nomad",
		jobs3.DisableRandomJobID(),
	)
	t.Cleanup(cleanup2)

	// Get updated allocations
	allocs = job2.Allocs()
	must.SliceNotEmpty(t, allocs)

	var foundPreviousAlloc bool
	for _, dAlloc := range allocForDisabledNode {
		for _, alloc := range allocs {
			if alloc.ID == dAlloc.ID {
				foundPreviousAlloc = true
				must.Eq(t, uint64(0), alloc.JobVersion)
			} else if alloc.ClientStatus == structs.AllocClientStatusRunning {
				// Ensure allocs running on non disabled node are
				// newer version
				must.Eq(t, uint64(1), alloc.JobVersion)
			}
		}
	}
	must.True(t, foundPreviousAlloc, must.Sprint("unable to find previous alloc for ineligible node"))
}

func testCanaryUpdate(t *testing.T) {
	_, cleanup := jobs3.Submit(t,
		"./input/system_canary_v0.nomad.hcl",
		jobs3.DisableRandomJobID(),
	)
	t.Cleanup(cleanup)

	// Update job
	job2, cleanup2 := jobs3.Submit(t,
Repository: hashicorp/nomad
File: e2e/scheduler_system/systemsched_test.go (Go)

Code before:
}

func testCanaryUpdate(t *testing.T) {
	_, cleanup := jobs3.Submit(t,
		"./input/system_canary_v0.nomad.hcl",
		jobs3.WaitComplete("group"),
		jobs3.DisableRandomJobID(),
	)
	t.Cleanup(cleanup)

	// Update job
	job2, cleanup2 := jobs3.Submit(t,
		"./input/system_canary_v1.nomad.hcl",
		jobs3.DisableRandomJobID(),
		jobs3.Detach(),
	)
	t.Cleanup(cleanup2)

	// Get updated allocations
	allocs := job2.Allocs()
	must.SliceNotEmpty(t, allocs)

	deploymentsApi := job2.DeploymentsApi()

	// wait for the canary allocations to become healthy
	timeout := time.After(30 * time.Second)
CANARYWAIT:
	for {
		select {
		case <-timeout:
			must.Unreachable(t, must.Sprint("timeout reached waiting for healthy status of canary allocs"))
		default:
		}

		deployments, _, err := deploymentsApi.List(nil)
		must.NoError(t, err)
		for _, d := range deployments {
			for _, tg := range d.TaskGroups { // this job has 1 tg
				if d.JobVersion == 1 && tg.HealthyAllocs >= tg.DesiredCanaries {
					break CANARYWAIT
				}
			}
		}
	}

	// filter allocation from v1 version of the job, they should all be canaries
	// and there should be exactly 2
	canaryAllocs := []*api.AllocationListStub{}
	for _, a := range allocs {
		if a.JobVersion == 1 {
			must.True(t, a.DeploymentStatus.Canary)

Review comment (suggestion):
This goroutine will still be running once the test ends. I'd probably use a context with timeout here instead of `time.After`, because we can guarantee it gets cleaned up with a defer

Code after:
		jobs3.Detach(),
	)
	t.Cleanup(cleanup2)

	// how many eligible nodes do we have?
	nodesApi := job2.NodesApi()
	nodesList, _, err := nodesApi.List(nil)
	must.Nil(t, err)
	must.SliceNotEmpty(t, nodesList)

	numberOfEligibleNodes := 0
	for _, n := range nodesList {
		if n.SchedulingEligibility == api.NodeSchedulingEligible {
			numberOfEligibleNodes += 1
		}
	}

	// Get updated allocations
	allocs := job2.Allocs()
	must.SliceNotEmpty(t, allocs)

	deploymentsApi := job2.DeploymentsApi()
	deploymentsList, _, err := deploymentsApi.List(nil)
	must.NoError(t, err)

	var deployment *api.Deployment
	for _, d := range deploymentsList {
		if d.JobID == job2.JobID() && d.Status == api.DeploymentStatusRunning {
			deployment = d
		}
	}
	must.NotNil(t, deployment)

	// wait for the canary allocations to become healthy
	timeout, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()

	job2.WaitForDeploymentFunc(timeout, deployment.ID, func(d *api.Deployment) bool {
		for _, tg := range d.TaskGroups { // we only have 1 tg in this job
			if d.JobVersion == 1 && tg.HealthyAllocs >= tg.DesiredCanaries {
				return true
			}
		}
		return false
	})

	// find allocations from v1 version of the job, they should all be canaries
	// and there should be exactly 2
	count := 0
	for _, a := range allocs {
		if a.JobVersion == 1 {
Repository: hashicorp/nomad
File: nomad/structs/node_pool.go (Go)

Code before:
				_, _ = hash.Write([]byte("memory_oversubscription_disabled"))
			}
		}
	}

	// sort keys to ensure hash stability when meta is stored later
	var keys []string
	for k := range n.Meta {
		keys = append(keys, k)
	}
	sort.Strings(keys)

	for _, k := range keys {
		_, _ = hash.Write([]byte(k))
		_, _ = hash.Write([]byte(n.Meta[k]))
	}

	// Finalize the hash
	hashVal := hash.Sum(nil)

	// Set and return the hash
	n.Hash = hashVal
	return hashVal
}

// MarshalJSON implements the json.Marshaler interface and allows
// NodePool.NodeIdentityTTL to be marshaled correctly.
func (n *NodePool) MarshalJSON() ([]byte, error) {
	type Alias NodePool
	exported := &struct {
		NodeIdentityTTL string
		*Alias
	}{
		NodeIdentityTTL: n.NodeIdentityTTL.String(),
		Alias:           (*Alias)(n),
	}
	if n.NodeIdentityTTL == 0 {
		exported.NodeIdentityTTL = ""
	}
	return json.Marshal(exported)
}

// UnmarshalJSON implements the json.Unmarshaler interface and allows
// NodePool.NodeIdentityTTL to be unmarshalled correctly.
func (n *NodePool) UnmarshalJSON(data []byte) (err error) {
	type Alias NodePool
	aux := &struct {
		NodeIdentityTTL any
		*Alias
	}{
		Alias: (*Alias)(n),

Review comment (suggestion):
Something to keep in mind is that go-msgpack does our JSON encoding of these structs. I'm pretty sure this ends up working in dev but not with the generated code. There's a file `nomad/structs/extensions.go` that has examples of how to extend the encoding go-msgpack does.

Code after:
				_, _ = hash.Write([]byte("memory_oversubscription_disabled"))
			}
		}
	}

	// sort keys to ensure hash stability when meta is stored later
	var keys []string
	for k := range n.Meta {
		keys = append(keys, k)
	}
	sort.Strings(keys)

	for _, k := range keys {
		_, _ = hash.Write([]byte(k))
		_, _ = hash.Write([]byte(n.Meta[k]))
	}

	// Finalize the hash
	hashVal := hash.Sum(nil)

	// Set and return the hash
	n.Hash = hashVal
	return hashVal
}

// UnmarshalJSON implements the json.Unmarshaler interface and allows
// NodePool.NodeIdentityTTL to be unmarshalled correctly.
func (n *NodePool) UnmarshalJSON(data []byte) (err error) {
	type Alias NodePool
	aux := &struct {
		NodeIdentityTTL any
		*Alias
	}{
		Alias: (*Alias)(n),
	}

	if err = json.Unmarshal(data, &aux); err != nil {
		return err
	}
	if aux.NodeIdentityTTL != nil {
		switch v := aux.NodeIdentityTTL.(type) {
		case string:
			if v != "" {
				if n.NodeIdentityTTL, err = time.ParseDuration(v); err != nil {
					return err
				}
			}
		case float64:
			n.NodeIdentityTTL = time.Duration(v)
		}
Repository: hashicorp/nomad
File: nomad/job_endpoint_test.go (Go)

Code before:
}

func TestJobEndpoint_Register_PreserveResources(t *testing.T) {
	ci.Parallel(t)
	require := require.New(t)

	s1, cleanupS1 := TestServer(t, func(c *Config) {
		c.NumSchedulers = 0 // Prevent automatic dequeue
	})
	defer cleanupS1()
	codec := rpcClient(t, s1)
	testutil.WaitForLeader(t, s1.RPC)

	// Create the register request
	job := mock.Job()
	job.TaskGroups[0].Name = "group1"
	job.TaskGroups = append(job.TaskGroups, job.TaskGroups[0].Copy())
	job.TaskGroups[1].Name = "group2"
	job.TaskGroups[1].Tasks[0].Resources = &structs.Resources{
		CPU:      300,
		MemoryMB: 128,
	}
	job.Canonicalize()

	// Register the job
	require.NoError(msgpackrpc.CallWithCodec(codec, "Job.Register", &structs.JobRegisterRequest{
		Job: job,
		WriteRequest: structs.WriteRequest{
			Region:    "global",
			Namespace: job.Namespace,
		},
	}, &structs.JobRegisterResponse{}))

	// Check the job in the FSM state
	state := s1.fsm.State()
	out, err := state.JobByID(nil, job.Namespace, job.ID)
	require.NoError(err)
	require.NotNil(out)
	require.Equal(10, out.TaskGroups[0].Count)

	// New version:
	job = job.Copy()
	task := job.TaskGroups[0].Tasks[0]
	task.Resources.CPU = 200
	task.Resources.MemoryMB = 400

	job.TaskGroups[1].Tasks[0].Resources = &structs.Resources{
		CPU:      250,
		MemoryMB: 64,
	}


Review comment (suggestion):
Let's not add new users of `testify` and use `shoenig/test` exclusively for new tests.

Code after:
}

func TestJobEndpoint_Register_PreserveResources(t *testing.T) {
	ci.Parallel(t)

	s1, cleanupS1 := TestServer(t, func(c *Config) {
		c.NumSchedulers = 0 // Prevent automatic dequeue
	})
	defer cleanupS1()
	codec := rpcClient(t, s1)
	testutil.WaitForLeader(t, s1.RPC)

	// Create the register request
	job := mock.Job()
	job.TaskGroups[0].Name = "group1"
	job.TaskGroups = append(job.TaskGroups, job.TaskGroups[0].Copy())
	job.TaskGroups[1].Name = "group2"
	job.TaskGroups[1].Tasks[0].Resources = &structs.Resources{
		CPU:      300,
		MemoryMB: 128,
	}
	job.Canonicalize()

	// Register the job
	must.NoError(t, msgpackrpc.CallWithCodec(codec, "Job.Register", &structs.JobRegisterRequest{
		Job: job,
		WriteRequest: structs.WriteRequest{
			Region:    "global",
			Namespace: job.Namespace,
		},
	}, &structs.JobRegisterResponse{}))

	// Check the job in the FSM state
	state := s1.fsm.State()
	out, err := state.JobByID(nil, job.Namespace, job.ID)
	must.NoError(t, err)
	must.NotNil(t, out)
	must.Eq(t, 10, out.TaskGroups[0].Count)

	// New version:
	job = job.Copy()
	task := job.TaskGroups[0].Tasks[0]
	task.Resources.CPU = 200
	task.Resources.MemoryMB = 400

	job.TaskGroups[1].Tasks[0].Resources = &structs.Resources{
		CPU:      250,
		MemoryMB: 64,
	}

	// Perform the update
Repository: hashicorp/nomad
File: e2e/scheduler_system/systemsched_test.go (Go)

Code before:
	// wait for the canary allocations to become healthy
	timeout, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()

	job2.WaitForDeploymentFunc(timeout, deployment.ID, func(d *api.Deployment) bool {
		for _, tg := range d.TaskGroups { // we only have 1 tg in this job
			if d.JobVersion == 1 && tg.HealthyAllocs >= tg.DesiredCanaries {
				return true
			}
		}
		return false
	})

	// find allocations from v1 version of the job, they should all be canaries
	count := 0
	for _, a := range allocs {
		if a.JobVersion == 1 {
			must.True(t, a.DeploymentStatus.Canary)
			count += 1
		}
	}
	must.Eq(t, numberOfEligibleNodes, count, must.Sprint("expected canaries to be placed on all eligible nodes"))

	// deployment must not be terminal and needs to have the right status
	// description set
	must.Eq(t, deployment.StatusDescription, structs.DeploymentStatusDescriptionRunningNeedsPromotion)
}


Review comment (bug):
`shoenig/test` is pretty consistent about the expectation always going first, which makes for nicer test failure output:

```suggestion
	must.Eq(t, structs.DeploymentStatusDescriptionRunningNeedsPromotion, deployment.StatusDescription, )
```

Code after:
	}
	must.NotNil(t, deployment)

	// wait for the canary allocations to become healthy
	timeout, cancel := context.WithTimeout(context.Background(), 30*time.Second)
	defer cancel()

	job2.WaitForDeploymentFunc(timeout, deployment.ID, func(d *api.Deployment) bool {
		for _, tg := range d.TaskGroups { // we only have 1 tg in this job
			if d.JobVersion == 1 && tg.HealthyAllocs >= tg.DesiredCanaries {
				return true
			}
		}
		return false
	})

	// find allocations from v1 version of the job, they should all be canaries
	count := 0
	for _, a := range allocs {
		if a.JobVersion == 1 {
			must.True(t, a.DeploymentStatus.Canary)
			count += 1
		}
	}
	must.Eq(t, numberOfEligibleNodes, count, must.Sprint("expected canaries to be placed on all eligible nodes"))

	// deployment must not be terminal and needs to have the right status
	// description set
	must.Eq(t, structs.DeploymentStatusDescriptionRunningNeedsPromotion, deployment.StatusDescription)
}
Repository: hashicorp/nomad
File: drivers/docker/util/util_test.go (Go)

Code before:

import (
	"testing"
)

func TestCalculateCPUPercent(t *testing.T) {
	tests := []struct {
		name      string
		newSample uint64
		oldSample uint64
		newTotal  uint64
		oldTotal  uint64
		cores     int
		expected  float64
	}{
		{"valid case", 100, 50, 200, 100, 4, 200.0},
		{"zero denominator", 100, 50, 100, 100, 4, 0.0},
		{"negative case", 50, 100, 100, 200, 4, 0.0},
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			result := CalculateCPUPercent(tt.newSample, tt.oldSample, tt.newTotal, tt.oldTotal, tt.cores)
			if result != tt.expected {
				t.Errorf("expected %f, got %f", tt.expected, result)
			}
		})
	}
}


Review comment (suggestion):
Let use [`must.Eq`](https://github.com/shoenig/test/blob/a74f9d9f28e67b84e7d8230a0287e5ed04230172/must/must.go#L104) for this comparison.

Code after:

import (
	"testing"

	"github.com/shoenig/test/must"
)

func TestCalculateCPUPercent(t *testing.T) {
	tests := []struct {
		name      string
		newSample uint64
		oldSample uint64
		newTotal  uint64
		oldTotal  uint64
		cores     int
		expected  float64
	}{
		{
			name:      "valid case",
			newSample: 100,
			oldSample: 50,
			newTotal:  200,
			oldTotal:  100,
			cores:     4,
			expected:  200.0,
		},
		{
			name:      "zero denominator",
			newSample: 100,
			oldSample: 50,
			newTotal:  100,
			oldTotal:  100,
			cores:     4,
			expected:  0.0,
		},
		{
			name:      "zero numerator",
			newSample: 100,
			oldSample: 100,
			newTotal:  100,
			oldTotal:  50,
			cores:     4,
			expected:  0.0,
		},
		{
			name:      "negative case",
			newSample: 50,
			oldSample: 100,
			newTotal:  100,
			oldTotal:  200,
			cores:     4,
Repository: hashicorp/nomad
File: scheduler/scheduler_system.go (Go)

Code before:
	s.failedTGAllocs = nil

	// Create an evaluation context
	s.ctx = feasible.NewEvalContext(s.eventsCh, s.state, s.plan, s.logger)

	// Construct the placement stack
	s.stack = feasible.NewSystemStack(s.sysbatch, s.ctx)
	if !s.job.Stopped() {
		s.setJob(s.job)
	}

	// Compute the target job allocations
	if err := s.computeJobAllocs(); err != nil {
		s.logger.Error("failed to compute job allocations", "error", err)
		return false, err
	}

	// If the plan is a no-op, we can bail. If AnnotatePlan is set submit the plan
	// anyways to get the annotations.
	if s.plan.IsNoOp() && !s.eval.AnnotatePlan {
		return true, nil
	}

	// Submit the plan
	if s.eval.AnnotatePlan {
		s.plan.Annotations = s.planAnnotations
	}
	result, newState, err := s.planner.SubmitPlan(s.plan)
	s.planResult = result
	if err != nil {
		return false, err
	}

	// Decrement the number of allocations pending per task group based on the
	// number of allocations successfully placed
	adjustQueuedAllocations(s.logger, result, s.queuedAllocs)

	// If we got a state refresh, try again since we have stale data
	if newState != nil {
		s.logger.Debug("refresh forced")
		s.state = newState
		return false, nil
	}

	// Try again if the plan was not fully committed, potential conflict
	fullCommit, expected, actual := result.FullCommit(s.plan)
	if !fullCommit {
		s.logger.Debug("plan didn't fully commit", "attempted", expected, "placed", actual)
		return false, nil
	}


Review comment (bug):
AFAICT nextEval is now always `nil` ... can we just remove it? I removed it on main and everything compiled, but I hit a test failure.

Code after:

	// Create an evaluation context
	s.ctx = feasible.NewEvalContext(s.eventsCh, s.state, s.plan, s.logger)

	// Construct the placement stack
	s.stack = feasible.NewSystemStack(s.sysbatch, s.ctx)
	if !s.job.Stopped() {
		s.setJob(s.job)
	}

	// Compute the target job allocations
	if err := s.computeJobAllocs(); err != nil {
		s.logger.Error("failed to compute job allocations", "error", err)
		return false, err
	}

	// If the plan is a no-op, we can bail. If AnnotatePlan is set submit the plan
	// anyways to get the annotations.
	if s.plan.IsNoOp() && !s.eval.AnnotatePlan {
		return true, nil
	}

	// Submit the plan
	if s.eval.AnnotatePlan {
		s.plan.Annotations = s.planAnnotations
	}
	result, newState, err := s.planner.SubmitPlan(s.plan)
	s.planResult = result
	if err != nil {
		return false, err
	}

	// Decrement the number of allocations pending per task group based on the
	// number of allocations successfully placed
	adjustQueuedAllocations(s.logger, result, s.queuedAllocs)

	// If we got a state refresh, try again since we have stale data
	if newState != nil {
		s.logger.Debug("refresh forced")
		s.state = newState
		return false, nil
	}

	// Try again if the plan was not fully committed, potential conflict
	fullCommit, expected, actual := result.FullCommit(s.plan)
	if !fullCommit {
		s.logger.Debug("plan didn't fully commit", "attempted", expected, "placed", actual)
		return false, nil
	}

	// Success!