镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

Default-initialize TestFixture::SetupParams' bools - #8396

Open
danlapid wants to merge 1 commit into
mainfrom
dlapid/test-fixture-bool-defaults
Open

danlapid wants to merge 1 commit into
mainfrom
dlapid/test-fixture-bool-defaults

Conversation

@danlapid

Copy link
Copy Markdown
Collaborator

TestFixture::SetupParams had no initializers on ignoreAllAutogatesEnv, useRealTimers and flushActorStorage. Aggregate initialization ({...}) zeroes them, so the existing callers were fine. The new "reports an internal exception to the isolate's observer and the request's, once" case in uncaught-exception-test.c++ declares TestFixture::SetupParams params;, which leaves them indeterminate. buildConfig() then reads ignoreAllAutogatesEnv, and UBSan aborts:

src/workerd/tests/test-fixture.c++:287:57: runtime error: load of value 226, which is not a valid value for type 'bool'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/workerd/tests/test-fixture.c++:287:57

This fails @workerd//src/workerd/io:uncaught-exception-test (both variants) in the ubsan job of every edgeworker master pipeline since the test landed: 16 of 16 that reached the job on 2026-11-12.

Change

  • Give the three bools = false.
  • Clang rejects member initializers that the enclosing class's own in-class default argument depends on ("default member initializer ... needed within definition of enclosing class"). So TestFixture(SetupParams&& params = {.useRealTimers = false}) becomes a parameterless constructor that delegates, plus TestFixture(SetupParams&&).

Testing

  • bazel test --config=ubsan --runs_per_test=5 @workerd//src/workerd/io:uncaught-exception-test@ @workerd//src/workerd/io:uncaught-exception-test@all-autogates: 5/5 pass. Without the change, the same command fails with the error above.
  • Every test target that depends on //src/workerd/tests:test-fixture builds. bench-pumpto and bench-stream-piping fail at this base too: they still use the removed jsg::BackingStore.

🤖 Generated with Claude Code

SetupParams left ignoreAllAutogatesEnv, useRealTimers and
flushActorStorage without initializers. Aggregate initialization
(`{...}`) zeroes them, but uncaught-exception-test's "reports an
internal exception to the isolate's observer" case declares
`TestFixture::SetupParams params;`, which leaves them indeterminate.
buildConfig() then reads ignoreAllAutogatesEnv, and UBSan stops the
test:

  test-fixture.c++:287:57: runtime error: load of value 226, which
  is not a valid value for type 'bool'

That fails uncaught-exception-test in every edgeworker master
pipeline's ubsan job.

Give the bools `= false`. Clang rejects member initializers that the
enclosing class's own default argument depends on before the class is
complete, so replace TestFixture's `= {.useRealTimers = false}`
default argument with a parameterless constructor that delegates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@danlapid
danlapid requested review from a team as code owners November 12, 2026 15:39

@clanky-the-clanker clanky-the-clanker Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Approved — This is a small, correct fix for reading uninitialized bools in a test fixture. The constructor change is needed for the new initializers and keeps every existing call site working.

The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.


✅ All checks passed

📋 Code Quality: ✅ LGTM

The default member initializers fix the indeterminate bools in a default-initialized SetupParams. The delegating parameterless constructor keeps the old default-argument behaviour and avoids Clang's error about an incomplete enclosing class.


📝 Documentation: ✅ LGTM

The new comment above the constructors accurately describes why they are written this way.


Reviewed commit: ab282f1a

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.

3 participants