From 93ef9211564b736ed81e243d56cb66a1b7e46fe3 Mon Sep 17 00:00:00 2001
From: Tim Gross <tgross@hashicorp.com>
Date: Wed, 1 Mar 2023 11:42:38 -0500
Subject: [PATCH] fix tests that fail after catching `FSM.Apply` errors

`Job.Deregister` should not emit evals once the job has been purged and should
return an error if the job doesn't exist.
---
 api/jobs_test.go           |  2 +-
 api/nodes_test.go          |  1 +
 nomad/job_endpoint.go      |  7 +++--
 nomad/job_endpoint_test.go | 54 ++++++--------------------------------
 4 files changed, 15 insertions(+), 49 deletions(-)

diff --git a/api/jobs_test.go b/api/jobs_test.go
index dd3a3bb6fb6b..9cbe511e2e2b 100644
--- a/api/jobs_test.go
+++ b/api/jobs_test.go
@@ -1556,7 +1556,7 @@ func TestJobs_Deregister(t *testing.T) {
 
 	// Attempting delete on non-existing job returns an error
 	_, _, err = jobs.Deregister("nope", false, nil)
-	must.NoError(t, err)
+	must.Error(t, err)
 
 	// Do a soft deregister of an existing job
 	evalID, wm3, err := jobs.Deregister("job1", false, nil)
diff --git a/api/nodes_test.go b/api/nodes_test.go
index c8a658a69e08..2d1b4101eb5e 100644
--- a/api/nodes_test.go
+++ b/api/nodes_test.go
@@ -13,6 +13,7 @@ import (
 )
 
 func queryNodeList(t *testing.T, nodes *Nodes) ([]*NodeListStub, *QueryMeta) {
+	t.Helper()
 	var (
 		nodeListStub []*NodeListStub
 		queryMeta    *QueryMeta
diff --git a/nomad/job_endpoint.go b/nomad/job_endpoint.go
index 5768ff2ef7dd..4fc362983068 100644
--- a/nomad/job_endpoint.go
+++ b/nomad/job_endpoint.go
@@ -846,6 +846,9 @@ func (j *Job) Deregister(args *structs.JobDeregisterRequest, reply *structs.JobD
 	if err != nil {
 		return err
 	}
+	if job == nil {
+		return nil
+	}
 
 	var eval *structs.Evaluation
 
@@ -854,7 +857,7 @@ func (j *Job) Deregister(args *structs.JobDeregisterRequest, reply *structs.JobD
 	now := time.Now().UnixNano()
 
 	// If the job is periodic or parameterized, we don't create an eval.
-	if job == nil || !(job.IsPeriodic() || job.IsParameterized()) {
+	if !(job.IsPeriodic() || job.IsParameterized()) {
 
 		// The evaluation priority is determined by several factors. It
 		// defaults to the job default priority and is overridden by the
@@ -863,7 +866,7 @@ func (j *Job) Deregister(args *structs.JobDeregisterRequest, reply *structs.JobD
 		// If the user supplied an eval priority override, we subsequently
 		// use this.
 		priority := structs.JobDefaultPriority
-		if job != nil {
+		if job.Priority > 0 {
 			priority = job.Priority
 		}
 		if args.EvalPriority > 0 {
diff --git a/nomad/job_endpoint_test.go b/nomad/job_endpoint_test.go
index 272622e3c060..dde0bfca04ed 100644
--- a/nomad/job_endpoint_test.go
+++ b/nomad/job_endpoint_test.go
@@ -3587,13 +3587,11 @@ func TestJobEndpoint_Deregister_ACL(t *testing.T) {
 	require.NotZero(eval.CreateTime)
 	require.NotZero(eval.ModifyTime)
 
-	// Deregistration is not idempotent, produces a new eval after the job is
-	// deregistered. TODO(langmartin) make it idempotent.
+	// Deregistration is idempotent
 	var validResp2 structs.JobDeregisterResponse
 	err = msgpackrpc.CallWithCodec(codec, "Job.Deregister", req, &validResp2)
-	require.NoError(err)
-	require.NotEqual("", validResp2.EvalID)
-	require.NotEqual(validResp.EvalID, validResp2.EvalID)
+	must.NoError(t, err)
+	must.Eq(t, "", validResp2.EvalID)
 }
 
 func TestJobEndpoint_Deregister_Nonexistent(t *testing.T) {
@@ -3616,51 +3614,15 @@ func TestJobEndpoint_Deregister_Nonexistent(t *testing.T) {
 		},
 	}
 	var resp2 structs.JobDeregisterResponse
-	if err := msgpackrpc.CallWithCodec(codec, "Job.Deregister", dereg, &resp2); err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if resp2.JobModifyIndex == 0 {
-		t.Fatalf("bad index: %d", resp2.Index)
-	}
+	must.NoError(t, msgpackrpc.CallWithCodec(codec, "Job.Deregister", dereg, &resp2))
+	must.Eq(t, 0, resp2.JobModifyIndex, must.Sprint("expected no modify index"))
 
 	// Lookup the evaluation
 	state := s1.fsm.State()
 	ws := memdb.NewWatchSet()
-	eval, err := state.EvalByID(ws, resp2.EvalID)
-	if err != nil {
-		t.Fatalf("err: %v", err)
-	}
-	if eval == nil {
-		t.Fatalf("expected eval")
-	}
-	if eval.CreateIndex != resp2.EvalCreateIndex {
-		t.Fatalf("index mis-match")
-	}
-
-	if eval.Priority != structs.JobDefaultPriority {
-		t.Fatalf("bad: %#v", eval)
-	}
-	if eval.Type != structs.JobTypeService {
-		t.Fatalf("bad: %#v", eval)
-	}
-	if eval.TriggeredBy != structs.EvalTriggerJobDeregister {
-		t.Fatalf("bad: %#v", eval)
-	}
-	if eval.JobID != jobID {
-		t.Fatalf("bad: %#v", eval)
-	}
-	if eval.JobModifyIndex != resp2.JobModifyIndex {
-		t.Fatalf("bad: %#v", eval)
-	}
-	if eval.Status != structs.EvalStatusPending {
-		t.Fatalf("bad: %#v", eval)
-	}
-	if eval.CreateTime == 0 {
-		t.Fatalf("eval CreateTime is unset: %#v", eval)
-	}
-	if eval.ModifyTime == 0 {
-		t.Fatalf("eval ModifyTime is unset: %#v", eval)
-	}
+	eval, err := state.EvalsByJob(ws, structs.DefaultNamespace, jobID)
+	must.NoError(t, err)
+	must.Nil(t, eval)
 }
 
 func TestJobEndpoint_Deregister_EvalPriority(t *testing.T) {