Add mock() and spyOn() test APIs#210
Merged
Merged
Conversation
- add mock function implementation and matchers - document standalone `mock()` and `spyOn()` globals - expand Pascal coverage for call tracking and restoration
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:
📝 WalkthroughWalkthroughAdds full mock/spy support: new TGocciaMockFunctionValue with call/result/instance tracking and control APIs, built-in Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant MockFn as TGocciaMockFunctionValue
participant Tracker as Call/Result Tracker
participant Matcher as TGocciaExpectationValue
Test->>MockFn: create via mock()/spyOn()
activate MockFn
MockFn->>MockFn: init calls/results/instances/queues
deactivate MockFn
Test->>MockFn: configure (mockImplementation / mockReturnValue / once)
activate MockFn
MockFn->>MockFn: enqueue or set behavior (one-shot FIFO or permanent)
deactivate MockFn
Test->>MockFn: invoke mock(args)
activate MockFn
MockFn->>Tracker: record args and this -> append to calls/instances
MockFn->>MockFn: select behavior (implOnce → returnOnce → impl → default → undefined)
MockFn->>Tracker: append result (return or throw) to results
MockFn-->>Test: return or rethrow
deactivate MockFn
Test->>Matcher: expect(mockFn).toHaveBeenCalledWith(args)
activate Matcher
Matcher->>MockFn: verify mock type, read .mock.calls/.results
Matcher->>Matcher: deep-equal compare expected vs recorded call/result
Matcher-->>Test: pass or fail assertion
deactivate Matcher
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 12 improved · 🔴 150 regressed · 112 unchanged · avg -1.7% arraybuffer.js — Interp: 🔴 12, 2 unch. · avg -1.7% · Bytecode: 🟢 1, 🔴 13 · avg -10.0%
arrays.js — Interp: 🟢 1, 🔴 14, 4 unch. · avg -1.5% · Bytecode: 🔴 17, 2 unch. · avg -8.8%
async-await.js — Interp: 🔴 4, 2 unch. · avg -1.7% · Bytecode: 🔴 6 · avg -9.5%
classes.js — Interp: 🔴 28, 3 unch. · avg -3.0% · Bytecode: 🟢 4, 🔴 12, 15 unch. · avg -3.0%
closures.js — Interp: 🔴 10, 1 unch. · avg -2.3% · Bytecode: 🔴 6, 5 unch. · avg -3.8%
collections.js — Interp: 🔴 11, 1 unch. · avg -7.3% · Bytecode: 🟢 1, 🔴 10, 1 unch. · avg -9.8%
destructuring.js — Interp: 🟢 1, 🔴 10, 11 unch. · avg -1.3% · Bytecode: 🟢 2, 🔴 12, 8 unch. · avg -2.6%
fibonacci.js — Interp: 🔴 6, 2 unch. · avg -1.8% · Bytecode: 🔴 8 · avg -5.0%
for-of.js — Interp: 🔴 2, 5 unch. · avg -0.9% · Bytecode: 🔴 6, 1 unch. · avg -5.1%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 9, 11 unch. · avg -1.6% · Bytecode: 🔴 18, 2 unch. · avg -5.9%
json.js — Interp: 🔴 13, 7 unch. · avg -1.8% · Bytecode: 🔴 18, 2 unch. · avg -6.7%
jsx.jsx — Interp: 🟢 3, 🔴 1, 17 unch. · avg +0.2% · Bytecode: 🟢 5, 🔴 2, 14 unch. · avg +0.2%
modules.js — Interp: 🔴 5, 4 unch. · avg -2.1% · Bytecode: 🔴 6, 3 unch. · avg -2.4%
numbers.js — Interp: 🟢 2, 🔴 4, 5 unch. · avg -0.5% · Bytecode: 🟢 1, 🔴 9, 1 unch. · avg -12.2%
objects.js — Interp: 🟢 1, 🔴 1, 5 unch. · avg +0.4% · Bytecode: 🟢 2, 🔴 1, 4 unch. · avg -0.7%
promises.js — Interp: 🟢 2, 🔴 3, 7 unch. · avg -0.2% · Bytecode: 🔴 6, 6 unch. · avg -2.6%
regexp.js — Interp: 🔴 1, 10 unch. · avg +0.1% · Bytecode: 🔴 6, 5 unch. · avg -4.4%
strings.js — Interp: 🔴 7, 4 unch. · avg -2.7% · Bytecode: 🟢 1, 🔴 7, 3 unch. · avg -5.2%
typed-arrays.js — Interp: 🟢 2, 🔴 9, 11 unch. · avg -1.1% · Bytecode: 🟢 6, 🔴 14, 2 unch. · avg -1.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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
units/Goccia.Builtins.TestAssertions.Test.pas (2)
2907-2916: Testmock(impl)through the exported helper, not the raw constructor.This case builds
TGocciaMockFunctionValuedirectly, so it never proves that themock()global forwards its optional implementation argument correctly. A bug in the registration layer would still leave this test green.🧪 Suggested test adjustment
- M := TGocciaMockFunctionValue.Create( - TGocciaNativeFunctionValue.Create(CallbackReturn42, 'impl', 0)); + M := CreateMockViaGlobal( + TGocciaNativeFunctionValue.Create(CallbackReturn42, 'impl', 0));🤖 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 2907 - 2916, The test TestMockWithImplementation currently constructs TGocciaMockFunctionValue directly (using TGocciaNativeFunctionValue and CallbackReturn42), which doesn't verify that the exported mock() helper forwards an implementation; update the test to call the exported mock(impl) helper with the implementation (e.g., CallbackReturn42) and assign its result to M, then use CallMockFunction(M, []) and Expect on the returned value—replace direct TGocciaMockFunctionValue.Create usage with the mock(...) helper call so the registration/forwarding path is exercised.
3280-3387: Exercise spies through the patched object property at least once.Every spy assertion in this block calls
Spydirectly, so the suite never verifies the behaviorspyOn()is supposed to preserve: invokingobj.method(...)with the original receiver. A regression that drops call-sitethisor records the wrong instance would still pass.Based on learnings: shorthand methods receive call-site
this, andthisbinding is resolved via virtual dispatch onTGocciaFunctionValue.BindThis.🤖 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 3280 - 3387, These tests call Spy directly instead of exercising the patched object property, so they don't verify call-site "this" binding or that obj.method(...) routes through the spy; modify each affected test (TestSpyOnPassesThrough, TestSpyOnTracksCalls, TestSpyOnMockImplementationOverrides, TestSpyOnMockRestoreRestoresOriginal, TestSpyOnMockReturnValueOverrides) to invoke the spy via the object's property (get the method from Obj with Obj.GetProperty('method') or call the method through Obj rather than calling Spy directly) and pass Obj as the receiver so BindThis on TGocciaFunctionValue is exercised; ensure CallMockFunction is used on the property/value retrieved from Obj (or otherwise simulate calling obj.method(...)) so the spy records the correct instance and "this".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 527: Remove guidance that points matcher coverage to JavaScript E2E
tests: update the description for tests/language/testing to keep mock.js and
spyOn.js but remove or relocate any mention of expect-toMatch.js and instead
state that TGocciaExpectationValue matchers are tested in
units/Goccia.Builtins.TestAssertions.Test.pas; ensure references to
expect-toMatch.js are deleted or moved and explicitly mention
TGocciaExpectationValue and units/Goccia.Builtins.TestAssertions.Test.pas so
contributors know where matcher coverage lives.
In `@docs/testing.md`:
- Around line 380-384: The example declares const fn twice which causes a
runtime error; update the snippet to use distinct bindings or separate examples
so each mock() call is shown independently — e.g., keep the first as const fn =
mock(); and rename the second to const fnWithImpl = mock((x) => x * 2); or split
into two code blocks demonstrating mock() and mock((x)=>x*2) so the functions
(mock, fn, fnWithImpl) are uniquely identified and the example runs.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 3098-3108: The spyOn implementation currently permits spying on
any existing property; before calling TGocciaMockFunctionValue.CreateSpy check
that ExistingValue is actually callable: require ExistingValue to be a
TGocciaFunctionBase (or an explicit accessor type your runtime uses for
getter/setter properties), and if not call ThrowError('spyOn: cannot spy on
non-callable property "' + MethodName + '"', 0, 0) and return
TGocciaUndefinedLiteralValue.UndefinedValue; update the guard around
ExistingValue (the value returned by Target.GetProperty) so only
callable/allowed accessor types proceed to CreateSpy.
In `@units/Goccia.Values.MockFunction.pas`:
- Around line 131-145: The spy currently only saves the resolved value
(FSpyOriginalValue) and re-applies it with DefineProperty, which destroys
accessor/non-enumerable flags and turns inherited methods into own data
properties; update Create (the constructor code that sets
FSpyTarget/FSpyMethodName/FSpyOriginalValue) to also capture and store the
original property descriptor and whether it was an own property (e.g., add
fields like FSpyOriginalDescriptor and FSpyWasOwnProperty), and then change
mockRestore() to restore by re-applying the saved descriptor when
FSpyWasOwnProperty is true or by deleting the shadowing own property when it was
inherited so the original inherited descriptor remains intact (use the existing
ATarget.DefineProperty and any available GetOwnPropertyDescriptor/GetProperty
helpers to obtain the full descriptor and ownership info).
- Around line 281-294: The catch in the implementation branches (around
FImplementationOnce usage and the call to TGocciaFunctionBase.Call) only handles
TGocciaThrowValue so native/runtime failures (TGocciaError and other Exception)
are not recorded into FResults; update the try/except blocks in both places (the
block using FImplementationOnce and the similar block at 308-319) to catch
TGocciaThrowValue as before but also catch TGocciaError and generic Exception,
and for each caught exception append a CreateResultEntry(RESULT_TYPE_THROW,
<appropriate value>) to FResults (use the exception's Value for
TGocciaThrowValue/TGocciaError and a converted error value or message for
generic Exception) before re-raising so FResults stays aligned with FCalls.
- Around line 199-247: The temporary TGocciaValue instances created in
CreateResultEntry, BuildCallsArray, BuildResultsArray, BuildInstancesArray and
BuildMockObject must be protected from GC while being constructed and before
they are attached to their parent objects: wrap creations in AddTempRoot(...)
and RemoveTempRoot(...) calls (or add the temp container as a temp root) so each
temporary (e.g. the TGocciaObjectValue returned by CreateResultEntry, each
element added to Result.Elements in Build*Array, and LastCall in
BuildMockObject) is registered with AddTempRoot immediately after creation and
only unregistered with RemoveTempRoot after the value has been AssignProperty'd
or added to the final object/array; ensure every code path (including early
returns) removes the temp root to avoid leaks.
---
Nitpick comments:
In `@units/Goccia.Builtins.TestAssertions.Test.pas`:
- Around line 2907-2916: The test TestMockWithImplementation currently
constructs TGocciaMockFunctionValue directly (using TGocciaNativeFunctionValue
and CallbackReturn42), which doesn't verify that the exported mock() helper
forwards an implementation; update the test to call the exported mock(impl)
helper with the implementation (e.g., CallbackReturn42) and assign its result to
M, then use CallMockFunction(M, []) and Expect on the returned value—replace
direct TGocciaMockFunctionValue.Create usage with the mock(...) helper call so
the registration/forwarding path is exercised.
- Around line 3280-3387: These tests call Spy directly instead of exercising the
patched object property, so they don't verify call-site "this" binding or that
obj.method(...) routes through the spy; modify each affected test
(TestSpyOnPassesThrough, TestSpyOnTracksCalls,
TestSpyOnMockImplementationOverrides, TestSpyOnMockRestoreRestoresOriginal,
TestSpyOnMockReturnValueOverrides) to invoke the spy via the object's property
(get the method from Obj with Obj.GetProperty('method') or call the method
through Obj rather than calling Spy directly) and pass Obj as the receiver so
BindThis on TGocciaFunctionValue is exercised; ensure CallMockFunction is used
on the property/value retrieved from Obj (or otherwise simulate calling
obj.method(...)) so the spy records the correct instance and "this".
🪄 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: f864d3c1-3e69-42fa-bb84-5ebf5dd7f6dd
📒 Files selected for processing (5)
AGENTS.mddocs/testing.mdunits/Goccia.Builtins.TestAssertions.Test.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Values.MockFunction.pas
- Reject non-callable spy targets with clear error message - Preserve original property descriptor in spyOn for proper mockRestore - Temp-root metadata objects during construction to prevent GC sweep - Catch all exception types in Call to keep FResults aligned with FCalls - Fix duplicate const fn in docs/testing.md example - Fix CLAUDE.md test directory guidance for matcher coverage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (4)
units/Goccia.Values.MockFunction.pas (3)
143-152:⚠️ Potential issue | 🟠 MajorRestore the saved descriptor by kind, not as a data property.
Line 152 installs the spy with a generic writable/enumerable data descriptor, and Lines 503-505 restore it the same way. Own non-enumerable methods change shape while the spy is active, and accessor properties still cannot round-trip correctly on
mockRestore().Also applies to: 501-505
221-246:⚠️ Potential issue | 🟠 MajorTemp-root the
.mockarrays until they are attached.
BuildCallsArray,BuildResultsArray, andBuildInstancesArrayreturn freshTGocciaArrayValues that are only in Pascal temporaries when Lines 256-258 assign them ontoResult. If GC runs during that descriptor allocation window, those arrays are still unreachable.Based on learnings: Values held only by Pascal code (not in any GocciaScript scope) must be protected with
AddTempRoot/RemoveTempRootfor the duration they are needed.Also applies to: 256-258
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.MockFunction.pas` around lines 221 - 246, BuildCallsArray, BuildResultsArray and BuildInstancesArray create TGocciaArrayValue instances that can be collected before they are attached; wrap the newly created array with AddTempRoot immediately after creation and ensure the matching RemoveTempRoot is called in the caller right after the array is assigned/attached to its owner (the place that currently assigns these returned arrays onto the descriptor/result). Use AddTempRoot(Result) in each of BuildCallsArray/BuildResultsArray/BuildInstancesArray right after TGocciaArrayValue.Create and remove the temp root with RemoveTempRoot at the attachment site so the arrays are protected during the allocation window.
309-319:⚠️ Potential issue | 🟠 MajorRecord thrown JS error objects in
mock.results.The generic
Exceptionpath currently storesE.Messageas a string. That flattens runtime-thrownTypeError/Errorobjects into text, somock.results[n].valueno longer matches the actual thrown JS value for native and built-in failures.Based on learnings: All runtime errors must use error helper functions so the exception carries a proper JS Error object catchable from JavaScript `try...catch`.Minimal fix
on E: TGocciaThrowValue do begin FResults.Add(CreateResultEntry(RESULT_TYPE_THROW, E.Value)); raise; end; + on E: TGocciaError do + begin + FResults.Add(CreateResultEntry(RESULT_TYPE_THROW, E.Value)); + raise; + end; on E: Exception do begin FResults.Add(CreateResultEntry(RESULT_TYPE_THROW, TGocciaStringLiteralValue.Create(E.Message))); raise; end;Also applies to: 340-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.MockFunction.pas` around lines 309 - 319, The Exception catch currently stores E.Message as a string (TGocciaStringLiteralValue.Create), which flattens native JS Error objects; instead replace that string-wrapping with the runtime error helper that builds a proper JS Error object and pass that into CreateResultEntry(RESULT_TYPE_THROW, ...). Update the except on Exception block that follows TGocciaThrowValue and the similar handlers around the 340-350 area so they use the project's error helper (the function that constructs JS Error values) rather than TGocciaStringLiteralValue.Create(E.Message), keeping the TGocciaThrowValue branch unchanged.AGENTS.md (1)
527-527:⚠️ Potential issue | 🟡 MinorKeep matcher coverage out of the JS test-directory guidance.
This bullet still points contributors at
tests/language/testing/for matcher coverage, even though the same sentence saysTGocciaExpectationValuematchers live inunits/Goccia.Builtins.TestAssertions.Test.pas. That sends people back to the wrong layer.Based on learnings:
TGocciaExpectationValuematchers must NOT be tested via JavaScript end-to-end tests undertests/; the correct layer isunits/Goccia.Builtins.TestAssertions.Test.pas.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 527, The documentation bullet incorrectly directs matcher coverage to the JS test directory; remove the reference to tests/language/testing from that sentence and replace it with a clear statement that TGocciaExpectationValue matchers are exercised in the unit test Goccia.Builtins.TestAssertions.Test (the Pascal unit) rather than via JavaScript end-to-end tests, ensuring the doc references the TGocciaExpectationValue matcher symbol and the Goccia.Builtins.TestAssertions.Test unit to guide contributors to the correct test layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/testing.md`:
- Around line 376-390: The docs currently show global mock() and spyOn() usage
and describe mock.instances as per-call this values, which is
GocciaScript-specific and conflicts with Vitest where the equivalents are
vi.fn() and vi.spyOn() and mock.instances holds constructor instances only;
update the Mock Functions section to (1) explicitly state that mock(), spyOn(),
and the described mock.instances semantics are GocciaScript TestRunner behavior,
(2) add a short note showing Vitest equivalents (vi.fn / vi.spyOn) and mention
the differing mock.instances semantics, or (3) provide a brief setup note for
making examples cross-compatible, referencing the mock(), spyOn(), and
mock.instances symbols so readers can find and adjust the examples accordingly.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1672-1703: The toHaveBeenCalled and toHaveReturned matchers
currently ignore AArgs allowing accidental arguments; update
TGocciaExpectationValue.ToHaveBeenCalled (and the analogous ToHaveReturned
implementation) to first verify that AArgs.Count = 0 and if not call
TGocciaTestAssertions(FTestAssertions).AssertionFailed with a clear message like
"toHaveBeenCalled/toHaveReturned take no arguments" and return
TGocciaUndefinedLiteralValue.UndefinedValue immediately; leave the existing
mock/return checks untouched for the zero-argument case so behavior only changes
when extra args are provided.
- Around line 1944-1946: Replace hardcoded property name strings 'type' and
'value' used when accessing mock result properties with the centralized runtime
constants in Goccia.Constants.PropertyNames: change calls like
ResultObj.GetProperty('type') and comparisons against
TGocciaStringLiteralValue.Value being 'return' or other literals to use
Goccia.Constants.PropertyNames.Type and Goccia.Constants.PropertyNames.Value (or
the exact field names in PropertyNames) and compare against the same centralized
constants instead of literal strings; update all occurrences referenced (e.g.,
the checks around ResultType := ResultObj.GetProperty(...), other
ResultObj.GetProperty(...) uses, and TGocciaStringLiteralValue.Value comparisons
in the matcher code) so every access uses PropertyNames instead of hardcoded
'type'/'value'.
- Around line 3066-3075: The MockFunction currently accepts non-function or
extra arguments and silently returns a default mock; change
TGocciaTestAssertions.MockFunction to validate arguments strictly: require
exactly one argument and that AArgs.GetElement(0) is a TGocciaFunctionBase,
otherwise raise an explicit error/exception (with a clear message) instead of
returning a nil-backed TGocciaMockFunctionValue; ensure callers still get
TGocciaMockFunctionValue when valid, and use the function/class names
TGocciaTestAssertions.MockFunction and TGocciaMockFunctionValue to locate and
update the logic.
In `@units/Goccia.Values.MockFunction.pas`:
- Around line 96-105: Replace the file-local mock property name constants
(MOCK_DEFAULT_NAME, RESULT_TYPE_RETURN, RESULT_TYPE_THROW, PROP_TYPE,
PROP_VALUE, PROP_CALLS, PROP_RESULTS, PROP_INSTANCES, PROP_LAST_CALL and the
similar constants around lines 167-198) with the shared runtime constants from
Goccia.Constants.PropertyNames; remove the duplicated string literals in
units/Goccia.Values.MockFunction.pas and reference the appropriate identifiers
exported by Goccia.Constants.PropertyNames (e.g., the mockImplementation/mock,
calls, results, value, lastCall names) wherever those local constants are used
in functions or classes in this unit so the code uses the centralized constants
instead of file-local copies.
- Around line 25-28: Replace the two separate FIFO lists FImplementationOnce and
FReturnValuesOnce with a single ordered once-queue and make the dispatch and
enqueue methods operate on that queue: introduce a single field (e.g.
FOnceQueue) that stores tagged entries for either a return-value or an
implementation; update mockReturnValueOnce and mockImplementationOnce to enqueue
tagged entries into FOnceQueue preserving call order; change the call dispatch
logic (the function that currently drains FImplementationOnce before
FReturnValuesOnce) to pop the next entry from FOnceQueue and either return the
stored value or execute the stored implementation accordingly; remove usage of
FImplementationOnce and FReturnValuesOnce and ensure FDefaultReturnValue /
FHasDefaultReturnValue remain unchanged for fallback behavior.
---
Duplicate comments:
In `@AGENTS.md`:
- Line 527: The documentation bullet incorrectly directs matcher coverage to the
JS test directory; remove the reference to tests/language/testing from that
sentence and replace it with a clear statement that TGocciaExpectationValue
matchers are exercised in the unit test Goccia.Builtins.TestAssertions.Test (the
Pascal unit) rather than via JavaScript end-to-end tests, ensuring the doc
references the TGocciaExpectationValue matcher symbol and the
Goccia.Builtins.TestAssertions.Test unit to guide contributors to the correct
test layer.
In `@units/Goccia.Values.MockFunction.pas`:
- Around line 221-246: BuildCallsArray, BuildResultsArray and
BuildInstancesArray create TGocciaArrayValue instances that can be collected
before they are attached; wrap the newly created array with AddTempRoot
immediately after creation and ensure the matching RemoveTempRoot is called in
the caller right after the array is assigned/attached to its owner (the place
that currently assigns these returned arrays onto the descriptor/result). Use
AddTempRoot(Result) in each of
BuildCallsArray/BuildResultsArray/BuildInstancesArray right after
TGocciaArrayValue.Create and remove the temp root with RemoveTempRoot at the
attachment site so the arrays are protected during the allocation window.
- Around line 309-319: The Exception catch currently stores E.Message as a
string (TGocciaStringLiteralValue.Create), which flattens native JS Error
objects; instead replace that string-wrapping with the runtime error helper that
builds a proper JS Error object and pass that into
CreateResultEntry(RESULT_TYPE_THROW, ...). Update the except on Exception block
that follows TGocciaThrowValue and the similar handlers around the 340-350 area
so they use the project's error helper (the function that constructs JS Error
values) rather than TGocciaStringLiteralValue.Create(E.Message), keeping the
TGocciaThrowValue branch unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df305a1e-5a16-4ae2-8974-0d8d3fe87f0d
📒 Files selected for processing (4)
AGENTS.mddocs/testing.mdunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Values.MockFunction.pas
- Enforce zero-argument arity on toHaveBeenCalled and toHaveReturned - Validate non-negative integer for toHaveBeenCalledTimes, toHaveReturnedTimes - Validate positive integer index for toHaveBeenNthCalledWith, toHaveNthReturnedWith - Document mock()/spyOn() as GocciaScript-specific in Vitest divergences table - Clarify mock.instances semantic difference vs Vitest/Jest Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Move mock property names (calls, results, instances, lastCall, type) into Goccia.Constants.PropertyNames; remove local duplicates - Replace hardcoded 'type'/'value' strings in matchers with shared constants - Merge FImplementationOnce and FReturnValuesOnce into a single FIFO queue so interleaved mockImplementationOnce/mockReturnValueOnce respect call order - Fail fast when mock() receives a non-function argument Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
units/Goccia.Builtins.TestAssertions.pas (2)
1961-1964: 🛠️ Refactor suggestion | 🟠 MajorCentralize the mock-result field names.
These return matchers still hardcode
'type'and'value'. Please route them throughGoccia.Constants.PropertyNamesinstead of duplicating the public field names here, adding constants if needed.As per coding guidelines, "Use runtime constants from split constant units instead of hardcoded string literals:
Goccia.Constants.PropertyNames(PROP_LENGTH, PROP_NAME, etc.),Goccia.Constants.TypeNames,Goccia.Constants.ErrorNames,Goccia.Constants.ConstructorNames,Goccia.Constants.SymbolNames, andGoccia.Constants."Also applies to: 2019-2022, 2071-2077, 2133-2139, 2202-2208
🤖 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 1961 - 1964, Replace hardcoded field names 'type' and 'value' by referencing the central constants in Goccia.Constants.PropertyNames (e.g. use PropertyNames.PROP_TYPE and PropertyNames.PROP_VALUE or add those constants if missing) wherever ResultObj.GetProperty('type') and property accessors compare against TGocciaStringLiteralValue with 'return'/'value' (symbols to change: ResultObj, ResultType, TGocciaStringLiteralValue); apply the same replacement to the other occurrences noted (around the blocks at the other ranges) so all mock-result field name lookups use Goccia.Constants.PropertyNames instead of string literals.
3096-3105:⚠️ Potential issue | 🟠 MajorFail fast on invalid
mock()arguments.
mock(123)andmock(fn, extra)still quietly create a default undefined-return mock here. That hides setup mistakes and weakens the resulting test failures.🩹 Suggested fix
function TGocciaTestAssertions.MockFunction(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var Impl: TGocciaValue; begin - if (AArgs.Length > 0) and (AArgs.GetElement(0) is TGocciaFunctionBase) then - Impl := AArgs.GetElement(0) - else - Impl := nil; + if AArgs.Length > 1 then + ThrowError('mock expects at most one implementation argument', 0, 0); + + if AArgs.Length = 1 then + begin + if not (AArgs.GetElement(0) is TGocciaFunctionBase) then + ThrowError('mock expects an optional function implementation', 0, 0); + Impl := AArgs.GetElement(0); + end + else + Impl := nil; Result := TGocciaMockFunctionValue.Create(Impl); 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 3096 - 3105, The MockFunction currently accepts any input and silently returns a default mock; update TGocciaTestAssertions.MockFunction to validate arguments: require exactly one argument and that AArgs.GetElement(0) is a TGocciaFunctionBase, and if not raise a clear exception (e.g. EArgumentException or a test-assertion specific exception) with a message like "mock expects a single function argument"; keep using TGocciaMockFunctionValue.Create(Impl) when validation passes. This enforces fail-fast behavior for invalid usages such as mock(123) or mock(fn, extra).
🤖 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.pas`:
- Around line 2175-2179: The current arity check for toHaveNthReturnedWith only
enforces a minimum (AArgs.Length < 2) allowing extra trailing args; replace this
with an exact-arity validation using TGocciaArgumentValidator from
Goccia.Arguments.Validator.pas to require exactly 2 arguments before proceeding
(instead of the current TGocciaTestAssertions.ThrowError branch), so that calls
like toHaveNthReturnedWith(1,42,extra) are rejected; ensure the validator raises
or triggers the same error flow used elsewhere so the function still returns
TGocciaUndefinedLiteralValue.UndefinedValue on failure.
- Around line 1726-1731: The guard blocks in the matchers
(toHaveBeenCalledTimes, toHaveBeenNthCalledWith, toHaveReturnedTimes,
toHaveNthReturnedWith) currently reject NaN/Infinity/negative/non-integers but
not values > High(Integer), which can cause RRangeError or silent wrap when
assigning Trunc(NumVal.Value) to an Integer; add an upper-bound check
(NumVal.Value > High(Integer)) to those guards before assigning
ExpectedTimes/Index and call TGocciaTestAssertions.ThrowError with an
appropriate message ("toHaveBeenCalledTimes expects a non-negative integer" for
counts, and "toHaveBeenNthCalledWith/toHaveNthReturnedWith expects a positive
integer index" for 1-based indexes) so any out-of-range values are rejected
early.
---
Duplicate comments:
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 1961-1964: Replace hardcoded field names 'type' and 'value' by
referencing the central constants in Goccia.Constants.PropertyNames (e.g. use
PropertyNames.PROP_TYPE and PropertyNames.PROP_VALUE or add those constants if
missing) wherever ResultObj.GetProperty('type') and property accessors compare
against TGocciaStringLiteralValue with 'return'/'value' (symbols to change:
ResultObj, ResultType, TGocciaStringLiteralValue); apply the same replacement to
the other occurrences noted (around the blocks at the other ranges) so all
mock-result field name lookups use Goccia.Constants.PropertyNames instead of
string literals.
- Around line 3096-3105: The MockFunction currently accepts any input and
silently returns a default mock; update TGocciaTestAssertions.MockFunction to
validate arguments: require exactly one argument and that AArgs.GetElement(0) is
a TGocciaFunctionBase, and if not raise a clear exception (e.g.
EArgumentException or a test-assertion specific exception) with a message like
"mock expects a single function argument"; keep using
TGocciaMockFunctionValue.Create(Impl) when validation passes. This enforces
fail-fast behavior for invalid usages such as mock(123) or mock(fn, extra).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf6a2088-7db4-4937-b75b-ed39ff6fef67
📒 Files selected for processing (2)
docs/testing.mdunits/Goccia.Builtins.TestAssertions.pas
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
units/Goccia.Builtins.TestAssertions.pas (3)
3096-3112:⚠️ Potential issue | 🟡 MinorReject extra
mock()arguments instead of dropping them.Lines 3100-3108 only validate the first argument.
mock(fn, extra)currently creates a valid mock and silently discards the trailing argument, which makes mistyped tests harder to diagnose.🩹 Minimal fix
begin + if AArgs.Length > 1 then + begin + ThrowError('mock() expects at most one function argument', 0, 0); + Exit(TGocciaUndefinedLiteralValue.UndefinedValue); + end; + if AArgs.Length > 0 then begin if not (AArgs.GetElement(0) is TGocciaFunctionBase) then🤖 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 3096 - 3112, TGocciaTestAssertions.MockFunction currently only validates the first argument and silently ignores any extras; change the validation to reject calls with more than one argument by checking AArgs.Length > 1 and calling ThrowError (e.g., "mock() expects at most one argument or a function") then Exit with TGocciaUndefinedLiteralValue.UndefinedValue; keep the existing function-type check for the first element (AArgs.GetElement(0) is TGocciaFunctionBase) and only use Impl := AArgs.GetElement(0) when exactly one valid argument is present.
1726-1731:⚠️ Potential issue | 🟠 MajorReject oversized matcher numbers before
Trunc.Lines 1726-1731, 1891-1896, and 2008-2013 still allow large finite values through.
Trunc(...)returns anInt64; assigning that into theseIntegerlocals can raiseERangeErrorwith range checks enabled or wrap silently without them. Add aNumVal.Value > High(Integer)guard before truncating in each matcher.Also applies to: 1891-1896, 2008-2013
🤖 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 1726 - 1731, The matcher currently truncates NumVal.Value into ExpectedTimes (an Integer) without guarding for oversized finite values, which can raise ERangeError or wrap; before calling Trunc for the toHaveBeenCalledTimes matcher (where NumVal, ExpectedTimes and TGocciaTestAssertions.ThrowError are used) add a check that NumVal.Value <= High(Integer) and if not call ThrowError('toHaveBeenCalledTimes expects a non-negative integer', 0, 0); apply the same pre-Trunc High(Integer) guard to the other two matcher sites that use NumVal/ExpectedTimes (the other occurrences around the same logic) so large finite values are rejected before Trunc.
2175-2188:⚠️ Potential issue | 🟠 MajorTighten
toHaveNthReturnedWithargument validation.Lines 2175-2188 still have two gaps: trailing arguments are silently ignored, and oversized finite indexes still reach
Trunc.expect(fn).toHaveNthReturnedWith(1, 42, extra)currently degrades into a weaker assertion, and huge indexes can still overflow theIntegerassignment.🩹 Minimal fix
- if AArgs.Length < 2 then + if AArgs.Length <> 2 then begin TGocciaTestAssertions(FTestAssertions).ThrowError( - 'toHaveNthReturnedWith requires at least 2 arguments (call index and expected value)', 0, 0); + 'toHaveNthReturnedWith requires exactly 2 arguments (call index and expected value)', 0, 0); Exit(TGocciaUndefinedLiteralValue.UndefinedValue); end; @@ NumVal := AArgs.GetElement(0).ToNumberLiteral; if NumVal.IsNaN or NumVal.IsInfinity or NumVal.IsNegativeInfinity or - (NumVal.Value < 1) or (Frac(NumVal.Value) <> 0) then + (NumVal.Value < 1) or (Frac(NumVal.Value) <> 0) or + (NumVal.Value > High(Integer)) then TGocciaTestAssertions(FTestAssertions).ThrowError( 'toHaveNthReturnedWith expects a positive integer index', 0, 0);🤖 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 2175 - 2188, The current validation for toHaveNthReturnedWith lets extra arguments pass and allows very large finite indexes to be truncated into N causing overflow; after the existing checks add a guard that AArgs.Length must be exactly 2 (throw via TGocciaTestAssertions.ThrowError if >2) so trailing arguments produce an error, and validate NumVal.Value is <= High(Integer) (and >= 1 is already checked) before calling N := Trunc(NumVal.Value); use the same ThrowError call for out‑of‑range indexes so oversized but finite indexes are rejected instead of being truncated.units/Goccia.Values.MockFunction.pas (2)
137-147:⚠️ Potential issue | 🟠 MajorPreserve the original descriptor shape for spies.
Line 146 always installs the spy as a writable/enumerable data property, and Lines 510–512 restore another synthesized data descriptor from
FSpyOriginalValue. That still flattens own accessors into plain values and loses the original getter/setter behavior plus descriptor shape. Since you already captureFSpyOriginalDescriptor, reapply that descriptor (or an exact clone of it) instead of rebuildingTGocciaPropertyDescriptorData.Also applies to: 508-515
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.MockFunction.pas` around lines 137 - 147, The spy currently replaces methods by always installing a synthesized data descriptor via ATarget.DefineProperty(..., TGocciaPropertyDescriptorData.Create(...)) which flattens accessors and loses the original descriptor; instead, when restoring or installing the spy, preserve and reuse the captured FSpyOriginalDescriptor (or a deep clone of it) if FSpyWasOwnProperty is true, applying that descriptor back to ATarget (and when installing the spy keep the original descriptor's attributes like getter/setter/flags rather than forcing pfWritable/pfEnumerable/pfConfigurable); update the code paths that set the spy (near FSpyOriginalDescriptor/FSpyWasOwnProperty and the restore logic around FSpyOriginalValue) to reapply the original descriptor shape via ATarget.DefineProperty using the original/cloned descriptor when present.
159-193: 🛠️ Refactor suggestion | 🟠 MajorMove the remaining mock API property names into
Goccia.Constants.PropertyNames.This unit still hardcodes JS-visible property keys like
mockImplementation,mockReturnValueOnce,mockReset, andmockeven though it already imports the shared property-name constants unit. Centralizing these keeps the mock API, matchers, and builtins from drifting again. As per coding guidelines, useGoccia.Constants.PropertyNamesconstants (PROP_LENGTH,PROP_NAME,PROP_CONSTRUCTOR, etc.) instead of hardcoded string literals for property names.
🤖 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.Values.MockFunction.pas`:
- Around line 233-240: The code currently conflates constructor-created
instances and plain call receivers by using FInstances for both; create a
separate list field (e.g., FContexts: TList) to track plain call receivers and
leave FInstances to only store objects created by the constructor, update the
code that currently appends AThisValue to FInstances to append to FContexts
instead, ensure constructor path pushes newly constructed objects into
FInstances, and optionally add a BuildContextsArray method mirroring
BuildInstancesArray to expose mock.contexts while leaving BuildInstancesArray
unchanged to return only constructor-created instances.
---
Duplicate comments:
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 3096-3112: TGocciaTestAssertions.MockFunction currently only
validates the first argument and silently ignores any extras; change the
validation to reject calls with more than one argument by checking AArgs.Length
> 1 and calling ThrowError (e.g., "mock() expects at most one argument or a
function") then Exit with TGocciaUndefinedLiteralValue.UndefinedValue; keep the
existing function-type check for the first element (AArgs.GetElement(0) is
TGocciaFunctionBase) and only use Impl := AArgs.GetElement(0) when exactly one
valid argument is present.
- Around line 1726-1731: The matcher currently truncates NumVal.Value into
ExpectedTimes (an Integer) without guarding for oversized finite values, which
can raise ERangeError or wrap; before calling Trunc for the
toHaveBeenCalledTimes matcher (where NumVal, ExpectedTimes and
TGocciaTestAssertions.ThrowError are used) add a check that NumVal.Value <=
High(Integer) and if not call ThrowError('toHaveBeenCalledTimes expects a
non-negative integer', 0, 0); apply the same pre-Trunc High(Integer) guard to
the other two matcher sites that use NumVal/ExpectedTimes (the other occurrences
around the same logic) so large finite values are rejected before Trunc.
- Around line 2175-2188: The current validation for toHaveNthReturnedWith lets
extra arguments pass and allows very large finite indexes to be truncated into N
causing overflow; after the existing checks add a guard that AArgs.Length must
be exactly 2 (throw via TGocciaTestAssertions.ThrowError if >2) so trailing
arguments produce an error, and validate NumVal.Value is <= High(Integer) (and
>= 1 is already checked) before calling N := Trunc(NumVal.Value); use the same
ThrowError call for out‑of‑range indexes so oversized but finite indexes are
rejected instead of being truncated.
In `@units/Goccia.Values.MockFunction.pas`:
- Around line 137-147: The spy currently replaces methods by always installing a
synthesized data descriptor via ATarget.DefineProperty(...,
TGocciaPropertyDescriptorData.Create(...)) which flattens accessors and loses
the original descriptor; instead, when restoring or installing the spy, preserve
and reuse the captured FSpyOriginalDescriptor (or a deep clone of it) if
FSpyWasOwnProperty is true, applying that descriptor back to ATarget (and when
installing the spy keep the original descriptor's attributes like
getter/setter/flags rather than forcing pfWritable/pfEnumerable/pfConfigurable);
update the code paths that set the spy (near
FSpyOriginalDescriptor/FSpyWasOwnProperty and the restore logic around
FSpyOriginalValue) to reapply the original descriptor shape via
ATarget.DefineProperty using the original/cloned descriptor when present.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: edb8d657-4f93-43af-9e3c-57846373e95c
📒 Files selected for processing (3)
units/Goccia.Builtins.TestAssertions.pasunits/Goccia.Constants.PropertyNames.pasunits/Goccia.Values.MockFunction.pas
…ntexts - Add High(Integer) bound check before Trunc in all count/index matchers to prevent range-check errors on large values - Use RequireExactly for toHaveNthReturnedWith instead of minimum arity - Rename FInstances to FContexts (tracks all-call this values as mock.contexts); mock.instances is now an empty array per Vitest/Jest semantics Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
units/Goccia.Builtins.TestAssertions.pas (1)
3092-3108:⚠️ Potential issue | 🟡 MinorStill reject trailing
mock()arguments.Line 3096 only validates the first argument.
mock(fn, extra)still creates a usable mock, which makes call-site mistakes much harder to spot.🩹 Proposed fix
function TGocciaTestAssertions.MockFunction(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; var Impl: TGocciaValue; begin + if AArgs.Length > 1 then + begin + ThrowError('mock() expects at most one implementation argument', 0, 0); + Exit(TGocciaUndefinedLiteralValue.UndefinedValue); + end; + if AArgs.Length > 0 then begin if not (AArgs.GetElement(0) is TGocciaFunctionBase) then begin ThrowError('mock() expects a function argument or no arguments', 0, 0);As per coding guidelines,
units/Goccia.Builtins.*.pas: useTGocciaArgumentValidatorfromGoccia.Arguments.Validator.pasfor built-in argument validation.🤖 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 3092 - 3108, The MockFunction currently only validates the first argument allowing extra trailing arguments; update TGocciaTestAssertions.MockFunction to use TGocciaArgumentValidator (from Goccia.Arguments.Validator.pas) to enforce an exact arity of 0 or 1 and that the single argument, if present, is a TGocciaFunctionBase, and throw the existing ThrowError when validation fails; locate the function TGocciaTestAssertions.MockFunction and replace the manual AArgs.Length check with a TGocciaArgumentValidator call that enforces the allowed signatures and then proceed to assign Impl and return TGocciaMockFunctionValue as before.units/Goccia.Values.MockFunction.pas (2)
138-147:⚠️ Potential issue | 🔴 CriticalPersist an owned copy of the original descriptor before installing the spy.
GetOwnPropertyDescriptor()returns the descriptor object owned byATarget, andDefineProperty()frees that object when the spy is installed. That makesFSpyOriginalDescriptordangling by the timemockRestore()readsFlags. The restore path also rehydrates everything as a data descriptor fromFSpyOriginalValue, so own accessors are still restored as plain data properties. Save the descriptor kind and fields (or an owned clone) before redefining the property, and restore that saved descriptor verbatim.Also applies to: 509-513
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.MockFunction.pas` around lines 138 - 147, FSpyOriginalDescriptor is assigned the descriptor returned by ATarget.GetOwnPropertyDescriptor but that object is owned by ATarget and will be freed when ATarget.DefineProperty is called, leaving FSpyOriginalDescriptor dangling and losing accessor metadata; modify the spy install path (around FSpyOriginalDescriptor := ATarget.GetOwnPropertyDescriptor(AMethodName) and before ATarget.DefineProperty) to create and store an owned copy/clone of the descriptor (or at minimum copy its Kind, Getter, Setter, Value, and Flags into fields) so mockRestore can verbatim reapply the original descriptor (including accessor vs data kind) using those saved fields rather than relying on the original object or only FSpyOriginalValue.
161-192: 🛠️ Refactor suggestion | 🟠 MajorUse shared property-name constants for the remaining mock API strings.
This unit still hardcodes JS-visible property names like
'mockImplementation','mockReset','mock', and'contexts'even though it already importsGoccia.Constants.PropertyNames. Please add constants for the remaining names and reuse them here so the testing surface stays centralized.As per coding guidelines: Use
Goccia.Constants.PropertyNamesconstants (PROP_LENGTH,PROP_NAME,PROP_CONSTRUCTOR,PROP_PROTOTYPE, etc.) instead of hardcoded string literals for property names.Also applies to: 253-253
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.Values.MockFunction.pas` around lines 161 - 192, Replace the hardcoded JS-visible property name string literals used in the DefineProperty calls (e.g., 'mockImplementation', 'mockImplementationOnce', 'mockReturnValue', 'mockReturnValueOnce', 'mockReset', 'mockClear', 'mockRestore', 'mockName', 'getMockName', 'mock' and any other remaining literals such as 'contexts') with the centralized constants from Goccia.Constants.PropertyNames; update the DefineProperty and DefinePropertyAccessor calls that create TGocciaPropertyDescriptorData/TGocciaPropertyDescriptorAccessor and TGocciaNativeFunctionValue (functions DoMockImplementation, DoMockImplementationOnce, DoMockReturnValue, DoMockReturnValueOnce, DoMockReset, DoMockClear, DoMockRestore, DoMockName, DoGetMockName, DoGetMockProperty) to use those PROPERTY_* constants instead of literal strings so the JS-visible names are reused from PropertyNames.
🤖 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.pas`:
- Around line 3134-3144: ExistingValue may be nil for missing slots, which can
cause the "is not a function" branch to fire instead of the "non-existent
property" error; update the spyOn lookup to detect missing properties before the
non-callable check by testing for a nil/nullable value returned from
Target.GetProperty (e.g., if ExistingValue = nil then ThrowError('spyOn: cannot
spy on non-existent property "' + MethodName + '"', 0, 0) and
Exit(TGocciaUndefinedLiteralValue.UndefinedValue)); keep the existing
TGocciaUndefinedLiteralValue and TGocciaFunctionBase checks intact but ensure
the nil check occurs immediately after retrieving ExistingValue so ThrowError is
raised for truly missing slots.
In `@units/Goccia.Values.MockFunction.pas`:
- Around line 308-319: The current exception handler records
non-TGocciaThrowValue exceptions as TGocciaStringLiteralValue(E.Message), which
loses original JS Error objects; add a specific "on E: TGocciaError do" branch
before the generic "on E: Exception do" in the same try/except blocks that call
FResults.Add(CreateResultEntry(RESULT_TYPE_THROW, ...)) so you record E.Value
(preserving the original error object) for TGocciaError cases; apply this same
change to the other similar handler later in the file to keep
mock.results[*].value as the original error objects.
---
Duplicate comments:
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 3092-3108: The MockFunction currently only validates the first
argument allowing extra trailing arguments; update
TGocciaTestAssertions.MockFunction to use TGocciaArgumentValidator (from
Goccia.Arguments.Validator.pas) to enforce an exact arity of 0 or 1 and that the
single argument, if present, is a TGocciaFunctionBase, and throw the existing
ThrowError when validation fails; locate the function
TGocciaTestAssertions.MockFunction and replace the manual AArgs.Length check
with a TGocciaArgumentValidator call that enforces the allowed signatures and
then proceed to assign Impl and return TGocciaMockFunctionValue as before.
In `@units/Goccia.Values.MockFunction.pas`:
- Around line 138-147: FSpyOriginalDescriptor is assigned the descriptor
returned by ATarget.GetOwnPropertyDescriptor but that object is owned by ATarget
and will be freed when ATarget.DefineProperty is called, leaving
FSpyOriginalDescriptor dangling and losing accessor metadata; modify the spy
install path (around FSpyOriginalDescriptor :=
ATarget.GetOwnPropertyDescriptor(AMethodName) and before ATarget.DefineProperty)
to create and store an owned copy/clone of the descriptor (or at minimum copy
its Kind, Getter, Setter, Value, and Flags into fields) so mockRestore can
verbatim reapply the original descriptor (including accessor vs data kind) using
those saved fields rather than relying on the original object or only
FSpyOriginalValue.
- Around line 161-192: Replace the hardcoded JS-visible property name string
literals used in the DefineProperty calls (e.g., 'mockImplementation',
'mockImplementationOnce', 'mockReturnValue', 'mockReturnValueOnce', 'mockReset',
'mockClear', 'mockRestore', 'mockName', 'getMockName', 'mock' and any other
remaining literals such as 'contexts') with the centralized constants from
Goccia.Constants.PropertyNames; update the DefineProperty and
DefinePropertyAccessor calls that create
TGocciaPropertyDescriptorData/TGocciaPropertyDescriptorAccessor and
TGocciaNativeFunctionValue (functions DoMockImplementation,
DoMockImplementationOnce, DoMockReturnValue, DoMockReturnValueOnce, DoMockReset,
DoMockClear, DoMockRestore, DoMockName, DoGetMockName, DoGetMockProperty) to use
those PROPERTY_* constants instead of literal strings so the JS-visible names
are reused from PropertyNames.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e03974b-a94c-427e-a062-5302245b3978
📒 Files selected for processing (3)
docs/testing.mdunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Values.MockFunction.pas
- Add explicit TGocciaError handler in mock Call dispatch to preserve the original JS error object via CreateErrorObject instead of degrading to E.Message string - Use HasProperty before GetProperty in spyOn to distinguish missing properties from non-callable ones with the correct error message Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 18, 2026
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
mock()andspyOn()test globals with call tracking, configurable implementations, return-value helpers, and restore/reset behavior.toHaveBeenCalled,toHaveBeenCalledWith, andtoHaveReturnedWith.u-flag status.Fixes #119
Testing
units/Goccia.Builtins.TestAssertions.Test.pasfor mock behavior, spies, and matcher failures.docs/testing.mdundertests/language/testing/.