From f00fcc87ad23255d21ee20f4c1bc65f8c8db492a Mon Sep 17 00:00:00 2001 From: Mike Han <56001373+mhan83@users.noreply.github.com> Date: Fri, 16 Aug 2024 14:54:58 -0600 Subject: [PATCH] feat: Add option to download matching artifacts for all job attempts (#937) * Download based on attempt not last jobData * Download artifacts after an attempt * Remove unneeded properties * Add artifacts to result * Have downloader handle all logic * cleanup * Cleanup args list * Cleanup * fixup test mocks * update test * update schema * lint * add option to other schema * add cli flag for allAttempts * Update internal/saucecloud/downloader/downloader.go Co-authored-by: Alex Plischke * fixup test * lint --------- Co-authored-by: Alex Plischke --- api/saucectl.schema.json | 8 +++++ api/v1/subschema/artifacts.schema.json | 4 +++ api/v1alpha/subschema/artifacts.schema.json | 4 +++ internal/cmd/run/run.go | 1 + internal/config/config.go | 7 ++-- internal/job/job.go | 2 +- internal/mocks/download.go | 8 +++-- internal/saucecloud/cloud.go | 34 ++++++++----------- internal/saucecloud/cloud_test.go | 18 ++++++++++ internal/saucecloud/cypress_test.go | 2 +- internal/saucecloud/downloader/downloader.go | 17 +++++++--- .../saucecloud/downloader/downloader_test.go | 12 +++++-- internal/saucecloud/jobservice.go | 8 ++--- 13 files changed, 88 insertions(+), 37 deletions(-) diff --git a/api/saucectl.schema.json b/api/saucectl.schema.json index b7c919229..0dd078656 100644 --- a/api/saucectl.schema.json +++ b/api/saucectl.schema.json @@ -70,6 +70,10 @@ "directory": { "description": "Specifies the path to the folder in which to download artifacts. A separate subdirectory is generated in this location for each suite.", "type": "string" + }, + "allAttempts": { + "description": "If true and a test is retried, artifacts for every attempt will be downloaded. Otherwise, only artifacts for the final attempt will be downloaded.", + "type": "boolean" } }, "required": [ @@ -725,6 +729,10 @@ "directory": { "description": "Specifies the path to the folder in which to download artifacts. A separate subdirectory is generated in this location for each suite.", "type": "string" + }, + "allAttempts": { + "description": "If true and a test is retried, artifacts for every attempt will be downloaded. Otherwise, only artifacts for the final attempt will be downloaded.", + "type": "boolean" } }, "required": [ diff --git a/api/v1/subschema/artifacts.schema.json b/api/v1/subschema/artifacts.schema.json index b3e4ad199..26cc3a03d 100644 --- a/api/v1/subschema/artifacts.schema.json +++ b/api/v1/subschema/artifacts.schema.json @@ -32,6 +32,10 @@ "directory": { "description": "Specifies the path to the folder in which to download artifacts. A separate subdirectory is generated in this location for each suite.", "type": "string" + }, + "allAttempts": { + "description": "If true and a test is retried, artifacts for every attempt will be downloaded. Otherwise, only artifacts for the final attempt will be downloaded.", + "type": "boolean" } }, "required": [ diff --git a/api/v1alpha/subschema/artifacts.schema.json b/api/v1alpha/subschema/artifacts.schema.json index b3e4ad199..26cc3a03d 100644 --- a/api/v1alpha/subschema/artifacts.schema.json +++ b/api/v1alpha/subschema/artifacts.schema.json @@ -32,6 +32,10 @@ "directory": { "description": "Specifies the path to the folder in which to download artifacts. A separate subdirectory is generated in this location for each suite.", "type": "string" + }, + "allAttempts": { + "description": "If true and a test is retried, artifacts for every attempt will be downloaded. Otherwise, only artifacts for the final attempt will be downloaded.", + "type": "boolean" } }, "required": [ diff --git a/internal/cmd/run/run.go b/internal/cmd/run/run.go index 03615bdf0..98e950856 100644 --- a/internal/cmd/run/run.go +++ b/internal/cmd/run/run.go @@ -128,6 +128,7 @@ func Command() *cobra.Command { sc.String("artifacts.download.when", "artifacts::download::when", "never", "Specifies when to download test artifacts") sc.StringSlice("artifacts.download.match", "artifacts::download::match", []string{}, "Specifies which test artifacts to download") sc.String("artifacts.download.directory", "artifacts::download::directory", "", "Specifies the location where to download test artifacts to") + sc.Bool("artifacts.download.allAttempts", "artifacts::download::allAttempts", false, "Specifies whether to download artifacts for all attempted tests if a test needs to be retried. If false, only the artifacts of the last attempt will be downloaded.") sc.Bool("artifacts.cleanup", "artifacts::cleanup", false, "Specifies whether to remove all contents of artifacts directory") // Reporters diff --git a/internal/config/config.go b/internal/config/config.go index 7302886bb..11cf8e814 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -124,9 +124,10 @@ func (w When) IsNow(passed bool) bool { // ArtifactDownload represents the test artifacts configuration. type ArtifactDownload struct { - Match []string `yaml:"match,omitempty" json:"match"` - When When `yaml:"when,omitempty" json:"when"` - Directory string `yaml:"directory,omitempty" json:"directory"` + Match []string `yaml:"match,omitempty" json:"match"` + When When `yaml:"when,omitempty" json:"when"` + Directory string `yaml:"directory,omitempty" json:"directory"` + AllAttempts bool `yaml:"allAttempts,omitempty" json:"allAttempts"` } // Notifications represents the test notifications configuration. diff --git a/internal/job/job.go b/internal/job/job.go index 8ab774271..ea8fed944 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(jobID, suiteName string, realDevice bool) []string + DownloadArtifact(job Job, attempt int, retries int) []string } diff --git a/internal/mocks/download.go b/internal/mocks/download.go index b3830337b..738d8238a 100644 --- a/internal/mocks/download.go +++ b/internal/mocks/download.go @@ -1,11 +1,13 @@ package mocks +import "github.com/saucelabs/saucectl/internal/job" + // FakeArtifactDownloader defines a fake Downloader type FakeArtifactDownloader struct { - DownloadArtifactFn func(jobID, suiteName string) []string + DownloadArtifactFn func(jobData job.Job, attempt int, retries int) []string } // DownloadArtifact defines a fake function for FakeDownloader -func (f *FakeArtifactDownloader) DownloadArtifact(jobID, suiteName string, realDevice bool) []string { - return f.DownloadArtifactFn(jobID, suiteName) +func (f *FakeArtifactDownloader) DownloadArtifact(jobData job.Job, attempt int, retries int) []string { + return f.DownloadArtifactFn(jobData, attempt, retries) } diff --git a/internal/saucecloud/cloud.go b/internal/saucecloud/cloud.go index b5924316e..9ec5c248f 100644 --- a/internal/saucecloud/cloud.go +++ b/internal/saucecloud/cloud.go @@ -83,7 +83,8 @@ type result struct { retries int attempts []report.Attempt - details insights.Details + details insights.Details + artifacts []report.Artifact } // ConsoleLogAsset represents job asset log file name. @@ -146,15 +147,7 @@ func (r *CloudRunner) collectResults(artifactCfg config.ArtifactDownload, result browser = fmt.Sprintf("%s %s", browser, res.job.BrowserVersion) } - var artifacts []report.Artifact - files := r.downloadArtifacts(res.name, res.job, artifactCfg.When) - for _, f := range files { - artifacts = append(artifacts, report.Artifact{ - FilePath: f, - }) - } - - r.FetchJUnitReports(&res, artifacts) + r.FetchJUnitReports(&res, res.artifacts) var url string if res.job.ID != "" { @@ -171,7 +164,7 @@ func (r *CloudRunner) collectResults(artifactCfg config.ArtifactDownload, result Platform: platform, DeviceName: res.job.DeviceName, URL: url, - Artifacts: artifacts, + Artifacts: res.artifacts, Origin: "sauce", RDC: res.job.IsRDC, TimedOut: res.job.TimedOut, @@ -341,6 +334,7 @@ func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- resu } if opts.Attempt < opts.Retries && ((!jobData.Passed && !skipped) || (opts.CurrentPassCount < opts.PassThreshold)) { + go r.JobService.DownloadArtifact(jobData, opts.Attempt, opts.Retries) if !jobData.Passed { log.Warn().Err(err).Msg("Suite errored.") } @@ -355,6 +349,7 @@ func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- resu TestSuites: junit.TestSuites{}, }) go r.Retrier.Retry(jobOpts, opts, jobData) + continue } @@ -375,6 +370,14 @@ func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- resu } } + files := r.JobService.DownloadArtifact(jobData, opts.Attempt, opts.Retries) + var artifacts []report.Artifact + for _, f := range files { + artifacts = append(artifacts, report.Artifact{ + FilePath: f, + }) + } + results <- result{ name: opts.DisplayName, browser: opts.BrowserName, @@ -393,6 +396,7 @@ func (r *CloudRunner) runJobs(jobOpts chan job.StartOptions, results chan<- resu EndTime: time.Now(), Status: jobData.Status, }), + artifacts: artifacts, } } } @@ -999,14 +1003,6 @@ func (r *CloudRunner) loadJUnitReport(jobID string, isRDC bool) (junit.TestSuite return junit.Parse(fileContent) } -func (r *CloudRunner) downloadArtifacts(suiteName string, job job.Job, when config.When) []string { - if job.ID == "" || job.TimedOut || r.Async || !when.IsNow(job.Passed) { - return []string{} - } - - return r.JobService.DownloadArtifact(job.ID, suiteName, job.IsRDC) -} - func arrayContains(list []string, want string) bool { for _, item := range list { if item == want { diff --git a/internal/saucecloud/cloud_test.go b/internal/saucecloud/cloud_test.go index 64646bd82..c6dff61ec 100644 --- a/internal/saucecloud/cloud_test.go +++ b/internal/saucecloud/cloud_test.go @@ -166,6 +166,12 @@ 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 { + return []string{} + }}, + RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string { + return []string{} + }}, }, } @@ -221,6 +227,12 @@ 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 { + return []string{} + }}, + RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string { + return []string{} + }}, }, } @@ -257,6 +269,12 @@ 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 { + return []string{} + }}, + RDCDownloader: &mocks.FakeArtifactDownloader{DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string { + return []string{} + }}, }, } diff --git a/internal/saucecloud/cypress_test.go b/internal/saucecloud/cypress_test.go index 8f1d42103..f5f0b0136 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(jobID string, suiteName string) []string { + DownloadArtifactFn: func(jobData job.Job, attempt int, retries int) []string { return []string{} }, }, diff --git a/internal/saucecloud/downloader/downloader.go b/internal/saucecloud/downloader/downloader.go index 95706ce49..78692f389 100644 --- a/internal/saucecloud/downloader/downloader.go +++ b/internal/saucecloud/downloader/downloader.go @@ -23,13 +23,21 @@ func NewArtifactDownloader(reader job.Reader, artifactConfig config.ArtifactDown } } -func (d *ArtifactDownloader) DownloadArtifact(jobID string, suiteName string, realDevice bool) []string { - targetDir, err := config.GetSuiteArtifactFolder(suiteName, d.config) +func (d *ArtifactDownloader) DownloadArtifact(jobData job.Job, attemptNumber int, retries int) []string { + if jobData.ID == "" || + jobData.TimedOut || !job.Done(jobData.Status) || + !d.config.When.IsNow(jobData.Passed) || + (!d.config.AllAttempts && attemptNumber < retries) { + return []string{} + } + + destDir, err := config.GetSuiteArtifactFolder(jobData.Name, d.config) if err != nil { log.Error().Msgf("Unable to create artifacts folder (%v)", err) return []string{} } - files, err := d.reader.GetJobAssetFileNames(context.Background(), jobID, realDevice) + + files, err := d.reader.GetJobAssetFileNames(context.Background(), jobData.ID, jobData.IsRDC) if err != nil { log.Error().Msgf("Unable to fetch artifacts list (%v)", err) return []string{} @@ -37,8 +45,9 @@ func (d *ArtifactDownloader) DownloadArtifact(jobID string, suiteName string, re filepaths := fpath.MatchFiles(files, d.config.Match) var artifacts []string + for _, f := range filepaths { - targetFile, err := d.downloadArtifact(targetDir, jobID, f, realDevice) + targetFile, err := d.downloadArtifact(destDir, jobData.ID, f, jobData.IsRDC) if err != nil { log.Err(err).Msg("Unable to download artifacts") return artifacts diff --git a/internal/saucecloud/downloader/downloader_test.go b/internal/saucecloud/downloader/downloader_test.go index 490c7260e..5aed86186 100644 --- a/internal/saucecloud/downloader/downloader_test.go +++ b/internal/saucecloud/downloader/downloader_test.go @@ -10,6 +10,7 @@ import ( "github.com/saucelabs/saucectl/internal/config" httpServices "github.com/saucelabs/saucectl/internal/http" + "github.com/saucelabs/saucectl/internal/job" ) func TestArtifactDownloader_DownloadArtifact(t *testing.T) { @@ -43,10 +44,17 @@ func TestArtifactDownloader_DownloadArtifact(t *testing.T) { artifactCfg := config.ArtifactDownload{ Directory: tempDir, Match: []string{"junit.xml"}, + When: config.WhenAlways, } - downloader := NewArtifactDownloader(&rc, artifactCfg) - downloader.DownloadArtifact("test-123", "suite name", true) + downloader.DownloadArtifact( + job.Job{ + ID: "test-123", + Name: "suite name", + IsRDC: true, + Status: job.StateComplete, + }, 0, 0, + ) fileName := filepath.Join(tempDir, "suite_name", "junit.xml") d, err := os.ReadFile(fileName) diff --git a/internal/saucecloud/jobservice.go b/internal/saucecloud/jobservice.go index 363598c05..eba543915 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(jobID, suiteName string, realDevice bool) []string { - if realDevice { - return s.RDCDownloader.DownloadArtifact(jobID, suiteName, realDevice) +func (s JobService) DownloadArtifact(jobData job.Job, attemptNumber int, retries int) []string { + if jobData.IsRDC { + return s.RDCDownloader.DownloadArtifact(jobData, attemptNumber, retries) } - return s.VDCDownloader.DownloadArtifact(jobID, suiteName, realDevice) + return s.VDCDownloader.DownloadArtifact(jobData, attemptNumber, retries) } func (s JobService) StopJob(ctx context.Context, jobID string, realDevice bool) (job.Job, error) {