fix: Should download artifacts when no retries (#943)

* fix: Should download artifacts when no retries

* revise comments

* revise logic

* revise shouldRetry

* We've known if it's the last attemp before downloading

* fix unit test

* revise downloader

* add ut for skipDownload

internal/job/job.go

@@ -93,5 +93,5 @@ type Service interface {
 // ArtifactDownloader represents the interface for downloading artifacts.
 type ArtifactDownloader interface {
 	// DownloadArtifact downloads artifacts and returns a list of what was downloaded.
-	DownloadArtifact(job Job, attempt int, retries int) []string
+	DownloadArtifact(job Job, isLastAttempt bool) []string
 }

internal/mocks/download.go

@@ -4,10 +4,10 @@ import "github.com/saucelabs/saucectl/internal/job"
 
 // FakeArtifactDownloader defines a fake Downloader
 type FakeArtifactDownloader struct {
-	DownloadArtifactFn func(jobData job.Job, attempt int, retries int) []string
+	DownloadArtifactFn func(jobData job.Job, isLastAttempt bool) []string
 }
 
 // DownloadArtifact defines a fake function for FakeDownloader
-func (f *FakeArtifactDownloader) DownloadArtifact(jobData job.Job, attempt int, retries int) []string {
-	return f.DownloadArtifactFn(jobData, attempt, retries)
+func (f *FakeArtifactDownloader) DownloadArtifact(jobData job.Job, isLastAttempt bool) []string {
+	return f.DownloadArtifactFn(jobData, isLastAttempt)
 }

internal/saucecloud/cloud.go

@@ -299,6 +299,26 @@ func (r *CloudRunner) runJob(opts job.StartOptions) (j job.Job, skipped bool, er
 	return j, false, nil
 }
 
+func belowRetryLimit(opts job.StartOptions) bool {
+	return opts.Attempt < opts.Retries
+}
+
+func belowThreshold(opts job.StartOptions) bool {
+	return opts.CurrentPassCount < opts.PassThreshold
+}
+
+// shouldRetryJob checks if the job should be retried,
+// based on whether it passed and if it was skipped.
+func shouldRetryJob(jobData job.Job, skipped bool) bool {
+	return !jobData.Passed && !skipped
+}
+
+// shouldRetry determines whether a job should be retried.
+func shouldRetry(opts job.StartOptions, jobData job.Job, skipped bool) bool {
+	return belowRetryLimit(opts) &&
+		(shouldRetryJob(jobData, skipped) || belowThreshold(opts))
+}
+
 func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- result) {
 	for opts := range jobOpts {
 		start := time.Now()
@@ -333,8 +353,8 @@ func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- resu
 			opts.CurrentPassCount++
 		}
 
-		if opts.Attempt < opts.Retries && ((!jobData.Passed && !skipped) || (opts.CurrentPassCount < opts.PassThreshold)) {
-			go r.JobService.DownloadArtifact(jobData, opts.Attempt, opts.Retries)
+		if shouldRetry(opts, jobData, skipped) {
+			go r.JobService.DownloadArtifact(jobData, false)
 			if !jobData.Passed {
 				log.Warn().Err(err).Msg("Suite errored.")
 			}
@@ -370,7 +390,7 @@ func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- resu
 			}
 		}
 
-		files := r.JobService.DownloadArtifact(jobData, opts.Attempt, opts.Retries)
+		files := r.JobService.DownloadArtifact(jobData, true)
 		var artifacts []report.Artifact
 		for _, f := range files {
 			artifacts = append(artifacts, report.Artifact{

internal/saucecloud/cloud_test.go

@@ -166,10 +166,10 @@ func TestRunJobTimeout(t *testing.T) {
 			VDCWriter: &mocks.FakeJobWriter{UploadAssetFn: func(jobID string, fileName string, contentType string, content []byte) error {
 				return nil
 			}},
-			VDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+			VDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, isLastAttempt bool) []string {
 				return []string{}
 			}},
-			RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+			RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, isLastAttempt bool) []string {
 				return []string{}
 			}},
 		},
@@ -227,10 +227,10 @@ func TestRunJobRetries(t *testing.T) {
 				VDCWriter: &mocks.FakeJobWriter{UploadAssetFn: func(jobID string, fileName string, contentType string, content []byte) error {
 					return nil
 				}},
-				VDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+				VDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, islastAttempt bool) []string {
 					return []string{}
 				}},
-				RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+				RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, islastAttempt bool) []string {
 					return []string{}
 				}},
 			},
@@ -269,10 +269,10 @@ func TestRunJobTimeoutRDC(t *testing.T) {
 					return job.Job{ID: id, TimedOut: true}, nil
 				},
 			},
-			VDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+			VDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, islastAttempt bool) []string {
 				return []string{}
 			}},
-			RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+			RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, islastAttempt bool) []string {
 				return []string{}
 			}},
 		},

internal/saucecloud/cypress_test.go

@@ -78,7 +78,7 @@ func TestRunSuites(t *testing.T) {
 					},
 				},
 				VDCDownloader: &mocks.FakeArtifactDownloader{
-					DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string {
+					DownloadArtifactFn: func(jobData job.Job, islastAttempt bool) []string {
 						return []string{}
 					},
 				},

internal/saucecloud/downloader/downloader.go

@@ -23,11 +23,15 @@ func NewArtifactDownloader(reader job.Reader, artifactConfig config.ArtifactDown
 	}
 }
 
-func (d *ArtifactDownloader) DownloadArtifact(jobData job.Job, attemptNumber int, retries int) []string {
-	if jobData.ID == "" ||
+func (d *ArtifactDownloader) skipDownload(jobData job.Job, isLastAttempt bool) bool {
+	return jobData.ID == "" ||
 		jobData.TimedOut || !job.Done(jobData.Status) ||
 		!d.config.When.IsNow(jobData.Passed) ||
-		(!d.config.AllAttempts && attemptNumber < retries) {
+		(!isLastAttempt && !d.config.AllAttempts)
+}
+
+func (d *ArtifactDownloader) DownloadArtifact(jobData job.Job, isLastAttempt bool) []string {
+	if d.skipDownload(jobData, isLastAttempt) {
 		return []string{}
 	}
 

internal/saucecloud/downloader/downloader_test.go

@@ -11,6 +11,7 @@ import (
 	"github.com/saucelabs/saucectl/internal/config"
 	httpServices "github.com/saucelabs/saucectl/internal/http"
 	"github.com/saucelabs/saucectl/internal/job"
+	"gotest.tools/v3/assert"
 )
 
 func TestArtifactDownloader_DownloadArtifact(t *testing.T) {
@@ -53,7 +54,7 @@ func TestArtifactDownloader_DownloadArtifact(t *testing.T) {
 			Name:   "suite name",
 			IsRDC:  true,
 			Status: job.StateComplete,
-		}, 0, 0,
+		}, true,
 	)
 
 	fileName := filepath.Join(tempDir, "suite_name", "junit.xml")
@@ -66,3 +67,72 @@ func TestArtifactDownloader_DownloadArtifact(t *testing.T) {
 		t.Errorf("file content mismatch: got '%v', expects: '%v'", d, fileContent)
 	}
 }
+
+func TestSkipDownload(t *testing.T) {
+	testcases := []struct {
+		name          string
+		config        config.ArtifactDownload
+		jobData       job.Job
+		isLastAttempt bool
+		expResult     bool
+	}{
+		{
+			name:          "Should not skip download",
+			config:        config.ArtifactDownload{When: config.WhenAlways},
+			jobData:       job.Job{ID: "fake-id", Status: job.StatePassed, Passed: true},
+			isLastAttempt: true,
+			expResult:     false,
+		},
+		{
+			name:          "Should skip download when job ID is empty",
+			config:        config.ArtifactDownload{When: config.WhenAlways},
+			jobData:       job.Job{Status: job.StatePassed, Passed: true},
+			isLastAttempt: true,
+			expResult:     true,
+		},
+		{
+			name:          "Should skip download when job is timeout",
+			config:        config.ArtifactDownload{When: config.WhenAlways},
+			jobData:       job.Job{ID: "fake-id", TimedOut: true},
+			isLastAttempt: true,
+			expResult:     true,
+		},
+		{
+			name:          "Should skip download when job is not done",
+			config:        config.ArtifactDownload{When: config.WhenAlways},
+			jobData:       job.Job{ID: "fake-id", TimedOut: true, Status: job.StateInProgress},
+			isLastAttempt: true,
+			expResult:     true,
+		},
+		{
+			name:          "Should skip download when artifact config is not set to download",
+			config:        config.ArtifactDownload{When: config.WhenNever},
+			jobData:       job.Job{ID: "fake-id", Status: job.StatePassed, Passed: true},
+			isLastAttempt: true,
+			expResult:     true,
+		},
+		{
+			name:          "Should skip download when it's not last attempt and not set download all attempts",
+			config:        config.ArtifactDownload{When: config.WhenAlways, AllAttempts: false},
+			jobData:       job.Job{ID: "fake-id", Status: job.StatePassed, Passed: true},
+			isLastAttempt: false,
+			expResult:     true,
+		},
+		{
+			name:          "Should download when it's the last attempt and not set download all attempts",
+			config:        config.ArtifactDownload{When: config.WhenAlways, AllAttempts: false},
+			jobData:       job.Job{ID: "fake-id", Status: job.StatePassed, Passed: true},
+			isLastAttempt: true,
+			expResult:     false,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			downloader := &ArtifactDownloader{config: tc.config}
+			got := downloader.skipDownload(tc.jobData, tc.isLastAttempt)
+			assert.Equal(t, tc.expResult, got)
+		})
+	}
+
+}

internal/saucecloud/jobservice.go

@@ -23,12 +23,12 @@ type JobService struct {
 	RDCDownloader job.ArtifactDownloader
 }
 
-func (s JobService) DownloadArtifact(jobData job.Job, attemptNumber int, retries int) []string {
+func (s JobService) DownloadArtifact(jobData job.Job, isLastAttempt bool) []string {
 	if jobData.IsRDC {
-		return s.RDCDownloader.DownloadArtifact(jobData, attemptNumber, retries)
+		return s.RDCDownloader.DownloadArtifact(jobData, isLastAttempt)
 	}
 
-	return s.VDCDownloader.DownloadArtifact(jobData, attemptNumber, retries)
+	return s.VDCDownloader.DownloadArtifact(jobData, isLastAttempt)
 }
 
 func (s JobService) StopJob(ctx context.Context, jobID string, realDevice bool) (job.Job, error) {