Skip to content

add await.Require and await.RequireTrue#9490

Merged
stephanos merged 8 commits into
mainfrom
stephanos/all-require
May 15, 2026
Merged

add await.Require and await.RequireTrue#9490
stephanos merged 8 commits into
mainfrom
stephanos/all-require

Conversation

@stephanos
Copy link
Copy Markdown
Contributor

@stephanos stephanos commented Mar 12, 2026

What changed?

Introduces common/testing/await, a polling-based test helper that replaces testify's Eventually / EventuallyWithT.

Why?

From the package doc:

Improvements over testify's eventually functions:

   - Misuse detection: accidentally using the real *testing.T (e.g. s.T() or
     suite assertion methods) instead of the callback's collect T is a
     common mistake. This package detects it and fails with a clear message.

   - Safer bool predicates: unlike testify's Eventually, [RequireTrue] only
     accepts func() bool, so returning false is the sole retry signal. If the
     predicate accidentally marks the real test failed, it reports that
     immediately instead of polling until timeout.

   - Timeout-aware callbacks: callbacks receive a context derived from the
     parent context and canceled when the await timeout or test deadline is
     reached, so RPCs and blocking waits can exit instead of continuing after
     the retry window has expired.

   - Panic propagation: if the condition panics (e.g. nil dereference), the
     panic is propagated immediately rather than being silently swallowed
     or retried until timeout.
     See https:github.com/stretchr/testify/issues/1810

   - Bounded goroutine lifetime: each attempt completes before the next
     starts, avoiding the overlapping-attempt data races and "panic: Fail
     in goroutine after Test has completed" crashes seen with testify's
     Eventually.
     See https:github.com/stretchr/testify/issues/1611

   - Deadlock detection: a condition that ignores t.Context() is abandoned
     after a grace period, producing a clear "does it honor t.Context()?"
     failure instead of hanging until go test -timeout.

   - Condition always runs: testify's Eventually can fail without ever
     running the condition due to a timer/ticker race with short timeouts.
     This package runs the condition immediately on the first iteration.
     See https://github.com/stretchr/testify/issues/1652

The upstream fix (testify#1657) aims to address several of these but has been open since Oct 2024 without merging.

How did you test it?

  • built
  • run locally and tested manually
  • covered by existing tests
  • added new unit test(s)
  • added new functional test(s)

Potential risks

The biggest downside is that it adds more code that we own. But apart from any unknown bugs, this package should rarely/never change.

@stephanos stephanos changed the title use require over assert better eventually Mar 12, 2026
@stephanos stephanos force-pushed the stephanos/all-require branch 29 times, most recently from 7d3afac to 4e042ac Compare March 12, 2026 16:39
@stephanos stephanos force-pushed the stephanos/all-require branch 8 times, most recently from 7624541 to 1814d82 Compare May 3, 2026 21:44
@temporalio temporalio deleted a comment from semgrep-managed-scans Bot May 3, 2026
s.NoError(descErr)
s.NotNil(desc)
s.NotNil(desc.GetPendingWorkflowTask())
}, 5*time.Second, 250*time.Millisecond, "speculative WFT should be scheduled after sending update")
Copy link
Copy Markdown
Contributor Author

@stephanos stephanos May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Await example

@stephanos stephanos force-pushed the stephanos/all-require branch 4 times, most recently from 3343e44 to 96a2172 Compare May 5, 2026 15:15
Comment thread .github/.golangci.yml
Comment on lines +51 to +52
- pattern: '(^|\.)(Eventually|Eventuallyf|EventuallyWithT|EventuallyWithTf)(\(|$)'
msg: "Use await.Require / s.Await for assertion conditions, or await.RequireTrue / s.AwaitTrue for bool predicates, instead of testify Eventually helpers"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Require new code to use new Await

s.AwaitTrue(func() bool {
// wait for workflow task to fail 3 times
return atomic.LoadInt32(&failures) >= 3
}, 10*time.Second, 50*time.Millisecond)
Copy link
Copy Markdown
Contributor Author

@stephanos stephanos May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ AwaitTrue example

Comment on lines +91 to +93
```go
s.Await(func(s *MySuite) {
resp, err := client.GetStatus(s.Context())
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the sleekest way I can imagine doing this.

Execution: wfExecution,
})
return descErr == nil && desc.GetPendingWorkflowTask() != nil
s.NoError(descErr)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will completely eliminate a lot of "using the wrong t" issues.

s.Eventually(func() bool {
desc, descErr := env.FrontendClient().DescribeWorkflowExecution(testcore.NewContext(),
s.Awaitf(func(s *PrematureEosTestSuite) {
desc, descErr := env.FrontendClient().DescribeWorkflowExecution(s.Context(),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I like about this is that s.Context() is the same in and outside the block; but it will resolve to the right context.

Errorf(string, ...any)
Fatal(...any)
Fatalf(string, ...any)
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ using interface so it doesn't require concrete testing.T

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type TB interface {
    testing.TB
}

To simplify

Errorf(string, ...any)
Fatal(...any)
Fatalf(string, ...any)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type TB interface {
    testing.TB
}

To simplify

@@ -0,0 +1,39 @@
// Package await provides polling-based test assertions as a replacement
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be in a doc.go file

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen both approaches (doc.go and same name as package), but happy to move it to doc.go

Comment thread common/testing/parallelsuite/suite.go Outdated
initSuite(t *testing.T, assertT require.TestingT, ctx contextOverride)
}

type contextOverride struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? I only see it used in initSuite to set the passed in ctx.

Comment thread common/testing/await/require_true.go Outdated
tb.Helper()
run(testcontext.New(tb), tb, func(t *T) {
if !condition() {
t.failed = true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a t.Failed() func, could use that or remove that func.

Comment thread common/testing/await/require_true.go Outdated
if !condition() {
t.failed = true
}
}, timeout, pollInterval, "", "await.RequireTrue", requireTrueMisuseHint, false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name "await.RequireTrue" is not piped in by the functions that call RequireTrue so it will always show up as await.RequireTrue

Comment thread common/testing/parallelsuite/suite.go Outdated
if s.ctx != nil {
return s.ctx
}
return testcontext.New(s.T())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should set s.ctx so we can avoid the mutex lock/unlock on every call to s.Context()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be a race between the test and a await/regular goroutine, though, both calling s.Context() which would likely create a data race warning and fail the test. But I can add a sync.Once to only call testcontext.New at most once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants