From cc9e1f8a00726e256bde3853f6c788c84555788d Mon Sep 17 00:00:00 2001
From: Danny Canter <36526702+dcantah@users.noreply.github.com>
Date: Wed, 23 Mar 2022 18:34:15 -0700
Subject: [PATCH] Fix up job object options for unit tests (#1335)

Most of the `jobobject` package tests we have ask for options that aren't
actually needed/used. This change makes it so that any test that doesn't need
a named job object doesn't ask for one and any test that doesn't plan on
using the iocp messages doesn't flip the notifications field either. This
wasn't causing any issues, but it's probably best to filter down what's
being tested to only what's needed.

Additionally fixes TestExecsWithJob that used log.Fatal instead of t.Fatal
in the test.

Signed-off-by: Daniel Canter <dcanter@microsoft.com>
---
 exec/exec_test.go           |  5 ++---
 jobobject/jobobject_test.go | 32 ++++++++++----------------------
 2 files changed, 12 insertions(+), 25 deletions(-)

diff --git a/exec/exec_test.go b/exec/exec_test.go
index 7afdf34ad6..3ad62a791e 100644
--- a/exec/exec_test.go
+++ b/exec/exec_test.go
@@ -3,7 +3,6 @@ package exec
 import (
 	"context"
 	"io"
-	"log"
 	"os"
 	"path/filepath"
 	"testing"
@@ -127,9 +126,9 @@ func TestExecStdinPowershell(t *testing.T) {
 
 func TestExecsWithJob(t *testing.T) {
 	// Test that we can assign processes to a job object at creation time.
-	job, err := jobobject.Create(context.Background(), &jobobject.Options{Name: "test"})
+	job, err := jobobject.Create(context.Background(), nil)
 	if err != nil {
-		log.Fatal(err)
+		t.Fatal(err)
 	}
 	defer job.Close()
 
diff --git a/jobobject/jobobject_test.go b/jobobject/jobobject_test.go
index c1baa033c7..68257c790b 100644
--- a/jobobject/jobobject_test.go
+++ b/jobobject/jobobject_test.go
@@ -23,7 +23,6 @@ func TestJobCreateAndOpen(t *testing.T) {
 		ctx     = context.Background()
 		options = &Options{Name: "test"}
 	)
-
 	jobCreate, err := Create(ctx, options)
 	if err != nil {
 		t.Fatal(err)
@@ -67,11 +66,7 @@ func createProcsAndAssign(num int, job *JobObject) (_ []*exec.Cmd, err error) {
 }
 
 func TestSetTerminateOnLastHandleClose(t *testing.T) {
-	options := &Options{
-		Name:          "test",
-		Notifications: true,
-	}
-	job, err := Create(context.Background(), options)
+	job, err := Create(context.Background(), nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -99,7 +94,7 @@ func TestSetTerminateOnLastHandleClose(t *testing.T) {
 		if !procs[0].ProcessState.Exited() {
 			errCh <- errors.New("process should have exited after closing job handle")
 		}
-		errCh <- nil
+		close(errCh)
 	}()
 
 	select {
@@ -116,11 +111,7 @@ func TestSetTerminateOnLastHandleClose(t *testing.T) {
 func TestSetMultipleExtendedLimits(t *testing.T) {
 	// Tests setting two different properties on the job that modify
 	// JOBOBJECT_EXTENDED_LIMIT_INFORMATION
-	options := &Options{
-		Name:          "test",
-		Notifications: true,
-	}
-	job, err := Create(context.Background(), options)
+	job, err := Create(context.Background(), nil)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -158,7 +149,6 @@ func TestNoMoreProcessesMessageKill(t *testing.T) {
 	// Test that we receive the no more processes in job message after killing all of
 	// the processes in the job.
 	options := &Options{
-		Name:          "test",
 		Notifications: true,
 	}
 	job, err := Create(context.Background(), options)
@@ -192,7 +182,8 @@ func TestNoMoreProcessesMessageKill(t *testing.T) {
 
 			switch notif.(type) {
 			case MsgAllProcessesExited:
-				errCh <- nil
+				close(errCh)
+				return
 			case MsgUnimplemented:
 			default:
 			}
@@ -213,7 +204,6 @@ func TestNoMoreProcessesMessageTerminate(t *testing.T) {
 	// Test that we receive the no more processes in job message after terminating the
 	// job (terminates every process in the job).
 	options := &Options{
-		Name:          "test",
 		Notifications: true,
 	}
 	job, err := Create(context.Background(), options)
@@ -245,7 +235,8 @@ func TestNoMoreProcessesMessageTerminate(t *testing.T) {
 
 			switch notif.(type) {
 			case MsgAllProcessesExited:
-				errCh <- nil
+				close(errCh)
+				return
 			case MsgUnimplemented:
 			default:
 			}
@@ -263,12 +254,9 @@ func TestNoMoreProcessesMessageTerminate(t *testing.T) {
 }
 
 func TestVerifyPidCount(t *testing.T) {
-	// This test verifies that job.Pids() returns the right info and works with > 1 process.
-	options := &Options{
-		Name:          "test",
-		Notifications: true,
-	}
-	job, err := Create(context.Background(), options)
+	// This test verifies that job.Pids() returns the right info and works with > 1
+	// process.
+	job, err := Create(context.Background(), nil)
 	if err != nil {
 		t.Fatal(err)
 	}