Skip to content

Commit

Permalink
fix(citrusframework#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 39833e1
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 24 deletions.
39 changes: 24 additions & 15 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,24 @@ 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 {
fileNames := make([]string, 0)

defer func() {
for _, fileName := range fileNames {
err := os.Remove(fileName)
if err != nil {
handleError(fmt.Errorf(fmt.Sprintf("Failed to remove temporary script file %s: %v", fileName, err)))
}
}
}()

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 +791,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 All @@ -795,7 +804,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
handleError(err)
return false
}
defer os.Remove(file.Name())
fileNames = append(fileNames, file.Name())

_, err = file.WriteString("#!/bin/bash\n\nset -e\n\n")
if err != nil {
Expand Down Expand Up @@ -824,7 +833,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 +843,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 +876,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
37 changes: 33 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,36 @@ func TestStepOnFailure(t *testing.T) {
saveErr := func(err error) {
scriptError = err
}
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)

results := v1alpha1.TestResults{
Suites: []v1alpha1.TestSuite{
{
Tests: []v1alpha1.TestResult{
{
Name: "foo",
ErrorType: "",
ErrorMessage: "",
},
},
},
},
}

runSteps(steps, "default", "", "foo", &results, saveErr)

assert.NilError(t, scriptError)

results.Suites[0].Tests = append(results.Suites[0].Tests, v1alpha1.TestResult{
Name: "bar",
ErrorType: "error",
ErrorMessage: "Something went wrong",
})

runSteps(steps, "default", "", "foo", &results, saveErr)

assert.NilError(t, scriptError)

runSteps(steps, "default", "", "bar", &results, saveErr)

assert.Equal(t, scriptError.Error(), "Failed to run failure-check-fail: exit status 127")
}
20 changes: 15 additions & 5 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,23 +231,32 @@ 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 len(suite.Errors) > 0 || suite.Summary.Errors > 0 || suite.Summary.Failed > 0 {
return true
}
}

return false
}

func hasSuiteErrors(suite *v1alpha1.TestSuite) bool {
if len(suite.Errors) > 0 || suite.Summary.Errors > 0 || suite.Summary.Failed > 0 {
return true
func hasError(name string, results *v1alpha1.TestResults) bool {
for _, suite := range results.Suites {
for _, test := range suite.Tests {
if test.Name == kubernetes.SanitizeName(name) && (test.ErrorType != "" || test.ErrorMessage != "") {
return true
}
}
}

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 39833e1

Please sign in to comment.