Improve smart retry for mobile (#784)

* Improve smart retry for mobile * fix build * refine * wording * Update internal/saucecloud/retry/junitretrier.go Co-authored-by: Alex Plischke <alex.plischke@saucelabs.com> * fix indent and add comments * improve functions * refine * fix typo * add comments for getting failed tests * oops * wording * wording * fix typo --------- Co-authored-by: Alex Plischke <alex.plischke@saucelabs.com>
saucelabs · May 25, 2023 · 823f89d · 823f89d
1 parent 5877bc6
commit 823f89d
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 62 deletions.
diff --git a/internal/espresso/config.go b/internal/espresso/config.go
@@ -51,21 +51,6 @@ type Espresso struct {
 	OtherApps          []string `yaml:"otherApps,omitempty" json:"otherApps"`
 }
 
-// TestOptions represents the espresso test filter options configuration.
-type TestOptions struct {
-	NotClass            []string `yaml:"notClass,omitempty" json:"notClass"`
-	Class               []string `yaml:"class,omitempty" json:"class"`
-	Package             string   `yaml:"package,omitempty" json:"package"`
-	NotPackage          string   `yaml:"notPackage,omitempty" json:"notPackage"`
-	Size                string   `yaml:"size,omitempty" json:"size"`
-	Annotation          string   `yaml:"annotation,omitempty" json:"annotation"`
-	NotAnnotation       string   `yaml:"notAnnotation,omitempty" json:"notAnnotation"`
-	ShardIndex          int      `json:"shardIndex"`
-	NumShards           int      `yaml:"numShards,omitempty" json:"numShards"`
-	ClearPackageData    bool     `yaml:"clearPackageData,omitempty" json:"clearPackageData"`
-	UseTestOrchestrator bool     `yaml:"useTestOrchestrator,omitempty" json:"useTestOrchestrator"`
-}
-
 // Suite represents the espresso test suite configuration.
 type Suite struct {
 	Name               string                 `yaml:"name,omitempty" json:"name"`

diff --git a/internal/junit/junit.go b/internal/junit/junit.go
@@ -1,6 +1,9 @@
 package junit
 
-import "encoding/xml"
+import (
+	"encoding/xml"
+	"fmt"
+)
 
 // JunitFileName is the name of the JUnit report.
 const JunitFileName = "junit.xml"
@@ -72,3 +75,52 @@ func Parse(data []byte) (TestSuites, error) {
 
 	return tss, err
 }
+
+// GetFailedTests get failed test list from testcase list.
+func GetFailedTests(testCases []TestCase) []string {
+	classes := map[string]bool{}
+	for _, tc := range testCases {
+		if tc.Error != "" || tc.Failure != "" {
+			// The format of the filtered test is "<className>/<testMethodName>".
+			// Fallback to <className> if the test method name is unexpectedly empty.
+			// tc.Name: <testMethodName>
+			// tc.ClassName: <className>
+			if tc.Name != "" {
+				classes[fmt.Sprintf("%s/%s", tc.ClassName, tc.Name)] = true
+			} else {
+				classes[tc.ClassName] = true
+			}
+		}
+	}
+	return getKeysFromMap(classes)
+}
+
+// GetFailedClasses get failed class list from testcase list
+func GetFailedClasses(testCases []TestCase) []string {
+	classes := map[string]bool{}
+	for _, tc := range testCases {
+		if tc.Error != "" || tc.Failure != "" {
+			classes[tc.ClassName] = true
+		}
+	}
+	return getKeysFromMap(classes)
+}
+
+// CollectTestCases collects testcases from a report.
+func CollectTestCases(testsuites TestSuites) []TestCase {
+	var tc []TestCase
+	for _, s := range testsuites.TestSuites {
+		tc = append(tc, s.TestCases...)
+	}
+	return tc
+}
+
+func getKeysFromMap(mp map[string]bool) []string {
+	var keys = make([]string, len(mp))
+	var i int
+	for k := range mp {
+		keys[i] = k
+		i++
+	}
+	return keys
+}
diff --git a/internal/msg/errormsg.go b/internal/msg/errormsg.go
@@ -187,7 +187,7 @@ const (
 	// SkippingSmartRetries indicates that the full suite will be run
 	SkippingSmartRetries = "Skipping SmartRetry, retrying complete suite"
 	// RetryWithClasses indicates which classes will be run
-	RetryWithClasses = "Retrying with only classes: %s"
+	RetryWithClasses = "Retrying with failed classes or tests: %q"
 	// UnableToCreateRunnerConfig indicates a failure to create runner config file
 	UnableToCreateRunnerConfig = "Unable to create runner config file"
 	// UnableToFilterFailedTests indicates a failure to filter failed tests

diff --git a/internal/saucecloud/retry/junitretrier.go b/internal/saucecloud/retry/junitretrier.go
@@ -3,7 +3,6 @@ package retry
 import (
 	"context"
 	"fmt"
-	"strings"
 
 	"github.com/rs/zerolog/log"
 	"github.com/saucelabs/saucectl/internal/job"
@@ -17,80 +16,57 @@ type JunitRetrier struct {
 	VDCReader job.Reader
 }
 
-func getKeysFromMap(mp map[string]bool) []string {
-	keys := make([]string, len(mp))
-
-	i := 0
-	for k := range mp {
-		keys[i] = k
-		i++
-	}
-	return keys
-}
-
-func getFailedClasses(report junit.TestSuites) []string {
-	classes := map[string]bool{}
-
-	for _, s := range report.TestSuites {
-		for _, tc := range s.TestCases {
-			if tc.Error != "" || tc.Failure != "" {
-				classes[tc.ClassName] = true
-			}
-		}
-	}
-	return getKeysFromMap(classes)
-}
-
-func (b *JunitRetrier) retryOnlyFailedClasses(reader job.Reader, jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
+func (b *JunitRetrier) retryFailedTests(reader job.Reader, jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
 	content, err := reader.GetJobAssetFileContent(context.Background(), previous.ID, junit.JunitFileName, previous.IsRDC)
 	if err != nil {
-		log.Debug().Err(err).Msgf(msg.UnableToFetchFile, junit.JunitFileName)
+		log.Err(err).Msgf(msg.UnableToFetchFile, junit.JunitFileName)
 		log.Info().Msg(msg.SkippingSmartRetries)
 		jobOpts <- opt
 		return
 	}
 	suites, err := junit.Parse(content)
 	if err != nil {
-		log.Debug().Err(err).Msgf(msg.UnableToUnmarshallFile, junit.JunitFileName)
+		log.Err(err).Msgf(msg.UnableToUnmarshallFile, junit.JunitFileName)
 		log.Info().Msg(msg.SkippingSmartRetries)
 		jobOpts <- opt
 		return
 	}
 
-	classes := getFailedClasses(suites)
-	log.Info().
-		Str("suite", opt.DisplayName).
-		Str("attempt", fmt.Sprintf("%d of %d", opt.Attempt+1, opt.Retries+1)).
-		Msgf(msg.RetryWithClasses, strings.Join(classes, ","))
-
-	setClassesToRetry(&opt, classes)
-
+	setClassesToRetry(&opt, junit.CollectTestCases(suites))
 	jobOpts <- opt
 }
 
 // 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 the
 // framework that is being executed.
-func setClassesToRetry(opt *job.StartOptions, classes []string) {
+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))
+
 	if opt.Framework == xcuitest.Kind {
-		opt.TestsToRun = classes
+		opt.TestsToRun = junit.GetFailedTests(testcases)
+		lg.Msgf(msg.RetryWithClasses, opt.TestsToRun)
 		return
 	}
+
 	if opt.TestOptions == nil {
 		opt.TestOptions = map[string]interface{}{}
 	}
+	classes := junit.GetFailedClasses(testcases)
 	opt.TestOptions["class"] = classes
+	lg.Msgf(msg.RetryWithClasses, classes)
 }
 
 func (b *JunitRetrier) Retry(jobOpts chan<- job.StartOptions, opt job.StartOptions, previous job.Job) {
 	if b.RDCReader != nil && previous.IsRDC && opt.SmartRetry.FailedOnly {
-		b.retryOnlyFailedClasses(b.RDCReader, jobOpts, opt, previous)
+		b.retryFailedTests(b.RDCReader, jobOpts, opt, previous)
 		return
 	}
 
 	if b.VDCReader != nil && !previous.IsRDC && opt.SmartRetry.FailedOnly {
-		b.retryOnlyFailedClasses(b.VDCReader, jobOpts, opt, previous)
+		b.retryFailedTests(b.VDCReader, jobOpts, opt, previous)
 		return
 	}
 

diff --git a/internal/saucecloud/retry/junitretrier_test.go b/internal/saucecloud/retry/junitretrier_test.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"testing"
 
+	"github.com/saucelabs/saucectl/internal/espresso"
 	"github.com/saucelabs/saucectl/internal/job"
 	"github.com/saucelabs/saucectl/internal/junit"
 	"github.com/saucelabs/saucectl/internal/mocks"
@@ -76,7 +77,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 			},
 		},
 		{
-			name: "Job retrying only failed suites if RDC + SmartRetry",
+			name: "Espresso job retrying only failed classes if RDC + SmartRetry",
 			init: init{
 				RDCReader: &mocks.FakeJobReader{
 					ReadJobFn:              nil,
@@ -94,6 +95,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 			args: args{
 				jobOpts: make(chan job.StartOptions),
 				opt: job.StartOptions{
+					Framework:   espresso.Kind,
 					DisplayName: "Dummy Test",
 					SmartRetry: job.SmartRetry{
 						FailedOnly: true,
@@ -108,6 +110,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 				},
 			},
 			expected: job.StartOptions{
+				Framework:   espresso.Kind,
 				DisplayName: "Dummy Test",
 				TestOptions: map[string]interface{}{
 					"class": []string{"Demo.Class1"},
@@ -118,7 +121,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 			},
 		},
 		{
-			name: "Job retrying only failed suites if RDC + SmartRetry with no orig filters",
+			name: "Espresso job retrying only failed classes if RDC + SmartRetry with no orig filters",
 			init: init{
 				RDCReader: &mocks.FakeJobReader{
 					ReadJobFn:              nil,
@@ -136,6 +139,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 			args: args{
 				jobOpts: make(chan job.StartOptions),
 				opt: job.StartOptions{
+					Framework:   espresso.Kind,
 					DisplayName: "Dummy Test",
 					SmartRetry: job.SmartRetry{
 						FailedOnly: true,
@@ -147,6 +151,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 				},
 			},
 			expected: job.StartOptions{
+				Framework:   espresso.Kind,
 				DisplayName: "Dummy Test",
 				TestOptions: map[string]interface{}{
 					"class": []string{"Demo.Class1"},
@@ -157,15 +162,15 @@ func TestAppsRetrier_Retry(t *testing.T) {
 			},
 		},
 		{
-			name: "XCUITest: Job retrying only failed suites if RDC + SmartRetry with no orig filters",
+			name: "XCUITest: Job retrying only failed tests if RDC + SmartRetry with no orig filters",
 			init: init{
 				RDCReader: &mocks.FakeJobReader{
 					ReadJobFn:              nil,
 					PollJobFn:              nil,
 					GetJobAssetFileNamesFn: nil,
 					GetJobAssetFileContentFn: func(ctx context.Context, jobID, fileName string) ([]byte, error) {
 						if jobID == "fake-job-id" && fileName == junit.JunitFileName {
-							return []byte("<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"yes\"?>\n<testsuite>\n    <testcase classname=\"Demo.Class1\">\n        <failure>ERROR</failure>\n    </testcase>\n    <testcase classname=\"Demo.Class1\"/>\n    <testcase classname=\"Demo.Class2\"/>\n    <testcase classname=\"Demo.Class3\"/>\n</testsuite>\n"), nil
+							return []byte("<?xml version=\"1.0\" encoding=\"UTF-8\" standalone=\"yes\"?>\n<testsuite>\n    <testcase name=\"demoTest\" classname=\"Demo.Class1\">\n        <failure>ERROR</failure>\n    </testcase>\n    <testcase classname=\"Demo.Class1\"/>\n    <testcase classname=\"Demo.Class2\"/>\n    <testcase classname=\"Demo.Class3\"/>\n</testsuite>\n"), nil
 						}
 						return []byte{}, errors.New("unknown file")
 					},
@@ -189,7 +194,7 @@ func TestAppsRetrier_Retry(t *testing.T) {
 			expected: job.StartOptions{
 				Framework:   xcuitest.Kind,
 				DisplayName: "Dummy Test",
-				TestsToRun:  []string{"Demo.Class1"},
+				TestsToRun:  []string{"Demo.Class1/demoTest"},
 				SmartRetry: job.SmartRetry{
 					FailedOnly: true,
 				},