Feat/window or worker global scope #242
Conversation
Refactor some tests coupled implementation details of window->script engine coupling.
Window only had a reference to a clock that could allow time to pass; not add events. In order to decouple the script engine from global scope, and different global scopes will need different setTimeout wrappers, this permits the Window to expose all functionality of Global scope.
WalkthroughIntroduces a centralized Clock and wires it through Browser (new public field), ScriptEngineOptions, WindowOptions, and various script host implementations so hosts and windows use a host-provided clock instead of per-context clocks. Replaces direct clock.Tick() calls with host-backed tick() indirection and surfaces tick errors alongside call errors via errors.Join. Test scaffolding shifts toward browsertest-based browser/window initialization, adds WithIgnoreErrorLogs and a dummy HTTP handler for tests, and adds JsAssert.True/False helpers for boolean assertions. Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
scripting/sobekengine/script_engine.go (1)
15-24: Add a nil check in scriptContext.tick() to prevent potential panics.All callers of
NewHostfound in the codebase do provideopts.Clock. However,scriptContext.tick()directly callsc.host.clock.Tick()without a nil check. Add a defensive guard to handle the case where clock might be nil:func (c *scriptContext) tick() error { if c.host.clock == nil { return nil // or return an error } return c.host.clock.Tick() }This prevents potential nil pointer panics if clock is ever uninitialized.
scripting/v8engine/script_engine.go (1)
46-64: Verify Clock field is always non-nil when provided to ScriptEngineOptions.When reusing a host (line 57),
host.clockis set directly fromoptions.Clockwithout validation. While current callers (browser.go and script_host.go) always provide a non-nil clock viaclock.New(), theScriptEngineOptionsAPI does not enforce this requirement. If any caller passes a nil Clock, downstream code will panic:
- scripting/v8engine/script_context.go:35 calls
c.host.clock.Tick()- internal/html/xhr/xml_http_request.go:149 calls
req.clock.AddSafeTask(...)- scripting/v8engine/script_context.go:112 returns
ctx.host.clockdirectlyEither document that
ScriptEngineOptions.Clockmust be non-nil and enforce this at the API boundary, or add nil checks before all clock method calls.html/window.go (2)
175-210: PreventWindow.Clock()from returning nil when a ScriptContext clock exists.
Clock()now returnsw.clock(Line 401-402). If callers create a window with a ScriptHost but don’t setWindowOptions.Clock,w.clockcan be nil even thoughw.scriptContext.Clock()is usable—this is a behavior regression.Proposed fix (fallback to ScriptContext clock)
-func (w *window) Clock() Clock { return w.clock } +func (w *window) Clock() Clock { + if w.clock != nil { + return w.clock + } + if w.scriptContext != nil { + return w.scriptContext.Clock() + } + return nil +}Also applies to: 401-402
14-30: Use the publicClockinterface instead of internal*clock.Clockin option structs.
ScriptEngineOptions.Clock(line 29),WindowOptions.Clock(line 438), andWindowOptionClock()(line 470) expose*clock.Clockfrom theinternal/clockpackage. External consumers cannot import internal packages per Go's module rules, preventing them from implementing custom Clock behavior.The public
Clockinterface (line 60-84) already exists in the same package with the necessary methods. Use it for these API surfaces instead.scripting/internal/scripttests/event_loop_suite.go (1)
14-31: Store the window in the suite struct and close it properly in both SetupTest and TestDispatchError.The
SetupTestmethod creates a window but only stores its script context (line 25-26), making it impossible to close the window resource inTeardownTest. Additionally,TestDispatchErrorcreates a local window helper that is never closed (line 62), leaking its resources. Follow the established pattern used inScriptHostSuite: store the window as a struct field and close it inTeardownTest, and close the local window inTestDispatchErrorusing defer.Proposed fix
type EventLoopTestSuite struct { suite.Suite e html.ScriptEngine + w htmltest.WindowHelper ctx html.ScriptContext } func (s *EventLoopTestSuite) SetupTest() { - w := browsertest.InitWindow(s.T(), s.e) - s.ctx = w.ScriptContext() + s.w = browsertest.InitWindow(s.T(), s.e) + s.ctx = s.w.ScriptContext() } func (s *EventLoopTestSuite) TeardownTest() { - s.ctx.Close() + if s.w != nil { + s.w.Close() + } } func (s *EventLoopTestSuite) TestDispatchError() { Expect := gomega.NewWithT(s.T()).Expect w := browsertest.InitWindow(s.T(), s.e, browsertest.WithLogOption(gosttest.AllowErrors())) + defer w.Close() Expect(
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
browser.gohtml/window.gointernal/testing/browsertest/browsertest.gointernal/testing/htmltest/assert.goscripting/internal/html/htmlsuite/animation_frame_suite.goscripting/internal/scripttests/event_loop_suite.goscripting/internal/scripttests/location_suite.goscripting/internal/scripttests/script_host_factory_suite.goscripting/internal/scripttests/script_host_suite.goscripting/sobekengine/scope.goscripting/sobekengine/script_context.goscripting/sobekengine/script_engine.goscripting/sobekengine/script_host.goscripting/sobekengine/sobek_module.goscripting/v8engine/callback_context.goscripting/v8engine/module.goscripting/v8engine/script_context.goscripting/v8engine/script_engine.goscripting/v8engine/script_host.goscripting/v8engine/script_host_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:42:10.996Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 153
File: internal/test/scripttests/download_suite.go:13-39
Timestamp: 2025-11-25T05:42:10.996Z
Learning: In the gost-dom/browser repository, functions in `internal/test/scripttests` like `RunDownloadScriptSuite`, `RunHtmxTests`, etc. are reusable test suites (test code) rather than test helpers. They are designed to be invoked from multiple test functions with different script engines. Functions containing `t.Run` are test code, not helper code, and should not have `t.Helper()` added.
Applied to files:
scripting/internal/scripttests/script_host_factory_suite.gointernal/testing/browsertest/browsertest.goscripting/internal/scripttests/script_host_suite.goscripting/sobekengine/script_context.goscripting/internal/scripttests/event_loop_suite.goscripting/internal/scripttests/location_suite.goscripting/internal/html/htmlsuite/animation_frame_suite.go
📚 Learning: 2025-05-15T13:03:48.567Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 83
File: internal/dom/mutation/observer_test.go:52-57
Timestamp: 2025-05-15T13:03:48.567Z
Learning: In Go, files with `_test` suffix are test files and should implement strict validation (like panicking on unexpected conditions) rather than being forgiving, as this helps detect potential issues early during testing instead of allowing them to silently pass.
Applied to files:
scripting/internal/scripttests/location_suite.go
📚 Learning: 2025-06-30T14:43:51.301Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 115
File: scripting/internal/streams/readable_stream_byobreader.go:20-0
Timestamp: 2025-06-30T14:43:51.301Z
Learning: In the gost-dom JavaScript binding system, `js.Value[T]` is a Go interface that can be nil even though it represents a JavaScript value. Always check `v != nil` before calling methods like `v.Boolean()` to prevent panics when the Go interface is nil.
Applied to files:
internal/testing/htmltest/assert.go
🧬 Code graph analysis (12)
scripting/internal/scripttests/script_host_factory_suite.go (4)
internal/testing/gosttest/gomega_suite.go (1)
GomegaSuite(12-14)html/window.go (1)
Window(134-159)internal/testing/htmltest/helpers.go (1)
WindowHelper(48-51)internal/testing/browsertest/browsertest.go (1)
InitWindow(98-109)
internal/testing/browsertest/browsertest.go (1)
scripting/internal/scripttests/helpers.go (1)
WithLogOption(13-13)
html/window.go (1)
internal/clock/clock.go (1)
Clock(74-102)
scripting/v8engine/callback_context.go (1)
html/window.go (1)
Clock(60-84)
scripting/v8engine/script_host_test.go (1)
html/window.go (3)
WindowOptions(435-442)ScriptHost(55-58)Clock(60-84)
scripting/sobekengine/script_context.go (1)
html/window.go (1)
Clock(60-84)
scripting/internal/scripttests/event_loop_suite.go (3)
html/window.go (1)
ScriptContext(96-119)internal/testing/browsertest/browsertest.go (2)
InitWindow(98-109)WithLogOption(37-39)scripting/internal/scripttests/helpers.go (1)
WithLogOption(13-13)
scripting/internal/scripttests/location_suite.go (1)
internal/testing/browsertest/browsertest.go (1)
InitBrowser(66-87)
scripting/v8engine/script_engine.go (1)
html/window.go (1)
Clock(60-84)
scripting/sobekengine/script_engine.go (1)
html/window.go (1)
Clock(60-84)
scripting/sobekengine/script_host.go (1)
html/window.go (1)
Clock(60-84)
scripting/v8engine/script_context.go (1)
html/window.go (1)
Clock(60-84)
🔇 Additional comments (17)
scripting/sobekengine/scope.go (1)
28-28: LGTM! Clock access centralized through host.The method now correctly returns the clock from the host scope, aligning with the centralized clock management pattern introduced in this PR.
scripting/sobekengine/sobek_module.go (1)
25-25: LGTM! Consistent use of tick() abstraction.The change from direct clock access to the
tick()helper is consistent with the centralized clock management pattern and provides a cleaner abstraction.scripting/sobekengine/script_context.go (2)
28-29: LGTM! Clean abstraction for clock access.The introduction of the
tick()helper and the updatedClock()method correctly centralize clock access through the host, maintaining the public API while changing the internal implementation.
52-52: LGTM! Consistent use of tick() abstraction.The change to use
i.tick()aligns with the new abstraction layer and centralizes clock management.scripting/sobekengine/script_host.go (1)
32-32: LGTM! Clock dependency properly added to scriptHost.The addition of the
clockfield toscriptHostis consistent with the centralized clock management pattern. The clock is now injected viaNewHost(as seen inscript_engine.go) rather than created locally, which is a better design for dependency management.browser.go (1)
122-142: LGTM! Centralized clock management is well-implemented.The clock is properly initialized and propagated through the engine options, browser instance, and window creation path. This establishes a clean shared dependency for time simulation in tests.
internal/testing/browsertest/browsertest.go (2)
30-35: LGTM! Useful helper for error scenario testing.The
WithIgnoreErrorLogs()option provides a clear way to suppress error-level log failures when explicitly testing error scenarios.
76-78: LGTM! Sensible default handler.Providing a fallback handler when nil is passed simplifies test setup for tests that don't require specific HTTP behavior.
internal/testing/htmltest/assert.go (1)
41-63: LGTM! Well-designed assertion helpers.The new
True()andFalse()methods provide type-safe JavaScript boolean assertions with clear error messages. The genericevalAndAssertTypehelper ensures proper type checking and reporting.scripting/v8engine/callback_context.go (1)
113-113: LGTM! Consistent with centralized clock architecture.The change to return
s.host.clockinstead of a context-local clock aligns with the PR's goal of establishing a shared clock dependency throughout the browser, engine, and window layers.scripting/v8engine/module.go (1)
30-30: LGTM! Clock tick delegation refactored.The change from
mod.ctx.clock.Tick()tomod.ctx.tick()aligns with the clock centralization refactoring, where script contexts now delegate timing operations to a host-level clock through a helper method.scripting/v8engine/script_engine.go (1)
66-83: LGTM! Clock propagated correctly during host creation.The clock is properly initialized from
config.Clockwhen creating a new host. This ensures the centralized clock dependency is established at host creation time.scripting/v8engine/script_host_test.go (1)
22-22: LGTM! Clock properly wired into window initialization.The addition of
Clock: host.clockensures the window receives the clock from the host, aligning with the centralized clock management pattern introduced in this PR.scripting/internal/scripttests/location_suite.go (1)
23-25: LGTM! Test refactored to use browsertest infrastructure.The migration from direct
html.NewWindowconstruction to the browsertest-based initialization pattern (InitBrowser→OpenWindow) improves test maintainability and ensures consistent clock propagation through the browser and window creation paths.scripting/v8engine/script_host.go (2)
61-61: LGTM! Clock field added for centralized clock management.The addition of the
clockfield toV8ScriptHostcentralizes clock management at the host level, enabling contexts to share a single clock instance rather than maintaining per-context clocks.
122-133: LGTM! Clock initialization in deprecated New() function.The deprecated
New()function correctly initializes a new clock instance with the provided logger, ensuring backward compatibility for code that still uses this constructor. The clock creation with logger configuration enables proper observability for timing operations.scripting/internal/scripttests/script_host_factory_suite.go (1)
3-18: No cleanup is needed inSetupTestorTeardownTest. Each invocation ofInitWindow(line 17) automatically registers cleanup viat.Cleanup()inbrowsertest.InitBrowser, which closes the browser after each test completes. Since testify'ssuite.Suitecreates a fresh test instance and runs cleanup between test methods, eachSetupTestreceives its own isolated browser and window with independent cleanup. Additionally,WindowHelperhas noClose()method, so the proposed fix cannot be implemented.
| func (c *V8ScriptContext) iso() *v8.Isolate { return c.host.iso } | ||
| func (c *V8ScriptContext) Context() context.Context { return c.browsingCtx.Context() } | ||
| func (c *V8ScriptContext) tick() error { return c.host.clock.Tick() } | ||
|
|
There was a problem hiding this comment.
Don’t ignore tick() errors in runScript (can mask event-loop failures).
runScript calls ctx.tick() (Line 145) but drops the error; if ticking fails, callers won’t see it and tests may become flaky.
Proposed fix
res, err = ctx.v8ctx.RunScript(script, "")
- ctx.tick()
+ err = errors.Join(err, ctx.tick())
returnAlso applies to: 138-147
Removes dependency to old poor test function
Removes poor OpenWindow helper function
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
internal/testing/browsertest/browsertest.goscripting/internal/html/htmlsuite/animation_frame_suite.goscripting/internal/scripttests/event_loop_suite.goscripting/internal/scripttests/fetch_suite.goscripting/internal/scripttests/script_host_suite.goscripting/internal/scripttests/suites.goscripting/internal/scripttests/window_test_suite.goscripting/internal/scripttests/xhr_suite.goscripting/internal/testing/jsassert/package.go
💤 Files with no reviewable changes (1)
- scripting/internal/scripttests/suites.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:42:10.996Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 153
File: internal/test/scripttests/download_suite.go:13-39
Timestamp: 2025-11-25T05:42:10.996Z
Learning: In the gost-dom/browser repository, functions in `internal/test/scripttests` like `RunDownloadScriptSuite`, `RunHtmxTests`, etc. are reusable test suites (test code) rather than test helpers. They are designed to be invoked from multiple test functions with different script engines. Functions containing `t.Run` are test code, not helper code, and should not have `t.Helper()` added.
Applied to files:
scripting/internal/testing/jsassert/package.goscripting/internal/scripttests/event_loop_suite.goscripting/internal/scripttests/script_host_suite.goscripting/internal/scripttests/window_test_suite.goscripting/internal/html/htmlsuite/animation_frame_suite.gointernal/testing/browsertest/browsertest.goscripting/internal/scripttests/xhr_suite.goscripting/internal/scripttests/fetch_suite.go
📚 Learning: 2025-06-27T10:43:03.227Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 109
File: internal/testing/gosttest/assertions.go:37-37
Timestamp: 2025-06-27T10:43:03.227Z
Learning: The gost-dom/browser project prioritizes extremely fast tests. According to the project maintainer stroiman, anything that takes longer than a few milliseconds is considered problematic. Test timeouts should be kept minimal (e.g., 100ms is already generous) rather than increased to more conservative values like 1 second.
Applied to files:
scripting/internal/scripttests/event_loop_suite.go
📚 Learning: 2025-05-15T13:03:48.567Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 83
File: internal/dom/mutation/observer_test.go:52-57
Timestamp: 2025-05-15T13:03:48.567Z
Learning: In Go, files with `_test` suffix are test files and should implement strict validation (like panicking on unexpected conditions) rather than being forgiving, as this helps detect potential issues early during testing instead of allowing them to silently pass.
Applied to files:
scripting/internal/scripttests/window_test_suite.goscripting/internal/html/htmlsuite/animation_frame_suite.go
🧬 Code graph analysis (5)
scripting/internal/scripttests/event_loop_suite.go (2)
internal/testing/htmltest/helpers.go (1)
WindowHelper(48-51)internal/testing/browsertest/browsertest.go (2)
InitWindow(98-109)WithIgnoreErrorLogs(33-35)
scripting/internal/scripttests/script_host_suite.go (2)
html/window.go (1)
Window(134-159)internal/testing/browsertest/browsertest.go (1)
InitWindow(98-109)
scripting/internal/scripttests/window_test_suite.go (2)
internal/testing/browsertest/browsertest.go (1)
InitBrowser(66-87)internal/testing/gomega-matchers/expect.go (1)
Expect(18-20)
internal/testing/browsertest/browsertest.go (3)
scripting/internal/scripttests/helpers.go (1)
WithLogOption(13-13)dom/event/event_handler.go (1)
HandlerFunc(37-37)internal/fetch/fetch.go (2)
Request(42-49)Header(21-24)
scripting/internal/scripttests/xhr_suite.go (6)
internal/testing/gosttest/gomega_suite.go (1)
GomegaSuite(12-14)internal/testing/gomega-matchers/expect.go (1)
Expect(18-20)dom/event/event_handler.go (1)
NewEventHandlerFunc(50-52)dom/event/event.go (1)
Event(39-51)internal/testing/htmltest/helpers.go (1)
WindowHelper(48-51)internal/testing/browsertest/browsertest.go (1)
InitBrowser(66-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests on linux amd64 with Go
- GitHub Check: Tests on darwin amd64 with Go
- GitHub Check: Tests on darwin arm64 with Go
- GitHub Check: Tests on linux arm64 with Go
🔇 Additional comments (17)
scripting/internal/testing/jsassert/package.go (1)
1-10: Documentation clarification is helpful and honest.The updated package comment accurately describes the technical limitation (wrong source line in test output due to JS context scope), appropriately signals deprecation intent, and sets clear expectations for future maintainers. This aligns well with the PR's broader theme of improving test infrastructure.
scripting/internal/scripttests/fetch_suite.go (3)
11-11: LGTM: Import addition supports new test infrastructure.The browsertest import is correctly added to support the new browser/window initialization pattern used in the updated subtests.
22-27: LGTM: Prototypes subtest correctly verifies fetch API globals.The test properly uses the new browsertest infrastructure to verify that
fetch,Response, andRequestare all available as functions in the window global scope.
220-229: LGTM: Document location subtest correctly verifies Request URL resolution.The test properly verifies that a
Requestconstructed with a relative URL resolves against the document location. The use ofbrowsertest.InitBrowserandOpenWindowallows precise control of the document URL for testing this behavior.internal/testing/browsertest/browsertest.go (2)
30-35: LGTM! Clean convenience wrapper.The
WithIgnoreErrorLogshelper provides a well-documented abstraction overWithLogOption(gosttest.AllowErrors()), improving test readability for error scenario tests.
76-78: LGTM! Sensible default for nil handler.Providing a fallback dummy HTTP server when
handleris nil ensuresInitBrowseralways operates with a valid handler, simplifying test setup for scenarios that don't need custom HTTP behavior.scripting/internal/scripttests/window_test_suite.go (1)
76-79: LGTM! Clean migration to browsertest-based window initialization.The test correctly initializes a browser, opens a window with a specific URL, and verifies
location.hrefusing the window-scoped evaluator. This aligns with the broader test infrastructure migration to browsertest-based patterns.Note: This test intentionally creates its own browser instance rather than using the suite's
s.Window, which is appropriate since it needs to test navigation to a specific URL.scripting/internal/scripttests/event_loop_suite.go (4)
25-31: LGTM! Proper test setup with appropriate error log handling.Using
WithIgnoreErrorLogs()is correct here since tests likeTestDispatchErrorintentionally throw JavaScript errors. The setup/teardown properly initializes and closes the window helper.
33-44: LGTM! Clean migration to window helper pattern.The test correctly uses
s.w.Evalands.w.Clock().Advance()for deferred execution testing. The logic remains sound.
60-75: LGTM! Appropriate use of MustEval for error dispatch test.Using
MustEvalinstead ofEvalis correct here since the script itself should execute without errors (the error is thrown inside a setTimeout callback, not during eval).
91-107: LGTM! Comprehensive interval test with proper cleanup verification.The test correctly verifies interval behavior including execution timing and cleanup via
clearInterval. Usings.w.Runfor the clearInterval call is appropriate.scripting/internal/scripttests/xhr_suite.go (3)
17-26: LGTM! Clean refactoring to GomegaSuite base.The migration from
ScriptHostSuitetogosttest.GomegaSuitewith an explicitenginefield simplifies the test suite structure and aligns with the new browsertest-based patterns.
128-131: LGTM! Clever use of suite as HTTP handler.The
initWindowhelper elegantly usess(the suite itself) as the HTTP handler sinceXMLHttpRequestSuiteimplementsServeHTTP. This allows tests to capture request details likes.bodyands.actualPath.
49-83: LGTM! Test correctly uses addEventListener with event parameter.Unlike
TestOnloadAttribute, this test correctly captures the event in theaddEventListenercallback whereeis properly defined as a function parameter.scripting/internal/html/htmlsuite/animation_frame_suite.go (1)
21-30: No scope issues with the test code.The refactoring to explicit assertions (
win.Assert().True/False) is correct. Thecalledvariable declared withleton line 21 is properly accessible whenwin.Assert()evaluates the expression"called"on lines 27 and 30, since bothMustRun()andEval()operate within the same persistent JavaScript execution context.Likely an incorrect or invalid review comment.
scripting/internal/scripttests/script_host_suite.go (2)
3-8: Import switch tobrowsertestlooks appropriate for the new init path.
This aligns the suite with the browsertest-based initialization and avoids bespoke host setup.
21-27: Remove the resource leak concern—Browser cleanup is properly registered. Reconsider the defensive guard syntax.The Browser cleanup is already handled correctly:
InitBrowserregisters cleanup viat.Cleanup(b.Close)at line 85, so there is no resource leak. The Browser will be closed automatically at test completion.However, a guard is worth considering if SetupTest could panic before completing. If so, TeardownTest would be called with a zero-valued WindowHelper (nil embedded Window), causing a panic on Close(). The proposed guard syntax is incorrect for a struct value—it would need to check the embedded Window field, e.g.,
if s.Window.Window != nil { s.Window.Close() }.
If a script engine is initialized without a clock, it would panic when calling tick(). This is now fixed. It also adjusts some event callbacks to not having the responsibility to tick. The latter _does_ add duplication in the script engines; but it is script engine responsibility to run microtasks before returning.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripting/sobekengine/script_context.go (1)
51-60: Critical: Tick error is discarded.The tick() error is not captured or returned, creating an inconsistency with other call sites where
errors.Joinis used to merge tick errors (see function.go:26 and v8engine/script_context.go:92). If clock advancement fails, the error will be silently ignored.🐛 Proposed fix to capture and surface tick errors
func (i *scriptContext) run(script, location string) (sobek.Value, error) { select { case <-i.Context().Done(): return nil, html.ErrCancelled default: res, err := i.vm.RunScript(location, script) - i.tick() - return res, err + err = errors.Join(err, i.tick()) + return res, err } }scripting/v8engine/script_context.go (1)
143-153: Critical: Tick error is discarded.The tick() error is not captured, creating an inconsistency with line 92 in this same file where
errors.Joinis used. If clock advancement fails, the error will be silently ignored.🐛 Proposed fix to capture and surface tick errors
func (ctx *V8ScriptContext) runScript(script string) (res *v8.Value, err error) { goCtx := ctx.Context() select { case <-goCtx.Done(): return nil, html.ErrCancelled default: res, err = ctx.v8ctx.RunScript(script, "") - ctx.tick() - return + err = errors.Join(err, ctx.tick()) + return } }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
scripting/internal/dom/event_target.goscripting/internal/scripttests/event_loop_suite.goscripting/sobekengine/function.goscripting/sobekengine/script_context.goscripting/v8engine/function.goscripting/v8engine/script_context.goscripting/v8engine/script_host_test.gov8browser/browser_test.go
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-25T05:42:10.996Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 153
File: internal/test/scripttests/download_suite.go:13-39
Timestamp: 2025-11-25T05:42:10.996Z
Learning: In the gost-dom/browser repository, functions in `internal/test/scripttests` like `RunDownloadScriptSuite`, `RunHtmxTests`, etc. are reusable test suites (test code) rather than test helpers. They are designed to be invoked from multiple test functions with different script engines. Functions containing `t.Run` are test code, not helper code, and should not have `t.Helper()` added.
Applied to files:
scripting/v8engine/script_host_test.goscripting/internal/scripttests/event_loop_suite.goscripting/sobekengine/script_context.go
📚 Learning: 2025-06-30T14:43:51.301Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 115
File: scripting/internal/streams/readable_stream_byobreader.go:20-0
Timestamp: 2025-06-30T14:43:51.301Z
Learning: In the gost-dom JavaScript binding system, `js.Value[T]` is a Go interface that can be nil even though it represents a JavaScript value. Always check `v != nil` before calling methods like `v.Boolean()` to prevent panics when the Go interface is nil.
Applied to files:
scripting/v8engine/function.go
📚 Learning: 2025-05-15T13:03:48.567Z
Learnt from: stroiman
Repo: gost-dom/browser PR: 83
File: internal/dom/mutation/observer_test.go:52-57
Timestamp: 2025-05-15T13:03:48.567Z
Learning: In Go, files with `_test` suffix are test files and should implement strict validation (like panicking on unexpected conditions) rather than being forgiving, as this helps detect potential issues early during testing instead of allowing them to silently pass.
Applied to files:
scripting/internal/scripttests/event_loop_suite.go
🧬 Code graph analysis (4)
scripting/v8engine/script_host_test.go (2)
internal/testing/gomega-matchers/expect.go (1)
Expect(18-20)internal/testing/browsertest/browsertest.go (1)
InitWindow(98-109)
scripting/v8engine/script_context.go (1)
html/window.go (1)
Clock(60-84)
scripting/internal/scripttests/event_loop_suite.go (3)
internal/testing/htmltest/helpers.go (1)
WindowHelper(48-51)internal/testing/browsertest/browsertest.go (2)
InitWindow(98-109)WithIgnoreErrorLogs(33-35)internal/testing/gomega-matchers/expect.go (1)
Expect(18-20)
scripting/sobekengine/script_context.go (1)
html/window.go (1)
Clock(60-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests on linux arm64 with Go
- GitHub Check: Tests on linux amd64 with Go
- GitHub Check: Tests on darwin arm64 with Go
- GitHub Check: Tests on darwin amd64 with Go
🔇 Additional comments (10)
scripting/v8engine/function.go (1)
19-31: LGTM! Error handling correctly surfaces tick errors.The use of
errors.Joinensures that tick errors are propagated alongside function call errors, preventing silent failures in clock advancement.scripting/sobekengine/function.go (1)
24-35: LGTM! Consistent error handling across engines.The implementation mirrors the v8engine pattern, ensuring uniform tick error propagation in both script engines.
scripting/sobekengine/script_context.go (1)
28-34: LGTM! Defensive nil check and clean Clock() delegation.The tick() method safely handles cases where host.clock is not configured, and Clock() correctly returns the centralized host clock.
scripting/v8engine/script_context.go (2)
35-40: LGTM! Proper nil check and clock delegation.The tick() method safely handles missing clock configuration, and Clock() correctly exposes the host clock.
Also applies to: 117-117
88-96: LGTM! Proper error handling with tick error surfacing.The initialization correctly merges tick errors with script errors using
errors.Join, ensuring clock advancement failures are not silently ignored.scripting/v8engine/script_host_test.go (2)
22-22: LGTM: Simplified evaluation API.Direct
win.Evalis cleaner than retrieving the script context first, while maintaining identical test behavior.
14-19: Approve modernization to browsertest helpers.The refactoring to use
browsertest.InitWindowandwin.LoadHTMLis a clear improvement:
- Reduces boilerplate window setup code
- Cleanup is now handled automatically via browsertest infrastructure
- Error handling is appropriately preserved
defaultEngineis properly defined and initialized in package scopescripting/internal/scripttests/event_loop_suite.go (2)
7-8: LGTM! Clean migration to WindowHelper pattern.The refactoring from direct
ScriptHost/ScriptContextmanagement to theWindowHelperabstraction improves test maintainability and aligns with the broader testing infrastructure improvements across the codebase.Also applies to: 16-17, 21-21, 25-25, 29-29
90-106: LGTM! Correct use of Run for side-effect statement.The test correctly uses
s.w.Run("clearInterval(h)")(line 103) for the statement that doesn't return a value, vs.s.w.Eval()for expressions that return values to verify. The clock advancement pattern is clear and explicit.scripting/internal/dom/event_target.go (1)
18-26: Simplification is correct and preserves clock ticking behavior.The removal of the explicit
clock.Tick()call fromHandleEventis intentional and properly centralizes clock management. Clock ticking still occurs when the event listener is invoked throughf.Call(), which internally callsctx.tick()in both the V8 and Sobek engine implementations (seescripting/v8engine/function.go:26andscripting/sobekengine/function.go:33). This aligns with the PR's objective of moving clock management to script execution boundaries (function calls, script runs, module loads) rather than distributing it throughout event handling code.
No description provided.