Add Vitest lifecycle and structure APIs#184
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors test registration to a nested suite/test tree, adds suite-level hooks ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User Code"
participant Reg as "Registration System"
participant Tree as "Suite/Test Tree"
participant Exec as "Execution Engine"
participant Hooks as "Hooks Manager"
participant Test as "Test Function"
User->>Reg: call describe(name, fn) / test(...) / test.each(...)
Reg->>Tree: create suite/test nodes and set flags (IsFocused/IsSkipped/IsTodo)
Reg->>Tree: expand parameterized entries (each)
Exec->>Tree: RunTests — traverse root suite
Exec->>Hooks: run beforeAll for current suite
Hooks-->>Exec: beforeAll done
loop per test node (respecting focus/skips)
Exec->>Hooks: collect & run inherited beforeEach
Hooks-->>Exec: beforeEach done
alt executable
Exec->>Test: invoke test function with args
Test-->>Exec: result (pass/fail)
else skipped/todo
Exec-->>Exec: record skipped/todo
end
Exec->>Hooks: run afterEach
Hooks-->>Exec: afterEach done
end
Exec->>Hooks: run afterAll for current suite
Hooks-->>Exec: afterAll done
Exec-->>User: aggregate results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/testing.md (1)
343-358: Consider clarifying the example to avoid confusion.The example reassigns
instancein bothbeforeAll(line 344) andbeforeEach(line 348), which may confuse readers about the intended hook usage pattern. ThebeforeEachassignment effectively shadows thebeforeAllsetup on every test run.This is technically valid, but for documentation purposes, consider using distinct variables or separate examples to illustrate each hook's purpose more clearly.
📝 Suggested clarification
describe("with setup", () => { - let instance; + let sharedFixture; + let instance; beforeAll(() => { - instance = createSharedFixture(); + sharedFixture = createSharedFixture(); }); beforeEach(() => { instance = new MyClass(); }); afterEach(() => { // cleanup }); afterAll(() => { - instance = null; + sharedFixture = null; }); test("uses instance", () => { expect(instance).toBeTruthy(); + expect(sharedFixture).toBeTruthy(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/testing.md` around lines 343 - 358, The example uses the same variable name instance in both beforeAll (calling createSharedFixture()) and beforeEach (new MyClass()), which causes the beforeEach to shadow the beforeAll setup and confuses readers; update the docs to show distinct usage by either (a) using different variable names (e.g., sharedInstance for beforeAll and freshInstance for beforeEach) or (b) splitting into two separate examples that demonstrate beforeAll with createSharedFixture() and beforeEach with new MyClass() independently, referencing the hooks beforeAll, beforeEach and the symbols createSharedFixture, MyClass and instance to make the intent clear.units/Goccia.Builtins.TestAssertions.pas (1)
474-482: Consider adding an else clause for defensive coding.The
casestatement covers all currentTGocciaParameterizedRegistrationTargetvalues, but lacks anelseclause. If the enum is extended in the future, this could return an uninitialized string.🛡️ Defensive else clause
function TGocciaParameterizedRegistrationFunction.GetFunctionName: string; begin case FTarget of prtDescribe: Result := 'describe'; prtTest: Result := 'test'; + else + Result := 'unknown'; end; end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.pas` around lines 474 - 482, The GetFunctionName method lacks a fallback for unknown TGocciaParameterizedRegistrationTarget values; update TGocciaParameterizedRegistrationFunction.GetFunctionName to add an else branch in the case over FTarget that defensively handles unexpected enum values (for example set a safe default Result or raise an exception/assertion with a clear message referencing the unexpected FTarget value) so the function never returns an uninitialized string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/testing.md`:
- Around line 343-358: The example uses the same variable name instance in both
beforeAll (calling createSharedFixture()) and beforeEach (new MyClass()), which
causes the beforeEach to shadow the beforeAll setup and confuses readers; update
the docs to show distinct usage by either (a) using different variable names
(e.g., sharedInstance for beforeAll and freshInstance for beforeEach) or (b)
splitting into two separate examples that demonstrate beforeAll with
createSharedFixture() and beforeEach with new MyClass() independently,
referencing the hooks beforeAll, beforeEach and the symbols createSharedFixture,
MyClass and instance to make the intent clear.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 474-482: The GetFunctionName method lacks a fallback for unknown
TGocciaParameterizedRegistrationTarget values; update
TGocciaParameterizedRegistrationFunction.GetFunctionName to add an else branch
in the case over FTarget that defensively handles unexpected enum values (for
example set a safe default Result or raise an exception/assertion with a clear
message referencing the unexpected FTarget value) so the function never returns
an uninitialized string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d4b57ed0-8322-4207-8d59-712a29cca44d
📒 Files selected for processing (10)
docs/built-ins.mddocs/testing.mdtests/language/testing/afterAll.jstests/language/testing/beforeAll.jstests/language/testing/describe-each.jstests/language/testing/describe-only.jstests/language/testing/test-each.jstests/language/testing/test-only.jstests/language/testing/test-todo.jsunits/Goccia.Builtins.TestAssertions.pas
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 93 improved · 🔴 32 regressed · 149 unchanged · avg +0.7% arraybuffer.js — Interp: 🟢 6, 🔴 1, 7 unch. · avg +0.9% · Bytecode: 🟢 13, 1 unch. · avg +13.2%
arrays.js — Interp: 🟢 2, 🔴 1, 16 unch. · avg +0.2% · Bytecode: 🟢 19 · avg +10.7%
async-await.js — Interp: 🟢 1, 🔴 2, 3 unch. · avg -0.4% · Bytecode: 🟢 6 · avg +8.8%
classes.js — Interp: 🟢 23, 8 unch. · avg +2.0% · Bytecode: 🟢 20, 🔴 1, 10 unch. · avg +9.3%
closures.js — Interp: 🟢 7, 4 unch. · avg +2.3% · Bytecode: 🟢 10, 1 unch. · avg +10.0%
collections.js — Interp: 🟢 9, 3 unch. · avg +2.2% · Bytecode: 🟢 9, 🔴 2, 1 unch. · avg +4.9%
destructuring.js — Interp: 🟢 3, 🔴 5, 14 unch. · avg -0.1% · Bytecode: 🟢 22 · avg +6.2%
fibonacci.js — Interp: 🟢 1, 7 unch. · avg +0.3% · Bytecode: 🟢 8 · avg +14.4%
for-of.js — Interp: 7 unch. · avg +0.2% · Bytecode: 🟢 7 · avg +7.6%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 4, 16 unch. · avg +0.7% · Bytecode: 🟢 20 · avg +12.7%
json.js — Interp: 🟢 8, 12 unch. · avg +1.0% · Bytecode: 🟢 20 · avg +11.0%
jsx.jsx — Interp: 🟢 2, 🔴 5, 14 unch. · avg -0.3% · Bytecode: 🟢 13, 🔴 1, 7 unch. · avg +2.3%
modules.js — Interp: 🟢 2, 7 unch. · avg +1.2% · Bytecode: 🟢 9 · avg +16.2%
numbers.js — Interp: 🟢 6, 🔴 1, 4 unch. · avg +2.2% · Bytecode: 🟢 10, 1 unch. · avg +13.5%
objects.js — Interp: 🔴 5, 2 unch. · avg -2.3% · Bytecode: 🟢 4, 🔴 2, 1 unch. · avg +1.8%
promises.js — Interp: 🔴 9, 3 unch. · avg -2.2% · Bytecode: 🟢 12 · avg +6.3%
regexp.js — Interp: 🟢 8, 3 unch. · avg +2.4% · Bytecode: 🟢 8, 3 unch. · avg +5.4%
strings.js — Interp: 🟢 4, 🔴 1, 6 unch. · avg +1.9% · Bytecode: 🟢 10, 🔴 1 · avg +7.0%
typed-arrays.js — Interp: 🟢 7, 🔴 2, 13 unch. · avg +0.1% · Bytecode: 🟢 21, 1 unch. · avg +12.9%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
units/Goccia.Builtins.TestAssertions.Test.pas (3)
2315-2348: Consider usingascast for clearer test failure messages.The direct cast
TGocciaNativeFunctionValue(OnlyValue)at line 2339 will raise an obscure access violation ifOnlyValueis nil or not aTGocciaNativeFunctionValue. Using anascast would produce a clearerInvalid typecastexception pointing to the exact issue.💡 Suggested improvement
- TGocciaNativeFunctionValue(OnlyValue).Call(TestArgs, nil); + (OnlyValue as TGocciaNativeFunctionValue).Call(TestArgs, nil);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.Test.pas` around lines 2315 - 2348, In TestItOnlySkipsNonFocusedTests replace the raw Delphi typecast of OnlyValue (TGocciaNativeFunctionValue(OnlyValue).Call(...)) with an "as" cast to produce a clear InvalidCast exception when the value is nil or of the wrong type; locate where OnlyValue is obtained via FScope.ResolveIdentifier('it') and change the call site to use OnlyValue as TGocciaNativeFunctionValue before invoking Call (or perform an explicit as-cast assignment to a TGocciaNativeFunctionValue local) so failures report a descriptive typecast error instead of an obscure access violation.
2418-2457: Test logic is correct; consider usingascast for clearer failure diagnostics.The test correctly verifies that
test.eachexpands tests for each table row, with arguments passed to the callback. The direct cast at line 2438 could useasfor more descriptive errors if the type is wrong.💡 Suggested improvement
- ReturnedFunc := TGocciaFunctionBase(FAssertions.TestEach(EachArgs, nil)); + ReturnedFunc := FAssertions.TestEach(EachArgs, nil) as TGocciaFunctionBase;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.Test.pas` around lines 2418 - 2457, Replace the hard C-style cast of the TestEach return with an "as" cast to get clearer failure diagnostics: instead of coercing FAssertions.TestEach(...) directly into TGocciaFunctionBase, use the "as" operator when assigning to ReturnedFunc so a wrong type raises a descriptive exception; update the assignment involving ReturnedFunc, FAssertions.TestEach and TGocciaFunctionBase accordingly.
2476-2515: Test correctly verifies suite expansion; sameascast suggestion applies.The test validates the two-pass registration/execution model: suite callbacks run during registration (adding
suite:one,suite:two), then test bodies execute (addingtest,test), giving 4 total events. The assertion at lines 2512-2514 properly verifies registration order.The cast at line 2495 has the same minor improvement opportunity as noted for line 2438.
💡 Suggested improvement
- ReturnedFunc := TGocciaFunctionBase(FAssertions.DescribeEach(EachArgs, nil)); + ReturnedFunc := FAssertions.DescribeEach(EachArgs, nil) as TGocciaFunctionBase;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.Test.pas` around lines 2476 - 2515, Replace the direct typecast of the DescribeEach return with an "as" cast to improve RTTI safety: in TestDescribeEachRegistersExpandedSuites change the assignment to ReturnedFunc so it uses FAssertions.DescribeEach(EachArgs, nil) as TGocciaFunctionBase instead of TGocciaFunctionBase(...); this mirrors the same improvement suggested for the earlier cast and keeps the symbol names ReturnedFunc and DescribeEach intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Builtins.TestAssertions.Test.pas`:
- Around line 2315-2348: In TestItOnlySkipsNonFocusedTests replace the raw
Delphi typecast of OnlyValue (TGocciaNativeFunctionValue(OnlyValue).Call(...))
with an "as" cast to produce a clear InvalidCast exception when the value is nil
or of the wrong type; locate where OnlyValue is obtained via
FScope.ResolveIdentifier('it') and change the call site to use OnlyValue as
TGocciaNativeFunctionValue before invoking Call (or perform an explicit as-cast
assignment to a TGocciaNativeFunctionValue local) so failures report a
descriptive typecast error instead of an obscure access violation.
- Around line 2418-2457: Replace the hard C-style cast of the TestEach return
with an "as" cast to get clearer failure diagnostics: instead of coercing
FAssertions.TestEach(...) directly into TGocciaFunctionBase, use the "as"
operator when assigning to ReturnedFunc so a wrong type raises a descriptive
exception; update the assignment involving ReturnedFunc, FAssertions.TestEach
and TGocciaFunctionBase accordingly.
- Around line 2476-2515: Replace the direct typecast of the DescribeEach return
with an "as" cast to improve RTTI safety: in
TestDescribeEachRegistersExpandedSuites change the assignment to ReturnedFunc so
it uses FAssertions.DescribeEach(EachArgs, nil) as TGocciaFunctionBase instead
of TGocciaFunctionBase(...); this mirrors the same improvement suggested for the
earlier cast and keeps the symbol names ReturnedFunc and DescribeEach intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fd2d3f3-314f-492a-8517-ffe1cd41e3fc
📒 Files selected for processing (1)
units/Goccia.Builtins.TestAssertions.Test.pas
- Cast focused and each-returned callbacks to their concrete function types - Simplify test assertions around `it.only`, `TestEach`, and `DescribeEach`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
units/Goccia.Builtins.TestAssertions.Test.pas (1)
1573-1592: Use this helper to replace the stale{ silent: true }setup.Lines 1577-1579 now match the real
RunTestscontract, but the older skip/runIf cases below still hand-roll{ silent: true }, whichRunTestsdoes not read. ReusingRunRegisteredTeststhere would remove dead setup and keep the suite aligned with the actual options surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.Test.pas` around lines 1573 - 1592, The tests still create a hand-rolled `{ silent: true }` arguments object instead of using the new helper; replace those manual setups in the skip/runIf test cases with a call to the existing RunRegisteredTests helper (which uses CreateRunOptions and calls FAssertions.RunTests) so the tests use the real options surface — locate occurrences that build a TGocciaArgumentsCollection with a Params.AssignProperty('silent', ...) and swap them to call RunRegisteredTests (adjusting expected return types to TGocciaObjectValue where needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@units/Goccia.Builtins.TestAssertions.Test.pas`:
- Around line 1647-1766: These test helpers are calling TGocciaTestAssertions
internals directly (e.g., FAssertions.BeforeAll, FAssertions.Test,
FAssertions.AfterAll) — change RegisterBeforeAllSuiteCallback,
RegisterAfterAllSuiteCallback, RegisterFocusedSuiteCallback,
RegisterNonFocusedSuiteCallback and RegisterDescribeEachSuiteCallback to resolve
the public callable from the runtime scope (like the pattern used in
TestItOnlySkipsNonFocusedTests: use FScope to get the global/test/describe
callable) and invoke that resolved TGocciaValue/TGocciaNativeFunctionValue
instead of calling FAssertions.* directly so the tests exercise the actual
public API surface users call.
---
Nitpick comments:
In `@units/Goccia.Builtins.TestAssertions.Test.pas`:
- Around line 1573-1592: The tests still create a hand-rolled `{ silent: true }`
arguments object instead of using the new helper; replace those manual setups in
the skip/runIf test cases with a call to the existing RunRegisteredTests helper
(which uses CreateRunOptions and calls FAssertions.RunTests) so the tests use
the real options surface — locate occurrences that build a
TGocciaArgumentsCollection with a Params.AssignProperty('silent', ...) and swap
them to call RunRegisteredTests (adjusting expected return types to
TGocciaObjectValue where needed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bff45268-5de0-4535-9e6c-8ffb28127645
📒 Files selected for processing (1)
units/Goccia.Builtins.TestAssertions.Test.pas
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.Builtins.TestAssertions.Test.pas (1)
1575-1585: Harden callable resolution failures for clearer diagnostics.
as TGocciaFunctionBasehere can fail with generic cast/access violations, which makes broken API-surface wiring harder to diagnose in test failures. Consider explicit type checks with targeted exception messages.Suggested refactor
function TTestSkipAndConditionalAPIs.ResolveGlobalCallable( const AName: string): TGocciaFunctionBase; +var + Value: TGocciaValue; begin - Result := FScope.ResolveIdentifier(AName) as TGocciaFunctionBase; + Value := FScope.ResolveIdentifier(AName); + if not (Value is TGocciaFunctionBase) then + raise Exception.CreateFmt('Expected callable global "%s"', [AName]); + Result := TGocciaFunctionBase(Value); end; function TTestSkipAndConditionalAPIs.ResolveCallableProperty(const AName, AProperty: string): TGocciaFunctionBase; +var + Value: TGocciaValue; begin - Result := ResolveGlobalCallable(AName).GetProperty(AProperty) as TGocciaFunctionBase; + Value := ResolveGlobalCallable(AName).GetProperty(AProperty); + if not (Value is TGocciaFunctionBase) then + raise Exception.CreateFmt('Expected callable property "%s.%s"', [AName, AProperty]); + Result := TGocciaFunctionBase(Value); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Builtins.TestAssertions.Test.pas` around lines 1575 - 1585, ResolveGlobalCallable and ResolveCallableProperty perform unchecked casts that can raise obscure access violations; instead verify the resolved objects and raise clear exceptions. In ResolveGlobalCallable, call FScope.ResolveIdentifier(AName), check for nil and confirm the returned object is a TGocciaFunctionBase (using "is" or TryCast), and raise a descriptive exception naming AName and the actual type when the check fails; in ResolveCallableProperty, call ResolveGlobalCallable(AName) then call GetProperty(AProperty) and similarly check for nil/type before casting, raising a descriptive exception that includes both AName and AProperty if the property is missing or not a TGocciaFunctionBase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.Builtins.TestAssertions.Test.pas`:
- Around line 1575-1585: ResolveGlobalCallable and ResolveCallableProperty
perform unchecked casts that can raise obscure access violations; instead verify
the resolved objects and raise clear exceptions. In ResolveGlobalCallable, call
FScope.ResolveIdentifier(AName), check for nil and confirm the returned object
is a TGocciaFunctionBase (using "is" or TryCast), and raise a descriptive
exception naming AName and the actual type when the check fails; in
ResolveCallableProperty, call ResolveGlobalCallable(AName) then call
GetProperty(AProperty) and similarly check for nil/type before casting, raising
a descriptive exception that includes both AName and AProperty if the property
is missing or not a TGocciaFunctionBase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd77bfb5-e7f8-40cd-b0a1-7049314d2f8f
📒 Files selected for processing (1)
units/Goccia.Builtins.TestAssertions.Test.pas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Goccia.Builtins.TestAssertionsTestAssertionsbehavior in the native Pascal test suite instead of JavaScript self-testsCloses #118
Testing
./build.pas clean testsfor t in build/Goccia.*.Test; do "$t"; done./build.pas testrunner && ./build/TestRunner tests --no-progress./format.pas --checkSummary by CodeRabbit