Add onTestFinished cleanup hook to test framework#223
Merged
Conversation
- Register per-test cleanup callbacks for the test framework - Document lifecycle hook order and add coverage for scoping and async callbacks
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new per-test cleanup hook Changes
Sequence Diagram(s)sequenceDiagram
participant TR as Test Runner
participant BE as beforeEach hooks
participant TB as Test Body
participant AE as afterEach hooks
participant OF as onTestFinished callbacks
TR->>BE: run beforeEach
BE-->>TR: complete
TR->>TB: execute test body\n(register onTestFinished)
TB-->>TR: complete
TR->>AE: run afterEach hooks (may be nested)
AE-->>TR: complete
TR->>OF: invoke registered callbacks\n(in registration order)
OF-->>TR: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/language/testing/onTestFinished.js`:
- Around line 54-77: The tests register onTestFinished callbacks but never
assert they ran; declare the arrays (asyncLog, marker) in the outer describe
scope (not as locals inside each test), push values from the onTestFinished
callbacks in the respective tests (use asyncLog and marker as currently named),
then change the final "cleanup ran for previous test" test to assert that
asyncLog contains "async-cleanup" and marker contains "cleanup" (or await/poll
briefly if callbacks run asynchronously) so the suite verifies the cleanup
callbacks actually executed.
In `@units/Goccia.Builtins.TestAssertions.pas`:
- Line 2348: The literal hook name 'onTestFinished' is duplicated; create a
shared constant PROP_ON_TEST_FINISHED in Goccia.Constants.PropertyNames and
replace all hardcoded occurrences with that constant (for example update the
AScope.DefineLexicalBinding call that currently uses
TGocciaNativeFunctionValue.Create(OnTestFinished, 'onTestFinished', 1) to use
PROP_ON_TEST_FINISHED instead) and update the corresponding
validation/registration sites (the other occurrences referenced around the
AScope.DefineLexicalBinding usage and the blocks near the other mentions) so
both registration and validation use
Goccia.Constants.PropertyNames.PROP_ON_TEST_FINISHED consistently.
- Line 2322: FOnTestFinishedCallbacks is storing JS callback values in a
Pascal-only collection (TGocciaArgumentsCollection) without GC roots; wrap
stored callbacks with the runtime's temporary rooting APIs: call AddTempRoot
when a callback is added to FOnTestFinishedCallbacks and call RemoveTempRoot
when the callback is removed or when the collection is cleared/finalized (also
ensure the destructor of the owner removes roots for any remaining elements);
update the code paths that insert/remove callbacks (references:
FOnTestFinishedCallbacks, TGocciaArgumentsCollection) and mirror this pattern
for the other occurrences noted (around lines handling the collections at 2370,
2835, 2909–2910, 3350–3359) so values held only by Pascal are protected for the
duration they are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d14d673-a64a-4ec7-816c-b99970af8528
📒 Files selected for processing (4)
AGENTS.mddocs/testing.mdtests/language/testing/onTestFinished.jsunits/Goccia.Builtins.TestAssertions.pas
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results274 benchmarks Interpreted: 🟢 183 improved · 🔴 17 regressed · 74 unchanged · avg +3.3% arraybuffer.js — Interp: 🟢 13, 🔴 1 · avg +4.7% · Bytecode: 🟢 11, 🔴 1, 2 unch. · avg +5.0%
arrays.js — Interp: 🟢 18, 1 unch. · avg +7.6% · Bytecode: 🟢 16, 🔴 2, 1 unch. · avg +4.0%
async-await.js — Interp: 🟢 6 · avg +6.4% · Bytecode: 🟢 6 · avg +5.0%
classes.js — Interp: 🟢 22, 🔴 1, 8 unch. · avg +3.4% · Bytecode: 🟢 4, 🔴 11, 16 unch. · avg -2.3%
closures.js — Interp: 🟢 10, 1 unch. · avg +3.5% · Bytecode: 🟢 8, 🔴 1, 2 unch. · avg +2.1%
collections.js — Interp: 🟢 6, 🔴 2, 4 unch. · avg +1.4% · Bytecode: 🟢 5, 🔴 4, 3 unch. · avg +1.2%
destructuring.js — Interp: 🟢 10, 🔴 2, 10 unch. · avg +2.1% · Bytecode: 🟢 5, 🔴 14, 3 unch. · avg -1.9%
fibonacci.js — Interp: 🟢 4, 4 unch. · avg +4.5% · Bytecode: 🟢 6, 2 unch. · avg +4.0%
for-of.js — Interp: 7 unch. · avg -1.1% · Bytecode: 🟢 4, 🔴 1, 2 unch. · avg +1.8%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🟢 10, 10 unch. · avg +1.7% · Bytecode: 🟢 15, 🔴 1, 4 unch. · avg +4.1%
json.js — Interp: 🟢 18, 2 unch. · avg +4.7% · Bytecode: 🟢 20 · avg +6.8%
jsx.jsx — Interp: 🟢 13, 8 unch. · avg +2.1% · Bytecode: 🟢 4, 🔴 8, 9 unch. · avg -0.5%
modules.js — Interp: 🟢 8, 🔴 1 · avg +5.6% · Bytecode: 🟢 3, 🔴 1, 5 unch. · avg +0.4%
numbers.js — Interp: 🟢 9, 2 unch. · avg +6.4% · Bytecode: 🟢 8, 🔴 2, 1 unch. · avg +3.3%
objects.js — Interp: 🟢 6, 🔴 1 · avg +3.1% · Bytecode: 🟢 2, 🔴 2, 3 unch. · avg +1.5%
promises.js — Interp: 🟢 9, 3 unch. · avg +2.2% · Bytecode: 🟢 11, 1 unch. · avg +4.0%
regexp.js — Interp: 🟢 6, 🔴 1, 4 unch. · avg +3.5% · Bytecode: 🟢 5, 6 unch. · avg +3.4%
strings.js — Interp: 🟢 6, 🔴 1, 4 unch. · avg +2.3% · Bytecode: 🟢 10, 1 unch. · avg +5.8%
typed-arrays.js — Interp: 🟢 9, 🔴 7, 6 unch. · avg +0.8% · Bytecode: 🟢 8, 🔴 9, 5 unch. · avg -1.1%
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. |
Add temp-root protection for callbacks stored in FOnTestFinishedCallbacks, matching the existing pattern used by beforeAll/beforeEach/afterEach/afterAll hooks. Hoist async test arrays to describe scope so follow-up tests can assert that cleanup callbacks actually executed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
onTestFinished(fn)lifecycle hook for per-test cleanup in the JS test framework.afterEachhooks and are reset between tests.Testing
tests/language/testing/onTestFinished.jsunits/Goccia.Builtins.TestAssertions.pasdocs/testing.mdandAGENTS.md