feat: skip smart retry on errored jobs (#932)

internal/cucumber/config.go

@@ -327,8 +327,9 @@ func (p *Project) FilterFailedTests(suiteName string, report saucereport.SauceRe
 		if s.Name != suiteName {
 			continue
 		}
-		found = true
 		p.Suites[i].Options.Paths = specs
+		found = true
+		break
 	}
 
 	if !found {
@@ -342,11 +343,13 @@ func getFailedSpecFiles(report saucereport.SauceReport) ([]string, error) {
 	if report.Status != saucereport.StatusFailed {
 		return failedSpecs, nil
 	}
+
+	re, err := regexp.Compile(".*.feature$")
+	if err != nil {
+		return failedSpecs, err
+	}
+
 	for _, s := range report.Suites {
-		re, err := regexp.Compile(".*.feature$")
-		if err != nil {
-			return failedSpecs, err
-		}
 		if s.Status == saucereport.StatusFailed && re.MatchString(s.Name) {
 			failedSpecs = append(failedSpecs, filepath.Clean(s.Name))
 		}

internal/cypress/v1/config.go

@@ -569,12 +569,12 @@ func (p *Project) FilterFailedTests(suiteName string, report saucereport.SauceRe
 		if s.Name != suiteName {
 			continue
 		}
-		found = true
 		if p.Suites[i].Config.Env == nil {
 			p.Suites[i].Config.Env = map[string]string{}
 		}
 		p.Suites[i].Config.Env["grep"] = strings.Join(failedTests, ";")
-
+		found = true
+		break
 	}
 	if !found {
 		return fmt.Errorf("suite(%s) not found", suiteName)

internal/msg/errormsg.go

@@ -204,6 +204,9 @@ const (
 	UnableToArchiveRunnerConfig = "Unable to archive sauce runner config file"
 	// UnableToUploadConfig indicates a failure to upload config
 	UnableToUploadConfig = "Unable to upload sauce runner config file %q"
+	// UnreliableReport indicates that the job is not smart-retryable due to its
+	// status.
+	UnreliableReport = "Test reports from errored jobs are not a reliable source to correctly determine failed tests."
 )
 
 // container

internal/playwright/config.go

@@ -433,8 +433,9 @@ func (p *Project) FilterFailedTests(suiteName string, report saucereport.SauceRe
 		if s.Name != suiteName {
 			continue
 		}
-		found = true
 		p.Suites[i].Params.Grep = strings.Join(failedTests, "|")
+		found = true
+		break
 	}
 	if !found {
 		return fmt.Errorf("suite(%s) not found", suiteName)

internal/saucecloud/cloud.go

@@ -958,7 +958,7 @@ func (r *CloudRunner) reportSuiteToInsights(res result) {
 	res.details.Device = j.DeviceName
 
 	var testRuns []insights.TestRun
-	if arrayContains(assets, saucereport.SauceReportFileName) {
+	if arrayContains(assets, saucereport.FileName) {
 		report, err := r.loadSauceTestReport(res.job.ID, res.job.IsRDC)
 		if err != nil {
 			log.Warn().Err(err).Str("action", "parsingJSON").Str("jobID", res.job.ID).Msg(msg.InsightsReportError)
@@ -982,7 +982,7 @@ func (r *CloudRunner) reportSuiteToInsights(res result) {
 }
 
 func (r *CloudRunner) loadSauceTestReport(jobID string, isRDC bool) (saucereport.SauceReport, error) {
-	fileContent, err := r.JobService.GetJobAssetFileContent(context.Background(), jobID, saucereport.SauceReportFileName, isRDC)
+	fileContent, err := r.JobService.GetJobAssetFileContent(context.Background(), jobID, saucereport.FileName, isRDC)
 	if err != nil {
 		log.Warn().Err(err).Str("action", "loading-json-report").Msg(msg.InsightsReportError)
 		return saucereport.SauceReport{}, err

internal/saucecloud/cloud_test.go

@@ -491,10 +491,10 @@ func TestCloudRunner_loadSauceTestReport(t *testing.T) {
 			},
 			fields: fields{
 				GetJobAssetFileNamesFn: func(ctx context.Context, jobID string) ([]string, error) {
-					return []string{saucereport.SauceReportFileName}, nil
+					return []string{saucereport.FileName}, nil
 				},
 				GetJobAssetFileContentFn: func(ctx context.Context, jobID, fileName string) ([]byte, error) {
-					if fileName == saucereport.SauceReportFileName {
+					if fileName == saucereport.FileName {
 						return []byte(`{"status":"failed","attachments":[],"suites":[{"name":"cypress/e2e/examples/actions.cy.js","status":"failed","metadata":{},"suites":[{"name":"Actions","status":"failed","metadata":{},"suites":[],"attachments":[],"tests":[{"name":".type() - type into a DOM element","status":"passed","startTime":"2022-12-22T10:10:11.083Z","duration":1802,"metadata":{},"output":null,"attachments":[],"code":{"lines":["() => {","    // https://on.cypress.io/type","    cy.get('.action-email').type('[email protected]').should('have.value', '[email protected]');","  }"]},"videoTimestamp":26.083},{"name":".type() - type into a wrong DOM element","status":"failed","startTime":"2022-12-22T10:10:12.907Z","duration":5010,"metadata":{},"output":"AssertionError: Timed out retrying after 4000ms: expected '<input#email1.form-control.action-email>' to have value '[email protected]', but the value was '[email protected]'\n\n  11 |     // https://on.cypress.io/type\n  12 |     cy.get('.action-email')\n> 13 |         .type('[email protected]').should('have.value', '[email protected]')\n     |                                 ^\n  14 |   })\n  15 | })\n  16 | ","attachments":[{"name":"screenshot","path":"Actions -- .type() - type into a wrong DOM element (failed).png","contentType":"image/png"}],"code":{"lines":["() => {","    // https://on.cypress.io/type","    cy.get('.action-email').type('[email protected]').should('have.value', '[email protected]');","  }"]},"videoTimestamp":27.907}]}],"attachments":[],"tests":[]}],"metadata":{}}`), nil
 					}
 					return []byte{}, errors.New("not-found")

internal/saucecloud/retry/junitretrier.go

@@ -18,35 +18,60 @@ type JunitRetrier struct {
 	VDCReader job.Reader
 }
 
-func (b *JunitRetrier) retryFailedTests(reader job.Reader, jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
+func (b *JunitRetrier) Retry(jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
+	var tests []string
+
+	if opt.SmartRetry.FailedOnly {
+		jobReader := b.VDCReader
+		if previous.IsRDC {
+			jobReader = b.RDCReader
+		}
+
+		tests = b.retryFailedTests(jobReader, &opt, previous)
+		if len(tests) == 0 {
+			log.Info().Msg(msg.SkippingSmartRetries)
+		}
+	}
+
+	lg := log.Info().
+		Str("suite", opt.DisplayName).
+		Str("attempt", fmt.Sprintf("%d of %d", opt.Attempt+1, opt.Retries+1))
+
+	if len(tests) > 0 {
+		lg.Msgf(msg.RetryWithTests, tests)
+	} else {
+		lg.Msg("Retrying suite.")
+	}
+
+	jobOpts <- opt
+}
+
+func (b *JunitRetrier) retryFailedTests(reader job.Reader, opt *job.StartOptions, previous job.Job) []string {
+	if previous.Status == job.StateError {
+		log.Warn().Msg(msg.UnreliableReport)
+		return nil
+	}
+
 	content, err := reader.GetJobAssetFileContent(context.Background(), previous.ID, junit.FileName, previous.IsRDC)
 	if err != nil {
 		log.Err(err).Msgf(msg.UnableToFetchFile, junit.FileName)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+		return nil
 	}
+
 	suites, err := junit.Parse(content)
 	if err != nil {
 		log.Err(err).Msgf(msg.UnableToUnmarshallFile, junit.FileName)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+		return nil
 	}
 
-	setClassesToRetry(&opt, suites.TestCases())
-	jobOpts <- opt
+	return setClassesToRetry(opt, suites.TestCases())
 }
 
 // setClassesToRetry sets the correct filtering flag when retrying.
 // RDC API does not provide different endpoints (or identical values) for Espresso
 // and XCUITest. Thus, we need set the classes at the correct position depending
 // on the framework that is being executed.
-func setClassesToRetry(opt *job.StartOptions, testcases []junit.TestCase) {
-	lg := log.Info().
-		Str("suite", opt.DisplayName).
-		Str("attempt", fmt.Sprintf("%d of %d", opt.Attempt+1, opt.Retries+1))
-
+func setClassesToRetry(opt *job.StartOptions, testcases []junit.TestCase) []string {
 	if opt.TestOptions == nil {
 		opt.TestOptions = map[string]interface{}{}
 	}
@@ -60,30 +85,14 @@ func setClassesToRetry(opt *job.StartOptions, testcases []junit.TestCase) {
 		} else {
 			opt.TestOptions["class"] = tests
 		}
-		lg.Msgf(msg.RetryWithTests, tests)
-		return
+
+		return tests
 	}
 
 	tests := getFailedEspressoTests(testcases)
 	opt.TestOptions["class"] = tests
-	lg.Msgf(msg.RetryWithTests, tests)
-}
-
-func (b *JunitRetrier) Retry(jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
-	if b.RDCReader != nil && previous.IsRDC && opt.SmartRetry.FailedOnly {
-		b.retryFailedTests(b.RDCReader, jobOpts, opt, previous)
-		return
-	}
-
-	if b.VDCReader != nil && !previous.IsRDC && opt.SmartRetry.FailedOnly {
-		b.retryFailedTests(b.VDCReader, jobOpts, opt, previous)
-		return
-	}
 
-	log.Info().Str("suite", opt.DisplayName).
-		Str("attempt", fmt.Sprintf("%d of %d", opt.Attempt+1, opt.Retries+1)).
-		Msg("Retrying suite.")
-	jobOpts <- opt
+	return tests
 }
 
 // getFailedXCUITests returns a list of failed XCUITest tests from the given

internal/saucecloud/retry/junitretrier_test.go

@@ -203,37 +203,6 @@ func TestAppsRetrier_Retry(t *testing.T) {
 				RealDevice: true,
 			},
 		},
-		{
-			name: "Job not retrying if RDC and config is VDC + SmartRetry",
-			init: init{
-				RetryVDC: true,
-			},
-			args: args{
-				jobOpts: make(chan job.StartOptions),
-				opt: job.StartOptions{
-					DisplayName: "Dummy Test",
-					SmartRetry: job.SmartRetry{
-						FailedOnly: true,
-					},
-					TestOptions: map[string]interface{}{
-						"class": []string{"Demo.Class1", "Demo.Class2", "Demo.Class3"},
-					},
-				},
-				previous: job.Job{
-					ID:    "fake-job-id",
-					IsRDC: true,
-				},
-			},
-			expected: job.StartOptions{
-				DisplayName: "Dummy Test",
-				TestOptions: map[string]interface{}{
-					"class": []string{"Demo.Class1", "Demo.Class2", "Demo.Class3"},
-				},
-				SmartRetry: job.SmartRetry{
-					FailedOnly: true,
-				},
-			},
-		},
 		{
 			name: "Job is retrying when VDC + SmartRetry",
 			init: init{

internal/saucecloud/retry/saucereportretrier.go

@@ -24,9 +24,10 @@ type SauceReportRetrier struct {
 }
 
 func (r *SauceReportRetrier) Retry(jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
-	if r.VDCReader != nil && opt.SmartRetry.FailedOnly {
-		r.RetryFailedTests(jobOpts, opt, previous)
-		return
+	if opt.SmartRetry.FailedOnly {
+		if ok := r.retryFailedTests(&opt, previous); !ok {
+			log.Info().Msg(msg.SkippingSmartRetries)
+		}
 	}
 
 	log.Info().Str("suite", opt.DisplayName).
@@ -35,54 +36,53 @@ func (r *SauceReportRetrier) Retry(jobOpts chan<- job.StartOptions, opt job.Star
 	jobOpts <- opt
 }
 
-func (r *SauceReportRetrier) RetryFailedTests(jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
-	report, err := r.getSauceReport(previous)
-	if err != nil {
-		log.Err(err).Msgf(msg.UnableToFetchFile, saucereport.SauceReportFileName)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+func (r *SauceReportRetrier) retryFailedTests(opt *job.StartOptions, previous job.Job) bool {
+	if previous.Status == job.StateError {
+		log.Warn().Msg(msg.UnreliableReport)
+		return false
 	}
-	tempDir, err := os.MkdirTemp(os.TempDir(), "saucectl-app-payload-")
+
+	report, err := r.getSauceReport(previous)
 	if err != nil {
-		log.Err(err).Msg(msg.UnableToCreateRunnerConfig)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+		log.Err(err).Msgf(msg.UnableToFetchFile, saucereport.FileName)
+		return false
 	}
 
 	if err := r.Project.FilterFailedTests(opt.Name, report); err != nil {
 		log.Err(err).Msg(msg.UnableToFilterFailedTests)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+		return false
+	}
+
+	tempDir, err := os.MkdirTemp(os.TempDir(), "saucectl-app-payload-")
+	if err != nil {
+		log.Err(err).Msg(msg.UnableToCreateRunnerConfig)
+		return false
 	}
+	defer os.RemoveAll(tempDir)
 
 	runnerFile, err := zip.ArchiveRunnerConfig(r.Project, tempDir)
 	if err != nil {
 		log.Err(err).Msg(msg.UnableToArchiveRunnerConfig)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+		return false
 	}
 
-	fileURL, err := r.uploadConfig(runnerFile)
+	storageID, err := r.uploadConfig(runnerFile)
 	if err != nil {
 		log.Err(err).Msgf(msg.UnableToUploadConfig, runnerFile)
-		log.Info().Msg(msg.SkippingSmartRetries)
-		jobOpts <- opt
-		return
+		return false
 	}
 
 	if len(opt.OtherApps) == 0 {
-		opt.OtherApps = []string{fmt.Sprintf("storage:%s", fileURL)}
+		opt.OtherApps = []string{fmt.Sprintf("storage:%s", storageID)}
 	} else {
-		opt.OtherApps[0] = fmt.Sprintf("storage:%s", fileURL)
+		// FIXME(AlexP): Code smell! The order of elements in OtherApps is
+		// defined by CloudRunner. While the order itself is not important, the
+		// type of app is. We should not rely on the order of elements in the
+		// slice. If we need to know the type, we should use a map.
+		opt.OtherApps[0] = fmt.Sprintf("storage:%s", storageID)
 	}
-	log.Info().Str("suite", opt.DisplayName).
-		Str("attempt", fmt.Sprintf("%d of %d", opt.Attempt+1, opt.Retries+1)).
-		Msg("Retrying suite.")
-	jobOpts <- opt
+
+	return true
 }
 
 func (r *SauceReportRetrier) uploadConfig(filename string) (string, error) {
@@ -109,7 +109,7 @@ func (r *SauceReportRetrier) uploadConfig(filename string) (string, error) {
 }
 
 func (r *SauceReportRetrier) getSauceReport(job job.Job) (saucereport.SauceReport, error) {
-	content, err := r.VDCReader.GetJobAssetFileContent(context.Background(), job.ID, saucereport.SauceReportFileName, false)
+	content, err := r.VDCReader.GetJobAssetFileContent(context.Background(), job.ID, saucereport.FileName, false)
 	if err != nil {
 		return saucereport.SauceReport{}, err
 	}

internal/saucereport/saucereport.go

@@ -5,8 +5,8 @@ import (
 	"time"
 )
 
-// SauceReportFileName is the name for Sauce Labs report.
-const SauceReportFileName = "sauce-test-report.json"
+// FileName is the name for Sauce Labs report.
+const FileName = "sauce-test-report.json"
 
 // The different states that a job can be in.
 const (

internal/testcafe/config.go

@@ -429,8 +429,9 @@ func (p *Project) FilterFailedTests(suiteName string, report saucereport.SauceRe
 		if s.Name != suiteName {
 			continue
 		}
-		found = true
 		p.Suites[i].Filter.TestGrep = strings.Join(failedTests, "|")
+		found = true
+		break
 	}
 	if !found {
 		return fmt.Errorf("suite(%s) not found", suiteName)