Bugfix FXIOS-14456 #31308 ⁃ Fix test failing on XCode 26.2 to enable using this version on Bitrise #38
Conversation
WalkthroughThe changes replace two mock classes (MockWKFrameInfo and MockWKSecurityOrigin) with a new test utility that captures real WKFrameInfo and WKSecurityOrigin objects from an actual WKWebView navigation. Test code is updated to use the real values instead of mocked ones. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift`:
- Around line 38-55: The test times out because expectation is created after
calling wait and never fulfilled; move creation of the XCTestExpectation to
before invoking subject.webView, capture it in the decision handler passed to
subject.webView(requestMediaCapturePermissionFor:initiatedByFrame:type:decisionHandler:),
and call expectation.fulfill() inside the decision handler (the closure
currently referenced as decisionHandler) after asserting
XCTAssertEqual(decision, .prompt); this ensures
WebKitTestHelpers.captureFrameAndOrigin(for:) / subject.webView(...) triggers
the closure which then fulfills the expectation.
🧹 Nitpick comments (2)
BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift (2)
12-12: Enforce main-thread usage for WKWebView in the helper.WKWebView APIs are main-thread only; without an explicit main-actor annotation, this helper can be accidentally called off-main later. Consider enforcing this at the API boundary.
✅ Suggested change
-final class WebKitTestHelpers { +@MainActor +final class WebKitTestHelpers {
34-48: Handle the wait result explicitly to avoid masking timeouts.
XCTWaiter.wait’s return value is unused. A guard on.completedboth removes the unused variable and makes timeout behavior explicit.✅ Suggested change
- let waiter = XCTWaiter.wait(for: [expect], timeout: timeout) - if let frame = delegate.capturedFrame, let origin = delegate.capturedOrigin { + let result = XCTWaiter.wait(for: [expect], timeout: timeout) + guard result == .completed else { return nil } + if let frame = delegate.capturedFrame, let origin = delegate.capturedOrigin { return (frame, origin) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
BrowserKit/Tests/WebEngineTests/Mock/MockWKFrameInfo.swiftBrowserKit/Tests/WebEngineTests/Mock/MockWKSecurityOrigin.swiftBrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swiftBrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift
💤 Files with no reviewable changes (2)
- BrowserKit/Tests/WebEngineTests/Mock/MockWKSecurityOrigin.swift
- BrowserKit/Tests/WebEngineTests/Mock/MockWKFrameInfo.swift
🧰 Additional context used
🧬 Code graph analysis (1)
BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift (1)
BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift (2)
captureFrameAndOrigin(34-49)webView(20-29)
🔇 Additional comments (2)
BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift (2)
59-76: LGTM for this test update.
81-98: LGTM for this test update.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| guard let (frame, origin) = WebKitTestHelpers.captureFrameAndOrigin(for: URL(string: "https://example.com")!) else { | ||
| XCTFail("Could not obtain WKFrameInfo") | ||
| return | ||
| } | ||
|
|
||
| let decisionHandler = { (decision: WKPermissionDecision) in | ||
| XCTAssertEqual(decision, .prompt) | ||
| expectation.fulfill() | ||
| } | ||
|
|
||
| let expectation = expectation(description: "Wait for the decision handler to be called") | ||
| subject.webView(MockWKWebView(), | ||
| requestMediaCapturePermissionFor: MockWKSecurityOrigin.new(nil), | ||
| initiatedByFrame: MockWKFrameInfo(), | ||
| requestMediaCapturePermissionFor: origin, | ||
| initiatedByFrame: frame, | ||
| type: .cameraAndMicrophone, | ||
| decisionHandler: decisionHandler | ||
| ) | ||
| decisionHandler: decisionHandler) | ||
| wait(for: [expectation]) | ||
| expectation.fulfill() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "WKUIHandlerTests.swift" -type fRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 146
🏁 Script executed:
cat -n ./BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift | head -100Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 4546
Fix expectation fulfillment order (test currently times out).
Line 53 calls wait(for:) before line 54 calls expectation.fulfill(), so the wait will never complete and the test will timeout. Move expectation creation before the closure and add fulfillment inside the decision handler.
Suggested fix
- let decisionHandler = { (decision: WKPermissionDecision) in
- XCTAssertEqual(decision, .prompt)
- }
-
- let expectation = expectation(description: "Wait for the decision handler to be called")
+ let expectation = expectation(description: "Wait for the decision handler to be called")
+ let decisionHandler = { (decision: WKPermissionDecision) in
+ XCTAssertEqual(decision, .prompt)
+ expectation.fulfill()
+ }
subject.webView(MockWKWebView(),
requestMediaCapturePermissionFor: origin,
initiatedByFrame: frame,
type: .cameraAndMicrophone,
decisionHandler: decisionHandler)
wait(for: [expectation])
- expectation.fulfill()📝 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.
| guard let (frame, origin) = WebKitTestHelpers.captureFrameAndOrigin(for: URL(string: "https://example.com")!) else { | |
| XCTFail("Could not obtain WKFrameInfo") | |
| return | |
| } | |
| let decisionHandler = { (decision: WKPermissionDecision) in | |
| XCTAssertEqual(decision, .prompt) | |
| expectation.fulfill() | |
| } | |
| let expectation = expectation(description: "Wait for the decision handler to be called") | |
| subject.webView(MockWKWebView(), | |
| requestMediaCapturePermissionFor: MockWKSecurityOrigin.new(nil), | |
| initiatedByFrame: MockWKFrameInfo(), | |
| requestMediaCapturePermissionFor: origin, | |
| initiatedByFrame: frame, | |
| type: .cameraAndMicrophone, | |
| decisionHandler: decisionHandler | |
| ) | |
| decisionHandler: decisionHandler) | |
| wait(for: [expectation]) | |
| expectation.fulfill() | |
| } | |
| let expectation = expectation(description: "Wait for the decision handler to be called") | |
| let decisionHandler = { (decision: WKPermissionDecision) in | |
| XCTAssertEqual(decision, .prompt) | |
| expectation.fulfill() | |
| } | |
| subject.webView(MockWKWebView(), | |
| requestMediaCapturePermissionFor: origin, | |
| initiatedByFrame: frame, | |
| type: .cameraAndMicrophone, | |
| decisionHandler: decisionHandler) | |
| wait(for: [expectation]) |
🤖 Prompt for AI Agents
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift` around lines 38 - 55,
The test times out because expectation is created after calling wait and never
fulfilled; move creation of the XCTestExpectation to before invoking
subject.webView, capture it in the decision handler passed to
subject.webView(requestMediaCapturePermissionFor:initiatedByFrame:type:decisionHandler:),
and call expectation.fulfill() inside the decision handler (the closure
currently referenced as decisionHandler) after asserting
XCTAssertEqual(decision, .prompt); this ensures
WebKitTestHelpers.captureFrameAndOrigin(for:) / subject.webView(...) triggers
the closure which then fulfills the expectation.
Benchmark PR from agentic-review-benchmarks#12
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.