[cleanup] staticcheck cleanup (#3397)

Running staticcheck highlighted a few places that might be issues/bugs
in tests and this PR cleans them up.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **Refactor**
- Enhanced error handling and assertions in test functions across
various modules.
- Simplified variable assignments and removed redundant code for
cleaner, more efficient testing.
- Removed unnecessary fields and imports to streamline structures and
test suites.

- **Tests**
- Improved test coverage by adding new assertions and modifying existing
test cases for better reliability and error detection.

- **Chores**
- Code cleanup including the removal of unused imports and fields,
contributing to a more maintainable and readable codebase.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

cmd/cli/cancel/cancel_test.go

@@ -40,7 +40,7 @@ func (suite *CancelSuite) TestCancelTerminalJob() {
 	job := testutils.GetJobFromTestOutputLegacy(ctx, suite.T(), suite.Client, stdout)
 	suite.T().Logf("Created job %s", job.Metadata.ID)
 
-	_, stdout, err = suite.ExecuteTestCobraCommand("cancel", job.Metadata.ID)
+	_, _, err = suite.ExecuteTestCobraCommand("cancel", job.Metadata.ID)
 	require.ErrorContains(suite.T(), err, "already in a terminal state")
 }
 

cmd/cli/get/get_test.go

@@ -205,6 +205,7 @@ func (s *GetSuite) TestGetSingleFileFromOutputBadChoice() {
 		fmt.Sprintf("%s/missing", jobID),
 	)
 
+	require.Error(s.T(), err, "expected error but it wasn't returned")
 	require.Contains(s.T(), getoutput, "error downloading job")
 }
 

pkg/authn/challenge/authenticator_test.go

@@ -67,6 +67,7 @@ func TestBadlySignedChallenge(t *testing.T) {
 	userPubKey := base64.StdEncoding.EncodeToString(x509.MarshalPKCS1PublicKey(&userPrivKey.PublicKey))
 
 	signature, err := system.Sign([]byte("other input phrase"), userPrivKey)
+	require.NoError(t, err)
 	response := response{
 		PhraseSignature: signature,
 		PublicKey:       userPubKey,
@@ -88,8 +89,11 @@ func TestGoodChallenge(t *testing.T) {
 	require.NoError(t, err)
 
 	userPubKey := base64.StdEncoding.EncodeToString(x509.MarshalPKCS1PublicKey(&userPrivKey.PublicKey))
+	require.NoError(t, err)
 
 	signature, err := system.Sign([]byte(req.InputPhrase), userPrivKey)
+	require.NoError(t, err)
+
 	response := response{
 		PhraseSignature: signature,
 		PublicKey:       userPubKey,

pkg/downloader/download_test.go

@@ -40,7 +40,6 @@ type DownloaderSuite struct {
 	ipfsClient       ipfs.Client
 	downloadSettings *downloader.DownloaderSettings
 	downloadProvider downloader.DownloaderProvider
-	s3downloader     *s3signed.Downloader
 	s3Signer         *s3helper.ResultSigner
 }
 

pkg/job/script_checker_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/stretchr/testify/suite"
 
 	"github.com/bacalhau-project/bacalhau/pkg/logger"
-	_ "github.com/bacalhau-project/bacalhau/pkg/logger"
 )
 
 type SystemScriptCheckerSuite struct {

pkg/orchestrator/evaluation/inmemory_broker_test.go

@@ -236,7 +236,7 @@ func (s *InMemoryBrokerTestSuite) TestNack_Delay() {
 		return true, nil
 	}}))
 
-	delay := time.Now().Sub(start)
+	delay := time.Since(start)
 	s.Require().GreaterOrEqual(delay, s.broker.subsequentNackDelay, "should have waited for the nack delay")
 
 	// Dequeue should work again

pkg/orchestrator/planner/compute_forwarder.go

@@ -6,15 +6,13 @@ import (
 	"github.com/bacalhau-project/bacalhau/pkg/compute"
 	"github.com/bacalhau-project/bacalhau/pkg/jobstore"
 	"github.com/bacalhau-project/bacalhau/pkg/models"
-	"github.com/bacalhau-project/bacalhau/pkg/orchestrator"
 	"github.com/rs/zerolog/log"
 )
 
 type ComputeForwarder struct {
 	id             string
 	computeService compute.Endpoint
 	jobStore       jobstore.Store
-	evalBroker     orchestrator.EvaluationBroker //nolint:unused
 }
 
 type ComputeForwarderParams struct {

pkg/orchestrator/planner/utils_test.go

@@ -174,7 +174,6 @@ func (m *UpdateJobMatcher) String() string {
 type ComputeRequestMatcher struct {
 	t         *testing.T
 	nodeID    string
-	plan      *models.Plan
 	execution *models.Execution
 	update    *models.PlanExecutionDesiredUpdate
 }
@@ -200,9 +199,8 @@ func (m *ComputeRequestMatcher) Matches(x interface{}) bool {
 	var routingMetadata compute.RoutingMetadata
 	var executionID string
 
-	switch x.(type) {
+	switch req := x.(type) {
 	case compute.AskForBidRequest:
-		req := x.(compute.AskForBidRequest)
 		routingMetadata = req.RoutingMetadata
 		executionID = req.Execution.ID
 		desiredState := m.execution.DesiredState.StateType
@@ -219,15 +217,12 @@ func (m *ComputeRequestMatcher) Matches(x interface{}) bool {
 			}
 		}
 	case compute.BidAcceptedRequest:
-		req := x.(compute.BidAcceptedRequest)
 		routingMetadata = req.RoutingMetadata
 		executionID = req.ExecutionID
 	case compute.BidRejectedRequest:
-		req := x.(compute.BidRejectedRequest)
 		routingMetadata = req.RoutingMetadata
 		executionID = req.ExecutionID
 	case compute.CancelExecutionRequest:
-		req := x.(compute.CancelExecutionRequest)
 		routingMetadata = req.RoutingMetadata
 		executionID = req.ExecutionID
 	default:

pkg/orchestrator/scheduler/batch_job_test.go

@@ -62,8 +62,8 @@ func TestBatchSchedulerTestSuite(t *testing.T) {
 
 func (s *BatchJobSchedulerTestSuite) TestProcess_ShouldCreateEnoughExecutions() {
 	ctx := context.Background()
-	job, executions, evaluation := mockJob()
-	executions = []models.Execution{} // no executions yet
+	job, _, evaluation := mockJob()
+	executions := []models.Execution{} // no executions yet
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 
@@ -165,8 +165,8 @@ func (s *BatchJobSchedulerTestSuite) TestProcess_TooManyExecutions() {
 
 func (s *BatchJobSchedulerTestSuite) TestProcessFail_NotEnoughExecutions() {
 	ctx := context.Background()
-	job, executions, evaluation := mockJob()
-	executions = []models.Execution{} // no executions yet
+	job, _, evaluation := mockJob()
+	executions := []models.Execution{} // no executions yet
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 

pkg/orchestrator/scheduler/daemon_job_test.go

@@ -42,8 +42,8 @@ func TestDaemonJobSchedulerTestSuite(t *testing.T) {
 
 func (s *DaemonJobSchedulerTestSuite) TestProcess_ShouldCreateNewExecutions() {
 	ctx := context.Background()
-	job, executions, evaluation := mockDaemonJob()
-	executions = []models.Execution{}
+	job, _, evaluation := mockDaemonJob()
+	executions := []models.Execution{}
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 
@@ -161,8 +161,8 @@ func (s *DaemonJobSchedulerTestSuite) TestProcess_WhenJobIsStopped_ShouldMarkNon
 
 func (s *DaemonJobSchedulerTestSuite) TestProcessFail_NoMatchingNodes() {
 	ctx := context.Background()
-	job, executions, evaluation := mockDaemonJob()
-	executions = []models.Execution{} // no executions yet
+	job, _, evaluation := mockDaemonJob()
+	executions := []models.Execution{} // no executions yet
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 	s.nodeSelector.EXPECT().AllMatchingNodes(gomock.Any(), job).Return([]models.NodeInfo{}, nil)

pkg/orchestrator/scheduler/ops_job_test.go

@@ -42,8 +42,8 @@ func TestOpsJobSchedulerTestSuite(t *testing.T) {
 
 func (s *OpsJobSchedulerTestSuite) TestProcess_ShouldCreateNewExecutions() {
 	ctx := context.Background()
-	job, executions, evaluation := mockOpsJob()
-	executions = []models.Execution{}
+	job, _, evaluation := mockOpsJob()
+	executions := []models.Execution{}
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 
@@ -152,8 +152,8 @@ func (s *OpsJobSchedulerTestSuite) TestProcess_WhenJobIsStopped_ShouldMarkNonTer
 
 func (s *OpsJobSchedulerTestSuite) TestProcessFail_NoMatchingNodes() {
 	ctx := context.Background()
-	job, executions, evaluation := mockOpsJob()
-	executions = []models.Execution{} // no executions yet
+	job, _, evaluation := mockOpsJob()
+	executions := []models.Execution{} // no executions yet
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 	s.mockNodeSelection(job, []models.NodeInfo{})

pkg/orchestrator/scheduler/service_job_test.go

@@ -54,8 +54,8 @@ func TestServiceSchedulerTestSuite(t *testing.T) {
 
 func (s *ServiceJobSchedulerTestSuite) TestProcess_ShouldCreateEnoughExecutions() {
 	ctx := context.Background()
-	job, executions, evaluation := mockServiceJob()
-	executions = []models.Execution{} // no executions yet
+	job, _, evaluation := mockServiceJob()
+	executions := []models.Execution{} // no executions yet
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 
@@ -160,8 +160,8 @@ func (s *ServiceJobSchedulerTestSuite) TestProcess_TooManyExecutions() {
 
 func (s *ServiceJobSchedulerTestSuite) TestProcessFail_NotEnoughExecutions() {
 	ctx := context.Background()
-	job, executions, evaluation := mockServiceJob()
-	executions = []models.Execution{} // no executions yet
+	job, _, evaluation := mockServiceJob()
+	executions := []models.Execution{} // no executions yet
 	s.jobStore.EXPECT().GetJob(gomock.Any(), job.ID).Return(*job, nil)
 	s.jobStore.EXPECT().GetExecutions(gomock.Any(), jobstore.GetExecutionsOptions{JobID: job.ID}).Return(executions, nil)
 

pkg/orchestrator/worker_test.go

@@ -30,7 +30,6 @@ type WorkerTestSuite struct {
 	worker               *Worker
 	eval                 *models.Evaluation
 	receiptHandle        string
-	cancelFn             context.CancelFunc
 }
 
 func (s *WorkerTestSuite) SetupTest() {

pkg/publicapi/server_test.go

@@ -70,7 +70,7 @@ func (s *APIServerTestSuite) TestListenAndServe() {
 	assert.NoError(s.T(), err)
 
 	// Make a request to ensure the server is shutdown
-	resp, err = http.Get(s.server.GetURI().String())
+	_, err = http.Get(s.server.GetURI().String())
 	assert.Error(s.T(), err)
 }
 

pkg/storage/local_directory/storage_test.go

@@ -10,7 +10,6 @@ import (
 	"testing"
 
 	"github.com/bacalhau-project/bacalhau/pkg/logger"
-	_ "github.com/bacalhau-project/bacalhau/pkg/logger"
 	"github.com/bacalhau-project/bacalhau/pkg/models"
 	"github.com/bacalhau-project/bacalhau/pkg/storage"
 	"github.com/stretchr/testify/require"

pkg/test/compute/ask_for_bid_test.go

@@ -19,18 +19,17 @@ type AskForBidSuite struct {
 }
 
 type bidResponseTestCase struct {
-	name          string
-	execution     *models.Execution
-	rejected      bool
-	resourceUsage models.Resources
+	name      string
+	execution *models.Execution
+	rejected  bool
 }
 
 func TestAskForBidSuite(t *testing.T) {
 	suite.Run(t, new(AskForBidSuite))
 }
 
 func (s *AskForBidSuite) TestAskForBid() {
-	s.runAskForBidTest(bidResponseTestCase{})
+	s.runAskForBidTest(bidResponseTestCase{name: "empty test case"})
 }
 
 func (s *AskForBidSuite) verify(response compute.BidResult, expected models.Resources) {
@@ -40,34 +39,38 @@ func (s *AskForBidSuite) verify(response compute.BidResult, expected models.Reso
 }
 
 func (s *AskForBidSuite) TestPopulateResourceUsage() {
-	response := s.runAskForBidTest(bidResponseTestCase{})
+	response := s.runAskForBidTest(bidResponseTestCase{name: "populate resrouce usage"})
 	s.verify(response, s.config.DefaultJobResourceLimits)
 }
 
 func (s *AskForBidSuite) TestUseSubmittedResourceUsage() {
 	usage := models.Resources{CPU: 1, Memory: 2, Disk: 3}
 	response := s.runAskForBidTest(bidResponseTestCase{
+		name:      "use submitted resource usage",
 		execution: addResourceUsage(mock.Execution(), usage),
 	})
 	s.verify(response, usage)
 }
 
 func (s *AskForBidSuite) TestAcceptUsageBelowLimits() {
 	s.runAskForBidTest(bidResponseTestCase{
+		name: "accept usage below limits",
 		execution: addResourceUsage(mock.Execution(),
 			models.Resources{CPU: s.config.JobResourceLimits.CPU / 2}),
 	})
 }
 
 func (s *AskForBidSuite) TestAcceptUsageMatachingLimits() {
 	s.runAskForBidTest(bidResponseTestCase{
+		name: "accept usage matching limits",
 		execution: addResourceUsage(mock.Execution(),
 			models.Resources{CPU: s.config.JobResourceLimits.CPU}),
 	})
 }
 
 func (s *AskForBidSuite) TestRejectUsageExceedingLimits() {
 	s.runAskForBidTest(bidResponseTestCase{
+		name: "reject usage exceeding limits",
 		execution: addResourceUsage(mock.Execution(),
 			models.Resources{CPU: s.config.JobResourceLimits.CPU + 0.01}),
 		rejected: true,
@@ -77,23 +80,26 @@ func (s *AskForBidSuite) TestRejectUsageExceedingLimits() {
 func (s *AskForBidSuite) runAskForBidTest(testCase bidResponseTestCase) compute.BidResult {
 	ctx := context.Background()
 
-	// setup default values
-	execution := testCase.execution
-	if execution == nil {
-		execution = mock.Execution()
-	}
-
-	result := s.askForBid(ctx, execution)
-	s.Equal(!testCase.rejected, result.Accepted)
-
-	// check execution state
-	localExecutionState, err := s.node.ExecutionStore.GetExecution(ctx, result.ExecutionID)
-	if testCase.rejected {
-		s.ErrorIs(err, store.NewErrExecutionNotFound(result.ExecutionID))
-	} else {
-		s.NoError(err)
-		s.Equal(store.ExecutionStateCreated, localExecutionState.State)
-	}
+	var result compute.BidResult
+	_ = s.Run(testCase.name, func() {
+		// setup default values
+		execution := testCase.execution
+		if execution == nil {
+			execution = mock.Execution()
+		}
+
+		result = s.askForBid(ctx, execution)
+		s.Equal(!testCase.rejected, result.Accepted)
+
+		// check execution state
+		localExecutionState, err := s.node.ExecutionStore.GetExecution(ctx, result.ExecutionID)
+		if testCase.rejected {
+			s.ErrorIs(err, store.NewErrExecutionNotFound(result.ExecutionID))
+		} else {
+			s.NoError(err)
+			s.Equal(store.ExecutionStateCreated, localExecutionState.State)
+		}
+	})
 
 	return result
 }

pkg/test/compute/resourcelimits_test.go

@@ -23,7 +23,6 @@ import (
 	noop_executor "github.com/bacalhau-project/bacalhau/pkg/executor/noop"
 	"github.com/bacalhau-project/bacalhau/pkg/job"
 	"github.com/bacalhau-project/bacalhau/pkg/logger"
-	_ "github.com/bacalhau-project/bacalhau/pkg/logger"
 	"github.com/bacalhau-project/bacalhau/pkg/node"
 	"github.com/bacalhau-project/bacalhau/pkg/setup"
 	"github.com/bacalhau-project/bacalhau/pkg/system"

pkg/test/compute/setup_test.go

@@ -13,7 +13,6 @@ import (
 
 	"github.com/bacalhau-project/bacalhau/pkg/authz"
 	"github.com/bacalhau-project/bacalhau/pkg/compute"
-	"github.com/bacalhau-project/bacalhau/pkg/compute/store"
 	"github.com/bacalhau-project/bacalhau/pkg/compute/store/resolver"
 	"github.com/bacalhau-project/bacalhau/pkg/executor"
 	noop_executor "github.com/bacalhau-project/bacalhau/pkg/executor/noop"
@@ -142,14 +141,3 @@ func (s *ComputeSuite) prepareAndAskForBid(ctx context.Context, execution *model
 	s.True(result.Accepted)
 	return result.ExecutionID
 }
-
-func (s *ComputeSuite) prepareAndRun(ctx context.Context, execution *models.Execution) string {
-	executionID := s.prepareAndAskForBid(ctx, execution)
-
-	// run the job
-	_, err := s.node.LocalEndpoint.BidAccepted(ctx, compute.BidAcceptedRequest{ExecutionID: executionID})
-	s.NoError(err)
-	err = s.stateResolver.Wait(ctx, executionID, resolver.CheckForState(store.ExecutionStateCompleted))
-	s.NoError(err)
-	return executionID
-}

pkg/test/devstack/submit_test.go

@@ -15,7 +15,6 @@ import (
 	testutils "github.com/bacalhau-project/bacalhau/pkg/test/teststack"
 
 	"github.com/bacalhau-project/bacalhau/pkg/logger"
-	_ "github.com/bacalhau-project/bacalhau/pkg/logger"
 	"github.com/bacalhau-project/bacalhau/pkg/model"
 )