Improve hooks invocation flow (#568)

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

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{
+			if err != nil {
-				msg:   fmt.Sprintf("%v", e),
+				err = &traceError{
-				stack: callStack(),
+					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 {
+		switch {
-			sr = models.NewStepResult(pickle.Id, step.Id, match)
+		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 {
+		} else {
-			return ctx, fmt.Errorf("%s: %+v", text, err)
+			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)
+		ctx, stepErr = s.runStep(ctx, pickle, step, scenarioErr, isFirst, isLast)
-		switch stepErr {
+		if scenarioErr == nil || s.shouldFail(stepErr) {
-		case ErrUndefined:
+			scenarioErr = stepErr
-			// do not overwrite failed error
-			if err == ErrUndefined || err == nil {
-				err = stepErr
-			}
-		case ErrPending:
-			err = stepErr
-		case nil:
-		default:
-			err = stepErr
 		}
 	}
 
-	return ctx, err
+	return ctx, scenarioErr
 }
 
 func (s *suite) shouldFail(err error) bool {
-	if err == nil {
+	if err == nil || err == ErrSkip {
 		return false
 	}
 

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: <nil>, 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: <nil>, 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: <nil>, status: skipped
+				<< After scenario "test", error: <nil>
+				<<<< 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: <nil>, status: skipped
+				<< After scenario "test", error: <nil>
+				<<<< 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: <nil>, 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"
+				<step action>
+				After step "step passes", error: <nil>, status: passed
+				Before step "step passes"
+				<step action>
+				After step "step passes", error: <nil>, status: passed
+				<< After scenario "test", error: <nil>
+				<<<< 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: <nil>, status: skipped
+				<< After scenario "test", error: <nil>
+				<<<< 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"
+				<step action>
+				After step "step passes", error: <nil>, status: passed
+				Before step "step passes"
+				<step action>
+				After step "step passes", error: <nil>, status: passed
+				After step "multistep passes", error: <nil>, status: passed
+				Before step "multistep passes"
+				Before step "step passes"
+				<step action>
+				After step "step passes", error: <nil>, status: passed
+				Before step "step passes"
+				<step action>
+				After step "step passes", error: <nil>, status: passed
+				After step "multistep passes", error: <nil>, status: passed
+				<< After scenario "test", error: <nil>
+				<<<< 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 += "<step action>\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)
+		})
+	}
+}