Skip to content

Commit 56c05a2

Browse files
christophdchristophd
committed
fix(#433): Optimize failure() script condition
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.
1 parent f096386 commit 56c05a2

File tree

3 files changed

+72
-24
lines changed

3 files changed

+72
-24
lines changed

‎pkg/cmd/run.go

+24-15
Original file line numberDiff line numberDiff line change
@@ -198,8 +198,8 @@ func (o *runCmdOptions) runTest(cmd *cobra.Command, source string, results *v1al
198198
handleError := func(err error) {
199199
handleTestError(runConfig.Config.Namespace.Name, source, results, err)
200200
}
201-
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError)
202-
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError) {
201+
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError)
202+
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError) {
203203
return
204204
}
205205

@@ -254,8 +254,8 @@ func (o *runCmdOptions) runTestGroup(cmd *cobra.Command, source string, results
254254
handleError := func(err error) {
255255
handleTestError(runConfig.Config.Namespace.Name, source, results, err)
256256
}
257-
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError)
258-
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, results, handleError) {
257+
defer runSteps(runConfig.Post, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError)
258+
if !runSteps(runConfig.Pre, runConfig.Config.Namespace.Name, runConfig.BaseDir, source, results, handleError) {
259259
return
260260
}
261261

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

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

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

769-
func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1alpha1.TestResults, handleError func(err error)) bool {
767+
func runSteps(steps []config.StepConfig, namespace, baseDir string, name string, results *v1alpha1.TestResults, handleError func(err error)) bool {
768+
fileNames := make([]string, 0)
769+
770+
defer func() {
771+
for _, fileName := range fileNames {
772+
err := os.Remove(fileName)
773+
if err != nil {
774+
handleError(fmt.Errorf(fmt.Sprintf("Failed to remove temporary script file %s: %v", fileName, err)))
775+
}
776+
}
777+
}()
778+
770779
for idx, step := range steps {
771780
if len(step.Name) == 0 {
772781
step.Name = fmt.Sprintf("step-%d", idx)
773782
}
774783

775-
if skipStep(step, results) {
784+
if skipStep(step, name, results) {
776785
fmt.Printf("Skip %s\n", step.Name)
777786
continue
778787
}
@@ -782,7 +791,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
782791
if desc == "" {
783792
desc = fmt.Sprintf("script %s", step.Script)
784793
}
785-
if err := runScript(step.Script, desc, namespace, baseDir, hasErrors(results), step.Timeout); err != nil {
794+
if err := runScript(step.Script, desc, namespace, baseDir, hasError(name, results), step.Timeout); err != nil {
786795
handleError(fmt.Errorf(fmt.Sprintf("Failed to run %s: %v", desc, err)))
787796
return false
788797
}
@@ -795,7 +804,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
795804
handleError(err)
796805
return false
797806
}
798-
defer os.Remove(file.Name())
807+
fileNames = append(fileNames, file.Name())
799808

800809
_, err = file.WriteString("#!/bin/bash\n\nset -e\n\n")
801810
if err != nil {
@@ -824,7 +833,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
824833
if desc == "" {
825834
desc = fmt.Sprintf("inline command %d", idx)
826835
}
827-
if err := runScript(file.Name(), desc, namespace, baseDir, hasErrors(results), step.Timeout); err != nil {
836+
if err := runScript(file.Name(), desc, namespace, baseDir, hasError(name, results), step.Timeout); err != nil {
828837
handleError(fmt.Errorf(fmt.Sprintf("Failed to run %s: %v", desc, err)))
829838
return false
830839
}
@@ -834,7 +843,7 @@ func runSteps(steps []config.StepConfig, namespace, baseDir string, results *v1a
834843
return true
835844
}
836845

837-
func skipStep(step config.StepConfig, results *v1alpha1.TestResults) bool {
846+
func skipStep(step config.StepConfig, name string, results *v1alpha1.TestResults) bool {
838847
if step.If == "" {
839848
return false
840849
}
@@ -867,7 +876,7 @@ func skipStep(step config.StepConfig, results *v1alpha1.TestResults) bool {
867876
case "os":
868877
skipStep = (keyValue)[1] != r.GOOS
869878
case "failure()":
870-
skipStep = !hasErrors(results)
879+
skipStep = !hasError(name, results)
871880
}
872881

873882
if skipStep {

‎pkg/cmd/run_test.go

+33-4
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func TestStepOsCheck(t *testing.T) {
4545
saveErr := func(err error) {
4646
scriptError = err
4747
}
48-
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
48+
runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)
4949

5050
assert.NilError(t, scriptError)
5151
}
@@ -81,7 +81,7 @@ func TestStepEnvCheck(t *testing.T) {
8181
saveErr := func(err error) {
8282
scriptError = err
8383
}
84-
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
84+
runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)
8585

8686
assert.NilError(t, scriptError)
8787
}
@@ -117,7 +117,7 @@ func TestStepCheckCombinations(t *testing.T) {
117117
saveErr := func(err error) {
118118
scriptError = err
119119
}
120-
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
120+
runSteps(steps, "default", "", "foo", &v1alpha1.TestResults{}, saveErr)
121121

122122
assert.NilError(t, scriptError)
123123
}
@@ -146,7 +146,36 @@ func TestStepOnFailure(t *testing.T) {
146146
saveErr := func(err error) {
147147
scriptError = err
148148
}
149-
runSteps(steps, "default", "", &v1alpha1.TestResults{}, saveErr)
149+
150+
results := v1alpha1.TestResults{
151+
Suites: []v1alpha1.TestSuite{
152+
{
153+
Tests: []v1alpha1.TestResult{
154+
{
155+
Name: "foo",
156+
ErrorType: "",
157+
ErrorMessage: "",
158+
},
159+
},
160+
},
161+
},
162+
}
163+
164+
runSteps(steps, "default", "", "foo", &results, saveErr)
165+
166+
assert.NilError(t, scriptError)
167+
168+
results.Suites[0].Tests = append(results.Suites[0].Tests, v1alpha1.TestResult{
169+
Name: "bar",
170+
ErrorType: "error",
171+
ErrorMessage: "Something went wrong",
172+
})
173+
174+
runSteps(steps, "default", "", "foo", &results, saveErr)
150175

151176
assert.NilError(t, scriptError)
177+
178+
runSteps(steps, "default", "", "bar", &results, saveErr)
179+
180+
assert.Equal(t, scriptError.Error(), "Failed to run failure-check-fail: exit status 127")
152181
}

‎pkg/cmd/util.go

+15-5
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"context"
2222
"encoding/csv"
2323
"fmt"
24+
"github.com/citrusframework/yaks/pkg/util/kubernetes"
2425
"io"
2526
"io/ioutil"
2627
"log"
@@ -230,23 +231,32 @@ func resolvePath(runConfig *config.RunConfig, resource string) string {
230231
}
231232

232233
func hasErrors(results *v1alpha1.TestResults) bool {
233-
for i := range results.Suites {
234-
if hasSuiteErrors(&results.Suites[i]) {
234+
for _, suite := range results.Suites {
235+
if len(suite.Errors) > 0 || suite.Summary.Errors > 0 || suite.Summary.Failed > 0 {
235236
return true
236237
}
237238
}
238239

239240
return false
240241
}
241242

242-
func hasSuiteErrors(suite *v1alpha1.TestSuite) bool {
243-
if len(suite.Errors) > 0 || suite.Summary.Errors > 0 || suite.Summary.Failed > 0 {
244-
return true
243+
func hasError(name string, results *v1alpha1.TestResults) bool {
244+
for _, suite := range results.Suites {
245+
for _, test := range suite.Tests {
246+
if test.Name == kubernetes.SanitizeName(name) && (test.ErrorType != "" || test.ErrorMessage != "") {
247+
return true
248+
}
249+
}
245250
}
246251

247252
return false
248253
}
249254

255+
func isFailed(test *v1alpha1.Test) bool {
256+
return test.Status.Phase == v1alpha1.TestPhaseFailed ||
257+
test.Status.Phase == v1alpha1.TestPhaseError && len(test.Status.Errors) > 0
258+
}
259+
250260
func loadData(ctx context.Context, fileName string) (string, error) {
251261
var content []byte
252262
var err error

0 commit comments

Comments
 (0)