feat(zest): support custom test fixtures#83
Conversation
Refactor test state from member variable to thread-local, making assertion macros usable anywhere (fixture methods, free functions, lambdas). Add TEST_SUITE_F macro for custom fixture base classes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIntroduces thread-local test-state accessors and mutators; removes per-instance test state and mutators from TestSuiteDef; makes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/eventide/zest/macro.h (1)
25-30:⚠️ Potential issue | 🟠 Major
ASSERT_*andCO_ASSERT_*in helper methods andsetup()don't stop test execution.These macros now compile in
setup(), fixture helpers, and other non-test functions becausefailure()is fully qualified. However, they still only execute a plainreturn/co_returntoZEST_CHECK_IMPL, which exits that function but allows execution to continue. A failed assertion insetup()will set the test state toFailedand return fromsetup(), butrun_testthen proceeds directly to the test body. The test continues running despite the failure marker, breaking the intended fatal-assertion contract. To actually stop test execution early, a non-local unwind mechanism (exception, longjmp, etc.) would be needed instead of a scopedreturn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/eventide/zest/macro.h` around lines 25 - 30, The macros (ZEST_CHECK_IMPL used by ASSERT_* and CO_ASSERT_*) currently only execute a local return_action, which lets setup() or helpers return and allows run_test to continue; to fix, make the failure path perform a non-local unwind: implement a dedicated exception type (e.g., ::eventide::zest::test_failure_exception) and change ::eventide::zest::failure() to throw that exception (or add a new ::eventide::zest::abort_test() that throws), update ZEST_CHECK_IMPL to call the throwing failure/abort helper instead of relying on return_action, and ensure run_test catches this exception to stop executing the test body immediately; target symbols: ZEST_CHECK_IMPL, ASSERT_*/CO_ASSERT_*, ::eventide::zest::failure(), run_test, setup().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@include/eventide/zest/macro.h`:
- Around line 25-30: The macros (ZEST_CHECK_IMPL used by ASSERT_* and
CO_ASSERT_*) currently only execute a local return_action, which lets setup() or
helpers return and allows run_test to continue; to fix, make the failure path
perform a non-local unwind: implement a dedicated exception type (e.g.,
::eventide::zest::test_failure_exception) and change ::eventide::zest::failure()
to throw that exception (or add a new ::eventide::zest::abort_test() that
throws), update ZEST_CHECK_IMPL to call the throwing failure/abort helper
instead of relying on return_action, and ensure run_test catches this exception
to stop executing the test body immediately; target symbols: ZEST_CHECK_IMPL,
ASSERT_*/CO_ASSERT_*, ::eventide::zest::failure(), run_test, setup().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 292a4f23-72a8-4d6f-b204-c95c16653bd4
📒 Files selected for processing (3)
include/eventide/zest/detail/registry.hinclude/eventide/zest/detail/suite.hinclude/eventide/zest/macro.h
TEST_SUITE(name) for default, TEST_SUITE(name, fixture) for custom fixture base class. No need for a separate macro. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract shared loop_fixture with schedule_all() helper to simplify async test setup across 9 test files (-296 lines, +85 lines). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
TestSuiteDefmember variable to thread-local, so assertion macros (EXPECT_*,ASSERT_*,CO_ASSERT_*) work anywhere — fixture methods, free functions, lambdasTEST_SUITE_F(name, fixture)macro for inheriting from a custom fixture base classfailure()/pass()/skip()are now free functions ineventide::zestnamespaceTest plan
TEST_SUITE_Fwith a custom fixture that calls assertions in fixture methods🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes