From 3e0f9026f3ddb697b9e4e82b0e770fbbe0e966b7 Mon Sep 17 00:00:00 2001 From: Viacheslav Poturaev Date: Thu, 13 Jul 2023 09:31:56 +0200 Subject: [PATCH] Improve hooks invocation flow (#568) --- CHANGELOG.md | 5 +- suite.go | 107 ++++++++++++----- suite_context_test.go | 262 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 345 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c99edcc..92dd482 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,10 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt ### Added - Support for reading feature files from an `fs.FS` ([550](https://github.com/cucumber/godog/pull/550) - [tigh-latte](https://github.com/tigh-latte)) - Added keyword functions. ([509](https://github.com/cucumber/godog/pull/509) - [otrava7](https://github.com/otrava7)) -- prefer go test to use of godog cli in README ([548](https://github.com/cucumber/godog/pull/548) - [danielhelfand](https://github.com/danielhelfand) +- Prefer go test to use of godog cli in README ([548](https://github.com/cucumber/godog/pull/548) - [danielhelfand](https://github.com/danielhelfand)) + +### Fixed +- Improve hooks invocation flow ([568](https://github.com/cucumber/godog/pull/568) - [vearutop](https://github.com/vearutop)) ## [v0.12.6] ### Changed diff --git a/suite.go b/suite.go index 8834365..16bfd15 100644 --- a/suite.go +++ b/suite.go @@ -2,6 +2,7 @@ package godog import ( "context" + "errors" "fmt" "reflect" "strings" @@ -27,6 +28,9 @@ var ErrUndefined = fmt.Errorf("step is undefined") // step implementation is pending var ErrPending = fmt.Errorf("step implementation is pending") +// ErrSkip should be returned by step definition or a hook if scenario and further steps are to be skipped. +var ErrSkip = fmt.Errorf("skipped") + // StepResultStatus describes step result. type StepResultStatus = models.StepResultStatus @@ -72,34 +76,53 @@ func (s *suite) matchStep(step *messages.PickleStep) *models.StepDefinition { return def } -func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, prevStepErr error, isFirst, isLast bool) (rctx context.Context, err error) { +func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scenarioErr error, isFirst, isLast bool) (rctx context.Context, err error) { var ( match *models.StepDefinition - sr = models.PickleStepResult{Status: models.Undefined} + sr = models.NewStepResult(pickle.Id, step.Id, match) ) rctx = ctx + sr.Status = StepUndefined // user multistep definitions may panic defer func() { if e := recover(); e != nil { - err = &traceError{ - msg: fmt.Sprintf("%v", e), - stack: callStack(), + if err != nil { + err = &traceError{ + msg: fmt.Sprintf("%s: %v", err.Error(), e), + stack: callStack(), + } + } else { + err = &traceError{ + msg: fmt.Sprintf("%v", e), + stack: callStack(), + } } } - earlyReturn := prevStepErr != nil || err == ErrUndefined + earlyReturn := scenarioErr != nil || err == ErrUndefined - if !earlyReturn { - sr = models.NewStepResult(pickle.Id, step.Id, match) + switch { + case errors.Is(err, ErrPending): + sr.Status = StepPending + case errors.Is(err, ErrSkip) || (err == nil && scenarioErr != nil): + sr.Status = StepSkipped + case errors.Is(err, ErrUndefined): + sr.Status = StepUndefined + case err != nil: + sr.Status = StepFailed + case err == nil && scenarioErr == nil: + sr.Status = StepPassed } // Run after step handlers. rctx, err = s.runAfterStepHooks(ctx, step, sr.Status, err) + shouldFail := s.shouldFail(err) + // Trigger after scenario on failing or last step to attach possible hook error to step. - if isLast || (sr.Status != StepSkipped && sr.Status != StepUndefined && err != nil) { + if !s.shouldFail(scenarioErr) && (isLast || shouldFail) { rctx, err = s.runAfterScenarioHooks(rctx, pickle, err) } @@ -137,6 +160,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, prevS match = s.matchStep(step) s.storage.MustInsertStepDefintionMatch(step.AstNodeIds[0], match) + sr.Def = match s.fmt.Defined(pickle, step, match.GetInternalStepDefinition()) if err != nil { @@ -162,6 +186,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, prevS Nested: match.Nested, Undefined: undef, } + sr.Def = match } sr = models.NewStepResult(pickle.Id, step.Id, match) @@ -172,7 +197,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, prevS return ctx, ErrUndefined } - if prevStepErr != nil { + if scenarioErr != nil { sr = models.NewStepResult(pickle.Id, step.Id, match) sr.Status = models.Skipped s.storage.MustInsertPickleStepResult(sr) @@ -344,7 +369,7 @@ func (s *suite) maybeSubSteps(ctx context.Context, result interface{}) (context. steps, ok := result.(Steps) if !ok { - return ctx, fmt.Errorf("unexpected error, should have been []string: %T - %+v", result, result) + return ctx, fmt.Errorf("unexpected error, should have been godog.Steps: %T - %+v", result, result) } var err error @@ -352,13 +377,48 @@ func (s *suite) maybeSubSteps(ctx context.Context, result interface{}) (context. for _, text := range steps { if def := s.matchStepTextAndType(text, messages.PickleStepType_UNKNOWN); def == nil { return ctx, ErrUndefined - } else if ctx, err = s.maybeSubSteps(def.Run(ctx)); err != nil { - return ctx, fmt.Errorf("%s: %+v", text, err) + } else { + ctx, err = s.runSubStep(ctx, text, def) + if err != nil { + return ctx, err + } } } return ctx, nil } +func (s *suite) runSubStep(ctx context.Context, text string, def *models.StepDefinition) (_ context.Context, err error) { + st := &Step{} + st.Text = text + st.Type = messages.PickleStepType_ACTION + + defer func() { + status := StepPassed + + switch { + case errors.Is(err, ErrUndefined): + status = StepUndefined + case errors.Is(err, ErrPending): + status = StepPending + case err != nil: + status = StepFailed + } + + ctx, err = s.runAfterStepHooks(ctx, st, status, err) + }() + + ctx, err = s.runBeforeStepHooks(ctx, st, nil) + if err != nil { + return ctx, fmt.Errorf("%s: %+v", text, err) + } + + if ctx, err = s.maybeSubSteps(def.Run(ctx)); err != nil { + return ctx, fmt.Errorf("%s: %+v", text, err) + } + + return ctx, nil +} + func (s *suite) matchStepTextAndType(text string, stepType messages.PickleStepType) *models.StepDefinition { for _, h := range s.steps { if m := h.Expr.FindStringSubmatch(text); len(m) > 0 { @@ -405,32 +465,23 @@ func keywordMatches(k formatters.Keyword, stepType messages.PickleStepType) bool func (s *suite) runSteps(ctx context.Context, pickle *Scenario, steps []*Step) (context.Context, error) { var ( - stepErr, err error + stepErr, scenarioErr error ) for i, step := range steps { isLast := i == len(steps)-1 isFirst := i == 0 - ctx, stepErr = s.runStep(ctx, pickle, step, err, isFirst, isLast) - switch stepErr { - case ErrUndefined: - // do not overwrite failed error - if err == ErrUndefined || err == nil { - err = stepErr - } - case ErrPending: - err = stepErr - case nil: - default: - err = stepErr + ctx, stepErr = s.runStep(ctx, pickle, step, scenarioErr, isFirst, isLast) + if scenarioErr == nil || s.shouldFail(stepErr) { + scenarioErr = stepErr } } - return ctx, err + return ctx, scenarioErr } func (s *suite) shouldFail(err error) bool { - if err == nil { + if err == nil || err == ErrSkip { return false } diff --git a/suite_context_test.go b/suite_context_test.go index b6efbe6..6df223b 100644 --- a/suite_context_test.go +++ b/suite_context_test.go @@ -469,6 +469,10 @@ func (tc *godogFeaturesScenario) iAmListeningToSuiteEvents() error { scenarioContext.Before(func(ctx context.Context, pickle *Scenario) (context.Context, error) { tc.events = append(tc.events, &firedEvent{"BeforeScenario", []interface{}{pickle}}) + if ctx.Value(ctxKey("BeforeScenario")) != nil { + return ctx, errors.New("unexpected BeforeScenario in context (double invocation)") + } + return context.WithValue(ctx, ctxKey("BeforeScenario"), pickle.Name), nil }) @@ -837,3 +841,261 @@ Feature: dummy assert.Fail(t, "failed to wait for context cancellation") } } + +func TestTestSuite_Run(t *testing.T) { + for _, tc := range []struct { + name string + body string + afterStepCnt int + beforeStepCnt int + log string + noStrict bool + suitePasses bool + }{ + { + name: "fail_then_pass_fails_scenario", afterStepCnt: 2, beforeStepCnt: 2, + body: ` + When step fails + Then step passes`, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step fails" + After step "step fails", error: oops, status: failed + << After scenario "test", error: oops + Before step "step passes" + After step "step passes", error: , status: skipped + <<<< After suite`, + }, + { + name: "pending_then_pass_fails_scenario", afterStepCnt: 2, beforeStepCnt: 2, + body: ` + When step is pending + Then step passes`, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step is pending" + After step "step is pending", error: step implementation is pending, status: pending + << After scenario "test", error: step implementation is pending + Before step "step passes" + After step "step passes", error: , status: skipped + <<<< After suite`, + }, + { + name: "pending_then_pass_no_strict_doesnt_fail_scenario", afterStepCnt: 2, beforeStepCnt: 2, noStrict: true, suitePasses: true, + body: ` + When step is pending + Then step passes`, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step is pending" + After step "step is pending", error: step implementation is pending, status: pending + Before step "step passes" + After step "step passes", error: , status: skipped + << After scenario "test", error: + <<<< After suite`, + }, + { + name: "undefined_then_pass_no_strict_doesnt_fail_scenario", afterStepCnt: 2, beforeStepCnt: 2, noStrict: true, suitePasses: true, + body: ` + When step is undefined + Then step passes`, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step is undefined" + After step "step is undefined", error: step is undefined, status: undefined + Before step "step passes" + After step "step passes", error: , status: skipped + << After scenario "test", error: + <<<< After suite`, + }, + { + name: "undefined_then_pass_fails_scenario", afterStepCnt: 2, beforeStepCnt: 2, + body: ` + When step is undefined + Then step passes`, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step is undefined" + After step "step is undefined", error: step is undefined, status: undefined + << After scenario "test", error: step is undefined + Before step "step passes" + After step "step passes", error: , status: skipped + <<<< After suite`, + }, + { + name: "fail_then_undefined_fails_scenario", afterStepCnt: 2, beforeStepCnt: 2, + body: ` + When step fails + Then step is undefined`, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step fails" + After step "step fails", error: oops, status: failed + << After scenario "test", error: oops + Before step "step is undefined" + After step "step is undefined", error: step is undefined, status: undefined + <<<< After suite`, + }, + { + name: "passes", afterStepCnt: 2, beforeStepCnt: 2, + body: ` + When step passes + Then step passes`, + suitePasses: true, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step passes" + + After step "step passes", error: , status: passed + Before step "step passes" + + After step "step passes", error: , status: passed + << After scenario "test", error: + <<<< After suite`, + }, + { + name: "skip_does_not_fail_scenario", afterStepCnt: 2, beforeStepCnt: 2, + body: ` + When step skips scenario + Then step fails`, + suitePasses: true, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "step skips scenario" + After step "step skips scenario", error: skipped, status: skipped + Before step "step fails" + After step "step fails", error: , status: skipped + << After scenario "test", error: + <<<< After suite`, + }, + { + name: "multistep_passes", afterStepCnt: 6, beforeStepCnt: 6, + body: ` + When multistep passes + Then multistep passes`, + suitePasses: true, + log: ` + >>>> Before suite + >> Before scenario "test" + Before step "multistep passes" + Before step "step passes" + + After step "step passes", error: , status: passed + Before step "step passes" + + After step "step passes", error: , status: passed + After step "multistep passes", error: , status: passed + Before step "multistep passes" + Before step "step passes" + + After step "step passes", error: , status: passed + Before step "step passes" + + After step "step passes", error: , status: passed + After step "multistep passes", error: , status: passed + << After scenario "test", error: + <<<< After suite`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + afterScenarioCnt := 0 + beforeScenarioCnt := 0 + + afterStepCnt := 0 + beforeStepCnt := 0 + + var log string + + suite := TestSuite{ + TestSuiteInitializer: func(suiteContext *TestSuiteContext) { + suiteContext.BeforeSuite(func() { + log += fmt.Sprintln(">>>> Before suite") + }) + + suiteContext.AfterSuite(func() { + log += fmt.Sprintln("<<<< After suite") + }) + }, + ScenarioInitializer: func(s *ScenarioContext) { + s.Before(func(ctx context.Context, sc *Scenario) (context.Context, error) { + log += fmt.Sprintf(">> Before scenario %q\n", sc.Name) + beforeScenarioCnt++ + + return ctx, nil + }) + + s.After(func(ctx context.Context, sc *Scenario, err error) (context.Context, error) { + log += fmt.Sprintf("<< After scenario %q, error: %v\n", sc.Name, err) + afterScenarioCnt++ + + return ctx, nil + }) + + s.StepContext().Before(func(ctx context.Context, st *Step) (context.Context, error) { + log += fmt.Sprintf("Before step %q\n", st.Text) + beforeStepCnt++ + + return ctx, nil + }) + + s.StepContext().After(func(ctx context.Context, st *Step, status StepResultStatus, err error) (context.Context, error) { + log += fmt.Sprintf("After step %q, error: %v, status: %s\n", st.Text, err, status.String()) + afterStepCnt++ + + return ctx, nil + }) + + s.Step("^step fails$", func() error { + return errors.New("oops") + }) + + s.Step("^step skips scenario$", func() error { + return ErrSkip + }) + + s.Step("^step passes$", func() { + log += "\n" + }) + + s.Step("^multistep passes$", func() Steps { + return Steps{"step passes", "step passes"} + }) + + s.Step("pending", func() error { + return ErrPending + }) + }, + Options: &Options{ + Format: "pretty", + Strict: !tc.noStrict, + NoColors: true, + FeatureContents: []Feature{ + { + Name: tc.name, + Contents: []byte(trimAllLines(` + Feature: test + Scenario: test + ` + tc.body)), + }, + }, + }, + } + + suitePasses := suite.Run() == 0 + assert.Equal(t, tc.suitePasses, suitePasses) + assert.Equal(t, 1, afterScenarioCnt) + assert.Equal(t, 1, beforeScenarioCnt) + assert.Equal(t, tc.afterStepCnt, afterStepCnt) + assert.Equal(t, tc.beforeStepCnt, beforeStepCnt) + assert.Equal(t, trimAllLines(tc.log), trimAllLines(log), log) + }) + } +}