Skip to content

Conversation

@stroiman
Copy link
Member

No description provided.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 24, 2026

Walkthrough

This PR refactors the WPT test pipeline: adds a runtime-controlled concurrency cap (MAX_CONCURRENT_TESTS); introduces WebPlatformTest and adds TotalCount and PassedCount to WptSuiteResult; reworks Run to use openWindow and processEvents helpers with layered panic recovery and enhanced logging; makes test execution concurrency-aware and adjusts per-test logging; updates test-case source signatures to accept a context.CancelCauseFunc and return channels; modifies manifest loading to surface errors via the cancel cause; and converts manifest parser functions to return and propagate errors instead of panicking.

Possibly related PRs

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No pull request description was provided by the author. Add a description explaining the error handling improvements, why they were made, and their impact on the test suite's reliability.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Test/wpt error handling' directly describes the main change—improving error handling in the WPT test suite by replacing panics with proper error propagation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot]

This comment was marked as resolved.

@stroiman stroiman force-pushed the test/wpt-error-handling branch from 9eaae8c to 5dc2a15 Compare January 24, 2026 08:44
coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Comment on lines +45 to +57
func (s manifestTestCaseSource) fetchManifest(ctx context.Context) (res *http.Response, err error) {
var req *http.Request
req, err = http.NewRequestWithContext(ctx, http.MethodGet, s.href, nil)
if err != nil {
panic(fmt.Sprintf("load manifest: create request: %v", err))
err = fmt.Errorf("load manifest: create request: %w", err)
return
}
res, err := http.DefaultClient.Do(req)
res, err = http.DefaultClient.Do(req)
if err != nil {
panic(fmt.Sprintf("load manifest: %v", err))
err = fmt.Errorf("load manifest: %w", err)
}
ch := make(chan TestCase)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Close response body on Do error to prevent leaks.
http.Client.Do can return a non-nil res with err; not closing res.Body can leak connections.

🩹 Proposed fix
 res, err = http.DefaultClient.Do(req)
 if err != nil {
+	if res != nil && res.Body != nil {
+		res.Body.Close()
+	}
+	res = nil
 	err = fmt.Errorf("load manifest: %w", err)
 	return
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (s manifestTestCaseSource) fetchManifest(ctx context.Context) (res *http.Response, err error) {
var req *http.Request
req, err = http.NewRequestWithContext(ctx, http.MethodGet, s.href, nil)
if err != nil {
panic(fmt.Sprintf("load manifest: create request: %v", err))
err = fmt.Errorf("load manifest: create request: %w", err)
return
}
res, err := http.DefaultClient.Do(req)
res, err = http.DefaultClient.Do(req)
if err != nil {
panic(fmt.Sprintf("load manifest: %v", err))
err = fmt.Errorf("load manifest: %w", err)
}
ch := make(chan TestCase)
return
}
func (s manifestTestCaseSource) fetchManifest(ctx context.Context) (res *http.Response, err error) {
var req *http.Request
req, err = http.NewRequestWithContext(ctx, http.MethodGet, s.href, nil)
if err != nil {
err = fmt.Errorf("load manifest: create request: %w", err)
return
}
res, err = http.DefaultClient.Do(req)
if err != nil {
if res != nil && res.Body != nil {
res.Body.Close()
}
res = nil
err = fmt.Errorf("load manifest: %w", err)
}
return
}

@github-actions github-actions bot merged commit b8b32c3 into main Jan 25, 2026
8 of 9 checks passed
@github-actions github-actions bot deleted the test/wpt-error-handling branch January 25, 2026 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants