diff --git a/internal/cucumber/config.go b/internal/cucumber/config.go index 65f00016e..70bc57883 100644 --- a/internal/cucumber/config.go +++ b/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)) } diff --git a/internal/cypress/v1/config.go b/internal/cypress/v1/config.go index a61498f6e..e10ff1df0 100644 --- a/internal/cypress/v1/config.go +++ b/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) diff --git a/internal/msg/errormsg.go b/internal/msg/errormsg.go index 796f0c62a..4f1c035f2 100644 --- a/internal/msg/errormsg.go +++ b/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 diff --git a/internal/playwright/config.go b/internal/playwright/config.go index 89d50b098..1846fc18c 100644 --- a/internal/playwright/config.go +++ b/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) diff --git a/internal/saucecloud/cloud.go b/internal/saucecloud/cloud.go index 0dddf7db4..b5924316e 100644 --- a/internal/saucecloud/cloud.go +++ b/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 diff --git a/internal/saucecloud/cloud_test.go b/internal/saucecloud/cloud_test.go index e54e39995..64646bd82 100644 --- a/internal/saucecloud/cloud_test.go +++ b/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('fake@email.com').should('have.value', 'fake@email.com');"," }"]},"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 '' to have value 'wrongy@email.com', but the value was 'fake@email.com'\n\n 11 | // https://on.cypress.io/type\n 12 | cy.get('.action-email')\n> 13 | .type('fake@email.com').should('have.value', 'wrongy@email.com')\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('fake@email.com').should('have.value', 'wrongy@email.com');"," }"]},"videoTimestamp":27.907}]}],"attachments":[],"tests":[]}],"metadata":{}}`), nil } return []byte{}, errors.New("not-found") diff --git a/internal/saucecloud/retry/junitretrier.go b/internal/saucecloud/retry/junitretrier.go index 51f96afdf..ad47048ee 100644 --- a/internal/saucecloud/retry/junitretrier.go +++ b/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 diff --git a/internal/saucecloud/retry/junitretrier_test.go b/internal/saucecloud/retry/junitretrier_test.go index c45465a89..48861f7e1 100644 --- a/internal/saucecloud/retry/junitretrier_test.go +++ b/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{ diff --git a/internal/saucecloud/retry/saucereportretrier.go b/internal/saucecloud/retry/saucereportretrier.go index 4dcc2af73..76c2cd742 100644 --- a/internal/saucecloud/retry/saucereportretrier.go +++ b/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 } diff --git a/internal/saucereport/saucereport.go b/internal/saucereport/saucereport.go index 866a05df2..56cffab66 100644 --- a/internal/saucereport/saucereport.go +++ b/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 ( diff --git a/internal/testcafe/config.go b/internal/testcafe/config.go index 97e4958bc..b3b99ff24 100644 --- a/internal/testcafe/config.go +++ b/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)