Skip to content

Commit

Permalink
Fix functional subtest execution (#7083)
Browse files Browse the repository at this point in the history
## What changed?
<!-- Describe what has changed in this PR -->
Fix functional subtest execution.
Credits to @stephanos for the brilliant idea!

## Why?
<!-- Tell your future self why have you made these changes -->
Previously, if there is a assert failure (like `s.Equal`) in subtest, it
failed entire test and stoped executing other subtests. Error message
went to parent test as well, while subtest failed with wired message
"subtest paniced". This is because we override `s.Assertions` in every
test with `require.New` and it calls `FailNow` when condition is not
meet. Same thing must to be done for every subtest. And every test
helper which depends on `s.T()`.

## How did you test it?
<!-- How have you verified this change? Tested locally? Added a unit
test? Checked in staging env? -->
Simulate failure of some sub tests and checked results.

## Potential risks
<!-- Assuming the worst case, what can be broken when deploying this
change to production? -->
No risks.

## Documentation
<!-- Have you made sure this change doesn't falsify anything currently
stated in `docs/`? If significant
new behavior is added, have you described that in `docs/`? -->
No.

## Is hotfix candidate?
<!-- Is this PR a hotfix candidate or does it require a notification to
be sent to the broader community? (Yes/No) -->
No.
  • Loading branch information
alexshtin authored Jan 15, 2025
1 parent 0d0c290 commit 59133e0
Show file tree
Hide file tree
Showing 2 changed files with 29 additions and 5 deletions.
20 changes: 16 additions & 4 deletions tests/testcore/client_suite.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ func (s *ClientFunctionalSuite) SetupSuite() {
}
s.SetDynamicConfigOverrides(dynamicConfigOverrides)
s.FunctionalTestBase.SetupSuite("testdata/client_cluster.yaml")

}

func (s *ClientFunctionalSuite) TearDownSuite() {
Expand All @@ -100,9 +99,7 @@ func (s *ClientFunctionalSuite) TearDownSuite() {
func (s *ClientFunctionalSuite) SetupTest() {
s.FunctionalTestBase.SetupTest()

// Have to define our overridden assertions in the test setup. If we did it earlier, s.T() will return nil
s.Assertions = require.New(s.T())
s.HistoryRequire = historyrequire.New(s.T())
s.initAssertions()

// Set URL template after httpAPAddress is set, see commonnexus.RouteCompletionCallback
s.OverrideDynamicConfig(
Expand Down Expand Up @@ -136,6 +133,21 @@ func (s *ClientFunctionalSuite) TearDownTest() {
}
}

func (s *ClientFunctionalSuite) SetupSubTest() {
s.initAssertions()
}

func (s *ClientFunctionalSuite) initAssertions() {
// `s.Assertions` (as well as other test helpers which depends on `s.T()`) must be initialized on
// both test and subtest levels (but not suite level, where `s.T()` is `nil`).
//
// If these helpers are not reinitialized on subtest level, any failed `assert` in
// subtest will fail the entire test (not subtest) immediately without running other subtests.

s.Assertions = require.New(s.T())
s.HistoryRequire = historyrequire.New(s.T())
}

func (s *ClientFunctionalSuite) EventuallySucceeds(ctx context.Context, operationCtx backoff.OperationCtx) {
s.T().Helper()
s.NoError(backoff.ThrottleRetryContext(
Expand Down
14 changes: 13 additions & 1 deletion tests/testcore/functional.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,20 @@ func (s *FunctionalSuite) TearDownSuite() {

func (s *FunctionalSuite) SetupTest() {
s.FunctionalTestBase.SetupTest()
s.initAssertions()
}

func (s *FunctionalSuite) SetupSubTest() {
s.initAssertions()
}

func (s *FunctionalSuite) initAssertions() {
// `s.Assertions` (as well as other test helpers which depends on `s.T()`) must be initialized on
// both test and subtest levels (but not suite level, where `s.T()` is `nil`).
//
// If these helpers are not reinitialized on subtest level, any failed `assert` in
// subtest will fail the entire test (not subtest) immediately without running other subtests.

// Have to define our overridden assertions in the test setup. If we did it earlier, s.T() will return nil
s.Assertions = require.New(s.T())
s.ProtoAssertions = protorequire.New(s.T())
s.HistoryRequire = historyrequire.New(s.T())
Expand Down

0 comments on commit 59133e0

Please sign in to comment.