Add Vitest-style matcher support for test assertions#164
Merged
Conversation
- Add `toStrictEqual`, `toContainEqual`, `toMatchObject`, and `toMatch` - Implement partial deep equality for matcher comparisons - Update docs and unit tests for the new assertion surface
Contributor
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds four new test assertion matchers— Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
Actionable comments posted: 2
🧹 Nitpick comments (4)
units/Goccia.Builtins.TestAssertions.pas (1)
206-213: Prefer member-definition tables when extending built-in surfaces.The new matcher property registrations continue the ad hoc
DefinePropertypattern. Please consider migrating matcher registration to Goccia object-model member-definition tables when touching this surface further.As per coding guidelines: “Prefer Goccia-specific member-definition tables (Goccia.ObjectModel) plus RegisterMemberDefinitions over ad hoc RegisterNativeMethod blocks when adding/refactoring prototype/static surfaces.”
🤖 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 206 - 213, The new matcher registrations use ad hoc DefineProperty calls for ToContainEqual, ToStrictEqual, ToMatchObject and ToMatch; convert these to the Goccia object-model member-definition approach by creating a Goccia.ObjectModel member-definition table that maps the matcher names to TGocciaNativeFunctionValue wrappers and then call RegisterMemberDefinitions (instead of calling DefineProperty directly) to attach them to the appropriate prototype/static surface; ensure the property attributes (pfConfigurable, pfWritable) are preserved in the member-definition entries and reference the existing TGocciaNativeFunctionValue constructors (e.g., ToContainEqual, ToStrictEqual, ToMatchObject, ToMatch) when building the table.units/Goccia.Builtins.TestAssertions.Test.pas (3)
485-545: Consider adding negation test fortoContainEqual.The implementation tests success, failure, and type-error cases, which is good. However, other matchers in this file (e.g.,
toBeNull,toEqual) include negation tests (not.toContainEqual). Adding a negation test would improve parity with existing coverage patterns.Example negation test
procedure TTestExpectationMatchers.TestToContainEqualNegated; var A: TGocciaArgumentsCollection; ActualArray: TGocciaArrayValue; ExpectedObject: TGocciaObjectValue; begin ActualArray := TGocciaArrayValue.Create; ActualArray.Elements.Add(TGocciaNumberLiteralValue.Create(1)); ExpectedObject := TGocciaObjectValue.Create; ExpectedObject.AssignProperty('id', TGocciaNumberLiteralValue.Create(99)); A := TGocciaArgumentsCollection.Create([ExpectedObject]); try ExpectPass(MakeExpectation(ActualArray, True).ToContainEqual(A, nil)); finally A.Free; end; 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 485 - 545, Add a negation unit test for toContainEqual to match existing matcher coverage: create a new procedure TestToContainEqualNegated that builds an ActualArray (e.g., with a number element), an ExpectedObject with a non-matching property, wrap ExpectedObject in TGocciaArgumentsCollection, then call ExpectPass(MakeExpectation(ActualArray, True).ToContainEqual(A, nil)); make sure to free A and any created values and register the new test method alongside other TTestExpectationMatchers tests (reference symbols: TestToContainEqualArrayValue, TestToContainEqualArrayMissing, TestToContainEqualOnNonArray, MakeExpectation, ToContainEqual, TGocciaArgumentsCollection).
598-683: Consider adding negation test fortoMatchObject.Similar to the
toContainEqualcomment, thetoMatchObjecttests cover success, nested success, failure, and type-error cases well, but lack a negation test fornot.toMatchObject. This would align with the test coverage pattern used for other matchers liketoBeNullandtoStrictEqual.🤖 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 598 - 683, Add negation coverage for ToMatchObject: create a new test (in TTestExpectationMatchers) that exercises the negative form of ToMatchObject via MakeExpectation(...).Not.ToMatchObject and asserts both outcomes—i) when Actual and Expected match, expect the negated matcher to fail (use ExpectFail), and ii) when Actual and Expected differ, expect the negated matcher to pass (use ExpectPass). Reuse the same object construction patterns from TestToMatchObject, TestToMatchObjectMismatch and TestToMatchObjectNestedSubset so the test targets ToMatchObject negation behavior consistently.
685-721: Consider adding negation test fortoMatch.The
toMatchtests cover the core cases effectively. Anot.toMatchnegation test would complete the coverage pattern consistent with other matchers in this file.🤖 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 685 - 721, Add a negation test method (e.g. TestToMatchNegation) that uses TGocciaArgumentsCollection and TGocciaStringLiteralValue with MakeExpectation(...).Not.ToMatch to assert negation behavior: create an args collection with 'world' and Assert ExpectFail(MakeExpectation(TGocciaStringLiteralValue.Create('hello world')).Not.ToMatch(A, nil)) for the case where the substring exists, and another case with a different arg (e.g. 'planet') asserting ExpectPass(MakeExpectation(TGocciaStringLiteralValue.Create('hello world')).Not.ToMatch(A, nil)); follow the same allocation/try/finally A.Free pattern as other TestToMatch functions.
🤖 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 318-503: Add JavaScript end-to-end tests under tests/ that
exercise the four new matchers: toStrictEqual, toContainEqual, toMatchObject,
and toMatch; for each matcher include (1) happy-path assertions that pass, (2)
type-validation error cases (e.g., passing non-array to toContainEqual,
non-object to toMatchObject, non-string to toMatch) and assert the runner
reports the expected failure, and (3) negation cases using not.toStrictEqual /
not.toContainEqual / not.toMatchObject / not.toMatch to ensure negated logic is
correct; mirror the scenarios covered in the Pascal unit tests
(units/Goccia.Builtins.TestAssertions.Test.pas) and place each matcher’s tests
in clearly named files in tests/ so the e2e test runner will pick them up.
In `@units/Goccia.Evaluator.Comparison.pas`:
- Around line 375-385: The loop that checks subset matching uses
ActualObj.HasProperty which traverses the prototype chain and can yield false
positives; change the ownership check to verify the property is an own property
(e.g., replace the HasProperty call with a HasOwn/OwnProperty-style check or
inspect property descriptor ownership) before calling ActualObj.GetProperty, so
in the for-loop over ExpectedKeys we only consider keys that are actually owned
by the actual object when invoking
IsPartialDeepEqual(ActualObj.GetProperty(Key), ExpectedObj.GetProperty(Key)).
---
Nitpick comments:
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Around line 206-213: The new matcher registrations use ad hoc DefineProperty
calls for ToContainEqual, ToStrictEqual, ToMatchObject and ToMatch; convert
these to the Goccia object-model member-definition approach by creating a
Goccia.ObjectModel member-definition table that maps the matcher names to
TGocciaNativeFunctionValue wrappers and then call RegisterMemberDefinitions
(instead of calling DefineProperty directly) to attach them to the appropriate
prototype/static surface; ensure the property attributes (pfConfigurable,
pfWritable) are preserved in the member-definition entries and reference the
existing TGocciaNativeFunctionValue constructors (e.g., ToContainEqual,
ToStrictEqual, ToMatchObject, ToMatch) when building the table.
In `@units/Goccia.Builtins.TestAssertions.Test.pas`:
- Around line 485-545: Add a negation unit test for toContainEqual to match
existing matcher coverage: create a new procedure TestToContainEqualNegated that
builds an ActualArray (e.g., with a number element), an ExpectedObject with a
non-matching property, wrap ExpectedObject in TGocciaArgumentsCollection, then
call ExpectPass(MakeExpectation(ActualArray, True).ToContainEqual(A, nil)); make
sure to free A and any created values and register the new test method alongside
other TTestExpectationMatchers tests (reference symbols:
TestToContainEqualArrayValue, TestToContainEqualArrayMissing,
TestToContainEqualOnNonArray, MakeExpectation, ToContainEqual,
TGocciaArgumentsCollection).
- Around line 598-683: Add negation coverage for ToMatchObject: create a new
test (in TTestExpectationMatchers) that exercises the negative form of
ToMatchObject via MakeExpectation(...).Not.ToMatchObject and asserts both
outcomes—i) when Actual and Expected match, expect the negated matcher to fail
(use ExpectFail), and ii) when Actual and Expected differ, expect the negated
matcher to pass (use ExpectPass). Reuse the same object construction patterns
from TestToMatchObject, TestToMatchObjectMismatch and
TestToMatchObjectNestedSubset so the test targets ToMatchObject negation
behavior consistently.
- Around line 685-721: Add a negation test method (e.g. TestToMatchNegation)
that uses TGocciaArgumentsCollection and TGocciaStringLiteralValue with
MakeExpectation(...).Not.ToMatch to assert negation behavior: create an args
collection with 'world' and Assert
ExpectFail(MakeExpectation(TGocciaStringLiteralValue.Create('hello
world')).Not.ToMatch(A, nil)) for the case where the substring exists, and
another case with a different arg (e.g. 'planet') asserting
ExpectPass(MakeExpectation(TGocciaStringLiteralValue.Create('hello
world')).Not.ToMatch(A, nil)); follow the same allocation/try/finally A.Free
pattern as other TestToMatch functions.
🪄 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: 09f0f184-259c-4179-8d49-18321d5be5fa
📒 Files selected for processing (6)
AGENTS.mddocs/built-ins.mddocs/testing.mdunits/Goccia.Builtins.TestAssertions.Test.pasunits/Goccia.Builtins.TestAssertions.pasunits/Goccia.Evaluator.Comparison.pas
Comment on lines
+318
to
+503
| function TGocciaExpectationValue.ToContainEqual(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Expected: TGocciaValue; | ||
| I: Integer; | ||
| Contains: Boolean; | ||
| begin | ||
| TGocciaArgumentValidator.RequireExactly(AArgs, 1, 'toContainEqual', FTestAssertions.ThrowError); | ||
|
|
||
| if not (FActualValue is TGocciaArrayValue) then | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toContainEqual') | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toContainEqual', | ||
| 'Expected an array but received ' + FActualValue.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| Exit; | ||
| end; | ||
|
|
||
| Expected := AArgs.GetElement(0); | ||
| Contains := False; | ||
| for I := 0 to TGocciaArrayValue(FActualValue).Elements.Count - 1 do | ||
| if IsDeepEqual(TGocciaArrayValue(FActualValue).Elements[I], Expected) then | ||
| begin | ||
| Contains := True; | ||
| Break; | ||
| end; | ||
|
|
||
| if FIsNegated then | ||
| Contains := not Contains; | ||
|
|
||
| if Contains then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toContainEqual'); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end | ||
| else | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toContainEqual', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' not to contain equal ' + Expected.ToStringLiteral.Value) | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toContainEqual', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' to contain equal ' + Expected.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; | ||
| end; | ||
|
|
||
| function TGocciaExpectationValue.ToStrictEqual(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Expected: TGocciaValue; | ||
| IsEqual: Boolean; | ||
| begin | ||
| TGocciaArgumentValidator.RequireExactly(AArgs, 1, 'toStrictEqual', FTestAssertions.ThrowError); | ||
|
|
||
| Expected := AArgs.GetElement(0); | ||
| IsEqual := IsDeepEqual(FActualValue, Expected); | ||
|
|
||
| if FIsNegated then | ||
| IsEqual := not IsEqual; | ||
|
|
||
| if IsEqual then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toStrictEqual'); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end | ||
| else | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toStrictEqual', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' not to strictly equal ' + Expected.ToStringLiteral.Value) | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toStrictEqual', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' to strictly equal ' + Expected.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; | ||
| end; | ||
|
|
||
| function TGocciaExpectationValue.ToMatchObject(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Expected: TGocciaValue; | ||
| Matches: Boolean; | ||
| begin | ||
| TGocciaArgumentValidator.RequireExactly(AArgs, 1, 'toMatchObject', FTestAssertions.ThrowError); | ||
|
|
||
| Expected := AArgs.GetElement(0); | ||
|
|
||
| if not (FActualValue is TGocciaObjectValue) then | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toMatchObject') | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatchObject', | ||
| 'Expected an object but received ' + FActualValue.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| Exit; | ||
| end; | ||
|
|
||
| if not (Expected is TGocciaObjectValue) then | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toMatchObject') | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatchObject', | ||
| 'Expected a match object but received ' + Expected.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| Exit; | ||
| end; | ||
|
|
||
| Matches := IsPartialDeepEqual(FActualValue, Expected); | ||
|
|
||
| if FIsNegated then | ||
| Matches := not Matches; | ||
|
|
||
| if Matches then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toMatchObject'); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end | ||
| else | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatchObject', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' not to match object ' + Expected.ToStringLiteral.Value) | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatchObject', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' to match object ' + Expected.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; | ||
| end; | ||
|
|
||
| function TGocciaExpectationValue.ToMatch(const AArgs: TGocciaArgumentsCollection; const AThisValue: TGocciaValue): TGocciaValue; | ||
| var | ||
| Expected: TGocciaValue; | ||
| ActualString: string; | ||
| ExpectedString: string; | ||
| Matches: Boolean; | ||
| begin | ||
| TGocciaArgumentValidator.RequireExactly(AArgs, 1, 'toMatch', FTestAssertions.ThrowError); | ||
|
|
||
| if not (FActualValue is TGocciaStringLiteralValue) then | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toMatch') | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatch', | ||
| 'Expected a string but received ' + FActualValue.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| Exit; | ||
| end; | ||
|
|
||
| Expected := AArgs.GetElement(0); | ||
| if not (Expected is TGocciaStringLiteralValue) then | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toMatch') | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatch', | ||
| 'Expected a string pattern but received ' + Expected.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| Exit; | ||
| end; | ||
|
|
||
| ActualString := FActualValue.ToStringLiteral.Value; | ||
| ExpectedString := Expected.ToStringLiteral.Value; | ||
| Matches := Pos(ExpectedString, ActualString) > 0; | ||
|
|
||
| if FIsNegated then | ||
| Matches := not Matches; | ||
|
|
||
| if Matches then | ||
| begin | ||
| TGocciaTestAssertions(FTestAssertions).AssertionPassed('toMatch'); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end | ||
| else | ||
| begin | ||
| if FIsNegated then | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatch', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' not to match ' + Expected.ToStringLiteral.Value) | ||
| else | ||
| TGocciaTestAssertions(FTestAssertions).AssertionFailed('toMatch', | ||
| 'Expected ' + FActualValue.ToStringLiteral.Value + ' to match ' + Expected.ToStringLiteral.Value); | ||
| Result := TGocciaUndefinedLiteralValue.UndefinedValue; | ||
| end; | ||
| end; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking JS/TS test usage of newly added matchers..."
rg -n --iglob 'tests/**/*.{js,jsx,ts,tsx,mjs}' '\.(toStrictEqual|toContainEqual|toMatchObject|toMatch)\(' || true
echo
echo "Checking native Pascal test coverage entries..."
rg -n 'ToStrictEqual|ToContainEqual|ToMatchObject|ToMatch' units/Goccia.Builtins.TestAssertions.Test.pas || true
echo
echo "Expected: matcher usage appears in tests/**/* for JS e2e coverage, not only in units/*.Test.pas."Repository: frostney/GocciaScript
Length of output: 3976
Add JavaScript end-to-end tests for the new matchers.
New matchers (toStrictEqual, toContainEqual, toMatchObject, toMatch) are covered by native Pascal unit tests in units/Goccia.Builtins.TestAssertions.Test.pas, but no JavaScript tests exist under tests/. JavaScript end-to-end tests are the primary testing layer for GocciaScript features. Add test cases under tests/ covering the same scenarios: happy paths (passing assertions), error cases (type validation), and negation (not.toXxx()).
🤖 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 318 - 503, Add
JavaScript end-to-end tests under tests/ that exercise the four new matchers:
toStrictEqual, toContainEqual, toMatchObject, and toMatch; for each matcher
include (1) happy-path assertions that pass, (2) type-validation error cases
(e.g., passing non-array to toContainEqual, non-object to toMatchObject,
non-string to toMatch) and assert the runner reports the expected failure, and
(3) negation cases using not.toStrictEqual / not.toContainEqual /
not.toMatchObject / not.toMatch to ensure negated logic is correct; mirror the
scenarios covered in the Pascal unit tests
(units/Goccia.Builtins.TestAssertions.Test.pas) and place each matcher’s tests
in clearly named files in tests/ so the e2e test runner will pick them up.
Owner
Author
There was a problem hiding this comment.
JavaScript end-to-end tests are the wrong abstraction layer for the test assertions. This is not a language feature in itself and we can't use the implementation to test the implementation.
Contributor
There was a problem hiding this comment.
@frostney, that's a completely valid point — testing the assertion matchers via JavaScript expect() calls would be circular, since the test runner itself depends on the very matchers being validated. The native Pascal unit tests in units/Goccia.Builtins.TestAssertions.Test.pas are clearly the right layer for this. I've noted this for future reviews.
✏️ Learnings added
Learnt from: frostney
Repo: frostney/GocciaScript PR: 164
File: units/Goccia.Builtins.TestAssertions.pas:318-503
Timestamp: 2026-04-02T04:42:34.022Z
Learning: In frostney/GocciaScript, the `TGocciaExpectationValue` matchers (e.g. `toBe`, `toEqual`, `toStrictEqual`, `toContainEqual`, `toMatchObject`, `toMatch`, and similar matchers in `units/Goccia.Builtins.TestAssertions.pas`) must NOT be tested via JavaScript end-to-end tests under `tests/`, because that would use the implementation to test itself (circular dependency). The correct and only appropriate testing layer for these matchers is the native Pascal unit tests in `units/Goccia.Builtins.TestAssertions.Test.pas`. Do not flag missing JS e2e test coverage for test-assertion matcher methods.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : All values inherit from `TGocciaValue`. Prefer VMT methods `GetProperty(Name)`, `SetProperty(Name, Value)`, `IsPrimitive`, and `IsCallable` over type-checking at call sites. Do not use `is` type checks for fundamental type-system properties.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.Test.pas : In Pascal tests, use `Value.ToNumberLiteral.IsNaN` (not `Math.IsNaN`) for checking NaN values, as special values store `0.0` internally.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T19:11:21.599Z
Learning: JavaScript end-to-end tests are the primary way of testing GocciaScript. Add JavaScript tests under tests/ for any new feature or bug fix, keeping tests isolated and grouped by feature/filename, following existing directory structure
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: JavaScript end-to-end tests are the primary way of testing GocciaScript. Always verify changes by running: `./build.pas testrunner && ./build/TestRunner tests`
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to tests/**/*.{js,jsx,ts,tsx,mjs} : JavaScript tests should cover happy paths, edge cases, and error cases. Keep tests isolated and grouped by feature/filename.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.Test.pas : When modifying AST logic, scope chain, evaluator, or value types, update the native Pascal tests in `units/*.Test.pas` to reflect any changes in behaviour (e.g. new parameters, changed return semantics). Both JavaScript tests and native Pascal tests must pass.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to units/**/*.Test.pas : Native Pascal unit tests in `units/*.Test.pas` must be updated whenever AST logic, scope chain, evaluator, or value types are modified. Both JavaScript tests and native Pascal tests must pass.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.694Z
Learning: Always verify JavaScript test changes by running `./build.pas testrunner && ./build/TestRunner tests`. Both JavaScript tests and native Pascal tests must pass when modifying internal implementation details.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to **/*.{js,jsx,ts,tsx,mjs} : When implementing a new JavaScript language feature, create test files under the `tests/` directory following the existing directory structure (`tests/language/` for language features, `tests/built-ins/` for built-in objects). Each test file should focus on a single concern.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T19:11:21.599Z
Learning: When modifying AST logic, scope chain, evaluator, or value types, build and run the native Pascal test suite: ./build.pas clean tests && for t in build/Goccia.*.Test; do "$t"; done. Update native tests in units/*.Test.pas to reflect changes
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T19:11:21.599Z
Learning: Add/update tests when implementing a new language feature following existing patterns in tests/. For AST logic, scope chain, evaluator, or value type changes, also update units/*.Test.pas
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-01T19:11:21.592Z
Learning: Applies to tests/**/*.js : Edge case tests (NaN, Infinity, negative indices, clamping, empty collections, boundary conditions) belong in the same file as the happy-path tests for that method, not in separate edge-cases.js files
Learnt from: frostney
Repo: frostney/GocciaScript PR: 147
File: units/Goccia.VM.pas:3042-3084
Timestamp: 2026-03-31T18:38:20.149Z
Learning: Repo: frostney/GocciaScript — units/Goccia.VM.pas (PR `#147`)
As of commit 457e712: (1) TGocciaVM.GetPropertyValue performs own-only private static accessor lookup on '#…' for TGocciaClassValue and throws TypeError on class-brand misses; (2) TGocciaVM.SetPropertyValue avoids inherited private accessor resolution, but still allows raw private static writes via SetRawPrivateValue to support current class initialization that emits OP_SET_PROP_CONST for private static fields. Planned follow-up is to add dedicated opcodes to define private static fields at class-declaration time and then make SetPropertyValue strict (throw on brand miss with no fallback). Do not re-flag the current fallback until those opcodes exist.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-08T04:38:51.695Z
Learning: Applies to **/Goccia.Engine.pas : Built-ins are registered by the engine via `TGocciaGlobalBuiltins` flags in `DefaultGlobals`. The TestRunner adds `ggTestAssertions` for `describe`/`test`/`expect`. The BenchmarkRunner adds `ggBenchmark` for `bench()` with setup/teardown.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : Built-ins are registered by the engine via `TGocciaGlobalBuiltins` flags. The TestRunner adds `ggTestAssertions` for the test framework. The BenchmarkRunner adds `ggBenchmark` for the benchmark framework.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:211-212
Timestamp: 2026-03-23T12:37:30.936Z
Learning: Repo: frostney/GocciaScript — units/Goccia.Runtime.Operations.pas (PR `#107`)
Learning: In TGocciaRuntimeOperations.MarkExternalRoots, entries in FModuleCache (cached module namespace/JSON records) must be traversed and their TSouffleValue references marked as external roots to survive GC. This behavior was added in commit 36f69cc; do not re‑flag missing FModuleCache rooting unless the logic regresses.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 58
File: units/Goccia.Runtime.Operations.pas:5959-6132
Timestamp: 2026-03-10T11:17:46.617Z
Learning: GocciaScript’s Promise statics are intentionally not species/subclass-aware. Interpreter-level implementations (PromiseAll, PromiseRace, PromiseAllSettled, PromiseAny, withResolvers, try, resolve, reject) construct plain TGocciaPromiseValue via WrapAsPromise and do not use the call receiver (AThisValue). The Souffle VM bridge mirrors this by not forwarding AReceiver and by constructing TGocciaPromiseValue directly where applicable. Do not flag missing receiver-based construction for Promise statics. This applies to units/Goccia.Runtime.Operations.pas and related Promise implementations; ensure consistency across related Pas files in the frostney/GocciaScript repository.
Learnt from: CR
Repo: frostney/GocciaScript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-20T22:00:59.812Z
Learning: Applies to units/**/*.pas : `type`/`interface` declarations and `import type`/`export type` must produce `TGocciaEmptyStatement` (no-op at runtime). Access modifiers (`public`, `protected`, `private`, `readonly`, `override`, `abstract`) in class bodies must be consumed and discarded.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:2797-2828
Timestamp: 2026-03-23T15:57:21.755Z
Learning: Repo frostney/GocciaScript — PR `#109`: Native Map/Set scaffolding
- Current behavior: ToSouffleValue wraps TGocciaMapValue/TGocciaSetValue as TGocciaWrappedValue; TGocciaSouffleMap/TGocciaSouffleSet instances are not produced in normal execution.
- Pre-wired fast paths: GetProperty, GetIterator, and IteratorNext recognize TGocciaSouffleMap/TGocciaSouffleSet and their iterator types; these are staged for future activation.
- Deferred work: When Construct is updated to produce native collection types, add corresponding fast paths in HasProperty, SetProperty, and DeleteProperty.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 87
File: units/Goccia.Runtime.Operations.pas:3383-3387
Timestamp: 2026-03-17T16:28:05.526Z
Learning: Repo: frostney/GocciaScript — Follow-up tracking
Learning: Splitting the blueprint bridge caches (value bridge vs construct bridge) is tracked in issue `#99`. Until that lands, sharing FBlueprintBridgeCache between UnwrapToGocciaValue (BlueprintToClassValue) and Construct (ConvertBlueprintToClassValue) should not block PRs; reference `#99` instead of re-flagging.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 100
File: tests/language/classes/blueprint-bridge-consistency.js:21-101
Timestamp: 2026-03-17T16:57:51.305Z
Learning: Repo: frostney/GocciaScript — tests/language/classes/
The `prototype.constructor` assertion for blueprint-bridged classes in bytecode mode is a pre-existing issue tracked in `#101`. Do not re-flag missing `prototype.constructor` checks in bridge-related tests as a new concern; reference `#101` instead.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 139
File: units/Goccia.ScriptLoader.JSON.Test.pas:1-113
Timestamp: 2026-03-30T13:02:29.132Z
Learning: In frostney/GocciaScript, CI workflows (`.github/workflows/ci.yml` and `.github/workflows/pr.yml`) use glob patterns (`build/Goccia.*.Test*` for artifact upload, `Goccia.*.Test` / `Goccia.*.Test.exe` for execution) to discover and run native Pascal test binaries automatically. Do not flag missing entries in a hard-coded test list — new `units/*.Test.pas` test programs are picked up automatically via these globs as of commit b63911d (PR `#139`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 53
File: souffle/Souffle.Wasm.Emitter.pas:1467-1477
Timestamp: 2026-03-09T17:21:38.193Z
Learning: In this codebase (Frostney/GocciaScript), for Pascal/Delphi (FPC) files using AnsiString with {$mode delphi} {H+}, treat String as byte-indexed. Length(str) returns byte count and Byte(str[I]) is a no-op cast. Do not replace per-byte copy loops like Data[I-1] := Byte(AStr[I]) with TEncoding.GetBytes, since TEncoding is a Delphi RTL feature unavailable in FPC's SysUtils and would break the build. Apply this guidance to all .pas files under the project; ensure manual per-byte copying logic remains correct for UTF-8 content encoded in AnsiString, and avoid introducing RTL dependencies not supported by FPC.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-11T01:50:11.071Z
Learning: Replace hardcoded string literals 'now', 'toJSON', and 'timeOrigin' with the new constants from Goccia.Constants.PropertyNames (PROP_NOW, PROP_TO_JSON, PROP_TIME_ORIGIN). Apply this refactor across the repository wherever these literals are used, including but not limited to units/Goccia.Runtime.Operations.pas and units/Goccia.Builtins.Performance.pas. Use a project-wide search (e.g., regex or code navigation) to identify all occurrences, update to the respective constants, and adjust imports or uses accordingly. Ensure all references compile and behavior remains unchanged.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 63
File: units/Goccia.Runtime.Operations.pas:7514-7544
Timestamp: 2026-03-11T08:22:20.482Z
Learning: When bridging a native namespace object to a TSouffleRecord (e.g., performance) in units/Goccia.Runtime.Operations.pas, always set Result.Delegate := ARuntime.VM.RecordDelegate after TSouffleRecord.Create(...). This ensures Object.prototype methods (toString/hasOwnProperty/valueOf/toLocaleString) work in bytecode mode and NativeRecordToString honors Symbol.toStringTag. Apply this pattern to BuildPerformanceRecord and any future bridged records.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/HashMap.pas:266-309
Timestamp: 2026-03-17T22:17:37.410Z
Learning: Do not flag enumeration in THashMap<TKey, TValue> as concurrent-modification unsafe. The no-mutation contract is maintained during enumeration: GetEnumerator shares the internal FSlots array (no per-call allocation) and only mutation occurs via destructor or Clear, not during active iteration. This pattern should be recognized across similar Pascal units/pascal collection implementations; when reviewing code, ensure no concurrent-modification warnings are raised for shared internal structures during enumeration if the no-mutation contract is preserved.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/OrderedMap.pas:0-0
Timestamp: 2026-03-17T22:17:55.588Z
Learning: Enforce the constraint on TOrderedMap<TKey, TValue> in units/OrderedMap.pas: TKey must be an unmanaged, fixed-size value type (no strings, dynamic arrays, or records with managed/pointer fields). HashKey should use fast paths for SizeOf(TKey)=8 (QWord) and SizeOf(TKey)=4 (Cardinal); KeysEqual should compare via direct PQWord/PCardinal for those sizes. The raw-byte DJB2/CompareMem fallback is dead code for current uses. Document this constraint in a unit-header comment as the enforcement mechanism. Do NOT re-flag the byte-wise hash/equality in future reviews as long as all TOrderedMap specializations use pointer-sized unmanaged keys.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/ScopeMap.pas:0-0
Timestamp: 2026-03-17T22:18:00.984Z
Learning: Do not re-introduce TScopeMap. In frostney/GocciaScript, TScopeMap<TValue> was removed due to performance regressions from its linear-scan Add-as-upsert behavior. Reviews should flag any use of TScopeMap in Units/ScopeMap.pas or elsewhere and ensure bindings use TOrderedStringMap<TLexicalBinding> instead. If reviewing similar Pascal files, prefer avoiding upsert-like add semantics that overwrite existing entries, as they correlated with bytecode performance drops. Ensure changes adhere to the established replacement: TOrderedStringMap<TLexicalBinding>.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Shared/utility units under the repository (e.g., units/BaseMap.pas, HashMap.pas, OrderedMap.pas, etc.) that are used across multiple parts of the project should not be required to use the Goccia.* prefix. A separate naming convention will be defined in a follow-up issue. Do not flag missing Goccia.* prefix for these shared infrastructure units in reviews.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 66
File: units/BaseMap.pas:22-22
Timestamp: 2026-03-18T08:12:24.165Z
Learning: Do not flag missing Goccia.* prefixes for shared/utility units that are used across multiple parts of the project or can be extracted as standalone infrastructure (e.g., BaseMap.pas, HashMap.pas, OrderedMap.pas, OrderedStringMap.pas, GC, test assertion units). A separate naming convention (to be defined in issue `#104`) should apply to these targets. When reviewing, apply this guideline to all similar shared utility units under the units directory; for one-off implementations outside this scope, continue with the existing naming expectations.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 110
File: units/Goccia.Runtime.Operations.pas:0-0
Timestamp: 2026-03-23T11:25:00.381Z
Learning: In this repo’s GocciaScript Pascal code, when implementing thenable detection/callability checks (e.g., around Await/unwrap logic), always read the thenable’s `then` property using `Goccia.Constants.PropertyNames.PROP_THEN` rather than a string literal. If the value being inspected is a `TSouffleRecord`, unwrap the retrieved `then` property via `UnwrapToGocciaValue` before testing whether it is callable (e.g., in `TGocciaRuntimeOperations.AwaitValue`).
Learnt from: frostney
Repo: frostney/GocciaScript PR: 107
File: units/Goccia.Runtime.Operations.pas:4349-4353
Timestamp: 2026-03-23T12:38:54.412Z
Learning: In Pascal source under units/, when decoding JSON string escape sequences that represent Unicode code points and then converting them to UTF-8, prefer using the existing helper (implemented via CodePointToUTF8) that correctly combines UTF-16 surrogate pairs (\uD800–\uDBFF + \uDC00–\uDFFF) into a single Unicode code point before UTF-8 encoding. Avoid ad-hoc conversions like Char(StrToInt(...)) for code-point→UTF-8 conversion logic; use the helper instead to ensure surrogate pairs are handled correctly.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 109
File: units/Goccia.Runtime.Operations.pas:6337-6344
Timestamp: 2026-03-23T15:57:55.802Z
Learning: In Pascal unit files under units/, any iterator result record objects produced by native iterators (e.g., in routines analogous to NativeMapIteratorNext / NativeSetIteratorNext) must: (1) set `Rec.Delegate := FVM.RecordDelegate`; and (2) set properties using `PROP_VALUE` / `PROP_DONE` constants from `Goccia.Constants.PropertyNames` rather than hard-coded string literals. Apply the same pattern to any future native iterator result objects so they interoperate correctly in bytecode mode (e.g., for `Object.prototype` method behavior) and keep property-name usage consistent via the shared constants.
Learnt from: frostney
Repo: frostney/GocciaScript PR: 136
File: docs/adding-built-in-types.md:105-133
Timestamp: 2026-03-27T14:09:25.371Z
Learning: In frostney/GocciaScript Pascal code, do not flag or suggest refactoring the early-exit singleton initialization guard `if Assigned(FShared) then Exit` when it appears at the top of `InitializePrototype`. The project documentation treats this early-exit form as the canonical/shared-prototype singleton initialization template (i.e., avoid recommending an `if not Assigned(FShared) then ...` wrapping alternative).
Contributor
Benchmark Results263 benchmarks Interpreted: 🟢 170 improved · 🔴 14 regressed · 79 unchanged · avg +2.1% arraybuffer.js — Interp: 🟢 10, 🔴 3, 1 unch. · avg +2.2% · Bytecode: 🟢 9, 🔴 3, 2 unch. · avg +2.0%
arrays.js — Interp: 🟢 13, 🔴 4, 2 unch. · avg +1.8% · Bytecode: 🟢 4, 🔴 2, 13 unch. · avg +0.2%
async-await.js — Interp: 🟢 3, 3 unch. · avg +1.4% · Bytecode: 🔴 5, 1 unch. · avg -1.7%
classes.js — Interp: 🟢 15, 16 unch. · avg +1.2% · Bytecode: 🟢 3, 28 unch. · avg +0.7%
closures.js — Interp: 🟢 8, 3 unch. · avg +3.1% · Bytecode: 🟢 1, 🔴 1, 9 unch. · avg -2.0%
collections.js — Interp: 🟢 10, 🔴 1, 1 unch. · avg +2.5% · Bytecode: 🟢 6, 🔴 4, 2 unch. · avg +0.3%
destructuring.js — Interp: 🟢 19, 🔴 1, 2 unch. · avg +2.4% · Bytecode: 🟢 4, 🔴 7, 11 unch. · avg +0.2%
fibonacci.js — Interp: 🟢 7, 1 unch. · avg +3.1% · Bytecode: 🟢 3, 🔴 2, 3 unch. · avg +2.7%
for-of.js — Interp: 🟢 5, 2 unch. · avg +2.0% · Bytecode: 🟢 5, 2 unch. · avg +3.6%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 13, 7 unch. · avg +2.7% · Bytecode: 🟢 4, 🔴 6, 10 unch. · avg -0.1%
json.js — Interp: 🟢 14, 6 unch. · avg +2.7% · Bytecode: 🟢 5, 🔴 1, 14 unch. · avg +1.5%
jsx.jsx — Interp: 🟢 16, 5 unch. · avg +2.0% · Bytecode: 🔴 6, 15 unch. · avg -0.8%
modules.js — Interp: 🟢 3, 🔴 1, 5 unch. · avg +0.2% · Bytecode: 🟢 2, 🔴 1, 6 unch. · avg +0.6%
numbers.js — Interp: 🟢 4, 🔴 2, 5 unch. · avg +1.1% · Bytecode: 🟢 1, 🔴 1, 9 unch. · avg +3.5%
objects.js — Interp: 🟢 4, 3 unch. · avg +3.4% · Bytecode: 🟢 3, 4 unch. · avg +1.6%
promises.js — Interp: 🟢 6, 6 unch. · avg +2.3% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg +0.4%
strings.js — Interp: 🟢 6, 🔴 1, 4 unch. · avg +1.4% · Bytecode: 🟢 3, 🔴 2, 6 unch. · avg +0.9%
typed-arrays.js — Interp: 🟢 14, 🔴 1, 7 unch. · avg +1.9% · Bytecode: 🟢 9, 🔴 5, 8 unch. · avg +1.8%
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
Suite Timing
Measured on ubuntu-latest x64. |
- require own properties for toMatchObject subset checks - add regression coverage for prototype-chain subset matching
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.Evaluator.Comparison.pas`:
- Around line 360-385: The recursive comparison (in IsDeepEqual /
IsPartialDeepEqual paths that handle TGocciaObjectValue pairs using AActual and
AExpected and iterating ExpectedKeys) can infinite-loop on cyclic object graphs;
add a visited-pair set keyed by the pair (AActual, AExpected) that is checked at
the start of each recursive comparison and updated before recursing so revisited
pairs immediately short-circuit to True (or the appropriate already-seen
result), and ensure both IsDeepEqual and IsPartialDeepEqual consult and update
this shared visited-pair tracking to prevent stack overflows on cycles.
🪄 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: 96db738c-4a98-4707-abb2-b780548654c0
📒 Files selected for processing (2)
units/Goccia.Builtins.TestAssertions.Test.pasunits/Goccia.Evaluator.Comparison.pas
🚧 Files skipped from review as they are similar to previous changes (1)
- units/Goccia.Builtins.TestAssertions.Test.pas
- track visited value pairs in deep and partial comparisons - add cyclic matcher regression coverage for toStrictEqual and toMatchObject
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
expect():toStrictEqual,toContainEqual,toMatchObject, andtoMatch.Testing
units/Goccia.Builtins.TestAssertions.Test.pas.Summary by CodeRabbit
New Features
Documentation
Tests