diff --git a/internal/job/job.go b/internal/job/job.go index ea8fed944..41c4bdab9 100644 --- a/internal/job/job.go +++ b/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 } diff --git a/internal/mocks/download.go b/internal/mocks/download.go index 738d8238a..edb611ade 100644 --- a/internal/mocks/download.go +++ b/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) } diff --git a/internal/saucecloud/cloud.go b/internal/saucecloud/cloud.go index 9ec5c248f..9ece74998 100644 --- a/internal/saucecloud/cloud.go +++ b/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{ diff --git a/internal/saucecloud/cloud_test.go b/internal/saucecloud/cloud_test.go index c6dff61ec..134605606 100644 --- a/internal/saucecloud/cloud_test.go +++ b/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{} }}, }, diff --git a/internal/saucecloud/cypress_test.go b/internal/saucecloud/cypress_test.go index f5f0b0136..ea9816e8e 100644 --- a/internal/saucecloud/cypress_test.go +++ b/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{} }, }, diff --git a/internal/saucecloud/downloader/downloader.go b/internal/saucecloud/downloader/downloader.go index 78692f389..b3ec94430 100644 --- a/internal/saucecloud/downloader/downloader.go +++ b/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{} } diff --git a/internal/saucecloud/downloader/downloader_test.go b/internal/saucecloud/downloader/downloader_test.go index 5aed86186..f43fbfd45 100644 --- a/internal/saucecloud/downloader/downloader_test.go +++ b/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) + }) + } + +} diff --git a/internal/saucecloud/jobservice.go b/internal/saucecloud/jobservice.go index eba543915..dc3218fef 100644 --- a/internal/saucecloud/jobservice.go +++ b/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) {