Modified chain.leave() to handle concurrency (#280)

Signed-off-by: ROHITH RAJU <rohithraju488@gmail.com>
Co-authored-by: Victor Gaydov <victor@enise.org>

chain.go

@@ -78,6 +78,7 @@ type chain struct {
 	context  AssertionContext
 	handler  AssertionHandler
 	severity AssertionSeverity
+	failure  *AssertionFailure
 }
 
 // If enabled, chain will panic if used incorrectly or gets illformed AssertionFailure.
@@ -273,12 +274,15 @@ func (c *chain) clone() *chain {
 	return &chain{
 		parent: c,
 		state:  stateCloned,
-		// since the new clone doesn't have children yet, flagFailedChildren
-		// is not inherited
+		// flagFailedChildren is not inherited because the newly created clone
+		// doesn't have children
 		flags:    (c.flags & ^flagFailedChildren),
 		context:  contextCopy,
 		handler:  c.handler,
 		severity: c.severity,
+		// failure is not inherited because it should be reported only once
+		// by the chain where it happened
+		failure: nil,
 	}
 }
 
@@ -334,20 +338,19 @@ func (c *chain) replace(name string, args ...interface{}) *chain {
 }
 
 // Finalize assertion.
-// If there were no failures, report succeeded assertion to AssertionHandler.
-// Otherwise, mark parent as failed and notify grandparents that they
-// have faield children.
+// Report success of failure to AssertionHandler.
+// In case of failure, also recursively notify parents and grandparents
+// that they have faield children.
 // Must be called after enter().
 // Chain can't be used after this call.
 func (c *chain) leave() {
 	var (
-		context       AssertionContext
-		handler       AssertionHandler
-		parent        *chain
-		reportSuccess bool
-		reportFailure bool
+		parent  *chain
+		flags   chainFlags
+		context AssertionContext
+		handler AssertionHandler
+		failure *AssertionFailure
 	)
-
 	func() {
 		c.mu.Lock()
 		defer c.mu.Unlock()
@@ -357,21 +360,30 @@ func (c *chain) leave() {
 		}
 		c.state = stateLeaved
 
-		if c.flags&(flagFailed|flagFailedChildren) == 0 {
-			context = c.context
-			handler = c.handler
-			reportSuccess = true
-		} else if c.parent != nil {
-			parent = c.parent
-			reportFailure = true
-		}
+		parent = c.parent
+		flags = c.flags
+
+		context = c.context
+		handler = c.handler
+		failure = c.failure
+
 	}()
 
-	if reportSuccess {
+	if flags&(flagFailed|flagFailedChildren) == 0 {
 		handler.Success(&context)
 	}
 
-	if reportFailure {
+	if flags&(flagFailed) != 0 && failure != nil {
+		handler.Failure(&context, failure)
+
+		if chainValidation {
+			if err := validateAssertion(failure); err != nil {
+				panic(err)
+			}
+		}
+	}
+
+	if flags&(flagFailed|flagFailedChildren) != 0 && parent != nil {
 		parent.mu.Lock()
 		parent.flags |= flagFailed
 		p := parent.parent
@@ -387,47 +399,28 @@ func (c *chain) leave() {
 	}
 }
 
-// Report assertion failure and mark chain as failed.
+// Mark chain as failed.
+// Remember failure inside chain. It will be reported in leave().
+// Subsequent fail() call will be ignored.
 // Must be called between enter() and leave().
 func (c *chain) fail(failure AssertionFailure) {
-	var (
-		context       AssertionContext
-		handler       AssertionHandler
-		reportFailure bool
-	)
-
-	func() {
-		c.mu.Lock()
-		defer c.mu.Unlock()
-
-		if chainValidation && c.state != stateEntered {
-			panic("fail allowed only between enter/leave")
-		}
-
-		if c.flags&flagFailed != 0 {
-			return
-		}
-		c.flags |= flagFailed
-
-		failure.Severity = c.severity
-		if c.severity == SeverityError {
-			failure.IsFatal = true
-		}
+	c.mu.Lock()
+	defer c.mu.Unlock()
 
-		context = c.context
-		handler = c.handler
-		reportFailure = true
-	}()
+	if chainValidation && c.state != stateEntered {
+		panic("fail allowed only between enter/leave")
+	}
 
-	if reportFailure {
-		handler.Failure(&context, &failure)
+	if c.flags&flagFailed != 0 {
+		return
+	}
+	c.flags |= flagFailed
 
-		if chainValidation {
-			if err := validateAssertion(&failure); err != nil {
-				panic(err)
-			}
-		}
+	failure.Severity = c.severity
+	if c.severity == SeverityError {
+		failure.IsFatal = true
 	}
+	c.failure = &failure
 }
 
 // Check if chain failed.

chain_test.go

@@ -1,6 +1,7 @@
 package httpexpect
 
 import (
+	"errors"
 	"strings"
 	"testing"
 
@@ -403,10 +404,10 @@ func TestChain_Panics(t *testing.T) {
 				chain.setRequestName("")
 			},
 			func(chain *chain) {
-				chain.setRequest(nil)
+				chain.setRequest(&Request{})
 			},
 			func(chain *chain) {
-				chain.setResponse(nil)
+				chain.setResponse(&Response{})
 			},
 		}
 
@@ -431,6 +432,7 @@ func TestChain_Panics(t *testing.T) {
 			opChain.fail(AssertionFailure{
 				Type: AssertionType(9999),
 			})
+			opChain.leave()
 		})
 	})
 }
@@ -662,3 +664,40 @@ func TestChain_Severity(t *testing.T) {
 		assert.Equal(t, SeverityLog, handler.failure.Severity)
 	})
 }
+
+func TestChain_Reporting(t *testing.T) {
+	handler := &mockAssertionHandler{}
+
+	failure := AssertionFailure{
+		IsFatal: true,
+		Errors: []error{
+			errors.New("test_error"),
+		},
+	}
+
+	chain := newChainWithConfig("test", Config{
+		AssertionHandler: handler,
+	}.withDefaults())
+
+	opChain := chain.enter("test")
+
+	assert.False(t, opChain.failed()) // no failure flag
+	assert.False(t, chain.failed())   // not reported to parent
+	assert.Nil(t, handler.ctx)        // not reported to handler
+	assert.Nil(t, handler.failure)
+
+	opChain.fail(failure)
+
+	assert.True(t, opChain.failed()) // has failure flag
+	assert.False(t, chain.failed())  // not reported to parent
+	assert.Nil(t, handler.ctx)       // not reported to handler
+	assert.Nil(t, handler.failure)
+
+	opChain.leave()
+
+	assert.True(t, opChain.failed()) // has failure flag
+	assert.True(t, chain.failed())   // reported to parent
+	assert.NotNil(t, handler.ctx)    // reported to handler
+	assert.NotNil(t, handler.failure)
+	assert.Equal(t, failure, *handler.failure)
+}

mocks_test.go

@@ -234,6 +234,7 @@ func (h *mockAssertionHandler) Failure(
 
 func mockFailure() AssertionFailure {
 	return AssertionFailure{
+		Type: AssertOperation,
 		Errors: []error{
 			errors.New("test_error"),
 		},