Skip to content

Commit

Permalink
fix(#433): Optimize failure() script condition
Browse files Browse the repository at this point in the history 
Make sure to run the pre/post script steps that use condition failure() only on those tests that actually have a failed test result instead of evaluating the failure state for the whole test suite.
  • Loading branch information
@christophd
christophd committed Aug 25, 2022
1 parent f096386 commit 5e85adf
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 22 deletions.
26 changes: 12 additions & 14 deletions pkg/cmd/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,8 @@ func (o *runCmdOptions) runTest(cmd *cobra.Command, source string, results *v1al
handleError := func(err error) {
handleTestError(runConfig.Config.Namespace.Name, source, results, err)
}
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError)
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError) {
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError)
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError) {
return
}

Expand Down Expand Up @@ -254,8 +254,8 @@ func (o *runCmdOptions) runTestGroup(cmd *cobra.Command, source string, results
handleError := func(err error) {
handleTestError(runConfig.Config.Namespace.Name, source, results, err)
}
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError)
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError) {
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError)
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError) {
return
}

Expand Down Expand Up @@ -352,7 +352,7 @@ func (o *runCmdOptions) createTempNamespace(runConfig *config.RunConfig, cmd *co
instance, err = v1alpha1.FindGlobalInstance(o.Context, c)

if err != nil && k8serrors.IsForbidden(err) {
// not allowed to list all instances on the clusterr
// not allowed to list all instances on the cluster
return namespace, nil
} else if err != nil {
return namespace, err
Expand Down Expand Up @@ -548,9 +548,7 @@ func (o *runCmdOptions) createAndRunTest(ctx context.Context, c client.Client, c
}

if runConfig.Config.Dump.Enabled {
if runConfig.Config.Dump.FailedOnly &&
test.Status.Phase != v1alpha1.TestPhaseFailed && test.Status.Phase != v1alpha1.TestPhaseError &&
len(test.Status.Errors) == 0 && !hasSuiteErrors(&test.Status.Results) {
if runConfig.Config.Dump.FailedOnly && !isFailed(&test) {
fmt.Println("Skip dump for successful test")
} else {
var fileName string
Expand Down Expand Up @@ -766,13 +764,13 @@ func (o *runCmdOptions) newSettings(ctx context.Context, runConfig *config.RunCo
return nil, nil
}

func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1alpha1.TestResults, handleError func(err error)) bool {
func runSteps(steps []config.StepConfig, namespace, baseDir string, name string, results *v1alpha1.TestResults, handleError func(err error)) bool {
for idx, step := range steps {
if len(step.Name) == 0 {
step.Name = fmt.Sprintf("step-%d", idx)
}

if skipStep(step, results) {
if skipStep(step, name, results) {
fmt.Printf("Skip %s\n", step.Name)
continue
}
Expand All @@ -782,7 +780,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
if desc == "" {
desc = fmt.Sprintf("script %s", step.Script)
}
if err := runScript(step.Script, desc, namespace, baseDir, hasErrors(results), step.Timeout); err != nil {
if err := runScript(step.Script, desc, namespace, baseDir, hasError(name, results), step.Timeout); err != nil {
handleError(fmt.Errorf(fmt.Sprintf("Failed to run %s: %v", desc, err)))
return false
}
Expand Down Expand Up @@ -824,7 +822,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
if desc == "" {
desc = fmt.Sprintf("inline command %d", idx)
}
if err := runScript(file.Name(), desc, namespace, baseDir, hasErrors(results), step.Timeout); err != nil {
if err := runScript(file.Name(), desc, namespace, baseDir, hasError(name, results), step.Timeout); err != nil {
handleError(fmt.Errorf(fmt.Sprintf("Failed to run %s: %v", desc, err)))
return false
}
Expand All @@ -834,7 +832,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
return true
}

func skipStep(step config.StepConfig, results *v1alpha1.TestResults) bool {
func skipStep(step config.StepConfig, name string, results *v1alpha1.TestResults) bool {
if step.If == "" {
return false
}
Expand Down Expand Up @@ -867,7 +865,7 @@ func skipStep(step config.StepConfig, results *v1alpha1.TestResults) bool {
case "os":
skipStep = (keyValue)[1] != r.GOOS
case "failure()":
skipStep = !hasErrors(results)
skipStep = !hasError(name, results)
}

if skipStep {
Expand Down
9 changes: 5 additions & 4 deletions pkg/cmd/run_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ func TestStepOsCheck(t *testing.T) {
saveErr := func(err error) {
scriptError = err
}
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)

assert.NilError(t, scriptError)
}
Expand Down Expand Up @@ -81,7 +81,7 @@ func TestStepEnvCheck(t *testing.T) {
saveErr := func(err error) {
scriptError = err
}
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)

assert.NilError(t, scriptError)
}
Expand Down Expand Up @@ -117,7 +117,7 @@ func TestStepCheckCombinations(t *testing.T) {
saveErr := func(err error) {
scriptError = err
}
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)

assert.NilError(t, scriptError)
}
Expand Down Expand Up @@ -146,7 +146,8 @@ func TestStepOnFailure(t *testing.T) {
saveErr := func(err error) {
scriptError = err
}
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)

runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)

assert.NilError(t, scriptError)
}
30 changes: 26 additions & 4 deletions pkg/cmd/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (

"github.com/citrusframework/yaks/pkg/apis/yaks/v1alpha1"
"github.com/citrusframework/yaks/pkg/cmd/config"
"github.com/citrusframework/yaks/pkg/util/kubernetes"
p "github.com/gertd/go-pluralize"
"github.com/mitchellh/mapstructure"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -230,8 +231,18 @@ func resolvePath(runConfig *config.RunConfig, resource string) string {
}

func hasErrors(results *v1alpha1.TestResults) bool {
for i := range results.Suites {
if hasSuiteErrors(&results.Suites[i]) {
for _, suite := range results.Suites {
if hasSuiteErrors(&suite) {
return true
}
}

return false
}

func hasError(name string, results *v1alpha1.TestResults) bool {
for _, suite := range results.Suites {
if hasTestError(name, &suite) {
return true
}
}
Expand All @@ -240,13 +251,24 @@ func hasErrors(results *v1alpha1.TestResults) bool {
}

func hasSuiteErrors(suite *v1alpha1.TestSuite) bool {
if len(suite.Errors) > 0 || suite.Summary.Errors > 0 || suite.Summary.Failed > 0 {
return true
return len(suite.Errors) > 0 || suite.Summary.Errors > 0 || suite.Summary.Failed > 0
}

func hasTestError(name string, suite *v1alpha1.TestSuite) bool {
for _, test := range suite.Tests {
if test.Name == kubernetes.SanitizeName(name) {
return test.ErrorType != "" || test.ErrorMessage != ""
}
}

return false
}

func isFailed(test *v1alpha1.Test) bool {
return test.Status.Phase == v1alpha1.TestPhaseFailed ||
test.Status.Phase == v1alpha1.TestPhaseError && len(test.Status.Errors) > 0
}

func loadData(ctx context.Context, fileName string) (string, error) {
var content []byte
var err error
Expand Down

0 comments on commit 5e85adf

Please sign in to comment.