Bugfix FXIOS-14456 #31308 ⁃ Fix test failing on XCode 26.2 to enable using this version on Bitrise #25
Conversation
WalkthroughThe pull request removes mock test helpers for WKFrameInfo and WKSecurityOrigin and replaces them with a new WebKitTestHelpers utility that uses real WKWebView instances to capture authentic frame and security origin data during navigation callbacks. Tests are updated to leverage this new approach. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant Helper as WebKitTestHelpers
participant WebView as WKWebView
participant Delegate as FakeWKNavigationDelegate
Test->>Helper: captureFrameAndOrigin(for: url)
Helper->>WebView: Initialize & load URL
WebView->>Delegate: decidePolicyFor navigationAction
Delegate->>Delegate: Capture frame & security origin
Delegate->>Delegate: Fulfill XCTestExpectation
Helper->>Helper: Wait for expectation (timeout: 3s)
Helper->>Test: Return (WKFrameInfo, WKSecurityOrigin)?
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🤖 Fix all issues with AI agents
In `@BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift`:
- Around line 13-28: The FakeWKNavigationDelegate's
webView(_:decidePolicyFor:decisionHandler:) unconditionally calls
expect.fulfill(), which can be invoked multiple times and crash tests; add a
Bool property (e.g., didFulfill or fulfilled) to the FakeWKNavigationDelegate
class, initialize it false, and guard the call to expect.fulfill() so it only
executes once (set the flag to true immediately after fulfilling); keep
capturing capturedFrame and capturedOrigin as-is and still call
decisionHandler(.allow).
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift`:
- Around line 35-55: The testRequestMediaCaptureSuccess test creates an
XCTestExpectation but fulfills it after wait(for:) returns, causing a timeout;
move the expectation.fulfill() into the decisionHandler closure so the
expectation is satisfied when the permission callback runs. Concretely, keep the
expectation declaration as-is, update the decisionHandler closure (used in
subject.webView(... requestMediaCapturePermissionFor: ... type: ...
decisionHandler: decisionHandler)) to call XCTAssertEqual(decision, .prompt) and
then call expectation.fulfill() inside that closure, and remove the trailing
expectation.fulfill() after wait(for:).
🧹 Nitpick comments (1)
BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift (1)
32-43: Avoid hard dependency on external network for test helper.Loading a real URL makes tests depend on outbound connectivity; this can be flaky in CI. Consider loading local HTML with a baseURL to still produce an HTTPS origin without the network.
♻️ Possible refactor
- // load a real https URL (use example.com to be safe) - webView.load(URLRequest(url: url)) + // avoid external network; keep HTTPS origin via baseURL + let html = "<html><body>test</body></html>" + webView.loadHTMLString(html, baseURL: url)
📜 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/MockWKFrameInfo.swift
- BrowserKit/Tests/WebEngineTests/Mock/MockWKSecurityOrigin.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)
57-76: LGTM: expectation is fulfilled within the decision handler.
78-98: LGTM: consistent frame/origin capture and expectation handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate { | ||
| let expect: XCTestExpectation | ||
| var capturedFrame: WKFrameInfo? | ||
| var capturedOrigin: WKSecurityOrigin? | ||
|
|
||
| init(expect: XCTestExpectation) { self.expect = expect } | ||
|
|
||
| func webView(_ webView: WKWebView, | ||
| decidePolicyFor navigationAction: WKNavigationAction, | ||
| decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) { | ||
| let frame = navigationAction.targetFrame | ||
| capturedFrame = frame | ||
| capturedOrigin = frame?.securityOrigin | ||
| decisionHandler(.allow) | ||
| expect.fulfill() | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "WebKitTestHelpers.swift" -type f 2>/dev/null | head -20Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 157
🏁 Script executed:
git ls-files | grep -i webkittesthelpersRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 155
🏁 Script executed:
find . -name "*.swift" -path "*Tests*" -type f 2>/dev/null | grep -i webkitRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 230
🏁 Script executed:
cat ./BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 2187
🏁 Script executed:
rg "FakeWKNavigationDelegate" --type swift -A 5 -B 2Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1813
🏁 Script executed:
rg "captureFrameAndOrigin" --type swift -B 3 -A 3Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 3079
🏁 Script executed:
web_search
XCTestExpectation fulfill multiple times behaviorRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 196
🌐 Web query:
XCTestExpectation fulfill called multiple times fatal error
💡 Result:
Cause:
- XCTest throws a fatal error when you call expectation.fulfill() more than the framework expects (over-fulfillment) or after the test has completed — typically because your async callback fires multiple times or after teardown. [1][2]
Fixes (choose one):
- Ensure you only call fulfill once: guard with a Bool flag or hold the expectation weak and nil it after fulfilling. This prevents duplicate callbacks from over‑fulfilling. [1][3]
- If you legitimately expect N calls, set expectedFulfillmentCount = N (and optionally assertForOverFulfill = true). [2][3]
- If you want to ignore extra fulfills, set expectation.assertForOverFulfill = false (but prefer fixing the multiple-callback root cause). [1][3]
- Prefer XCTest’s async/await helpers (await fulfillment(of:timeout:)) or confirmation APIs for more explicit multi-confirmation semantics when using Swift concurrency. [2][6]
References:
- StackOverflow: API violation — multiple calls made to -[XCTestExpectation fulfill]. [1]
- Apple Developer — Asynchronous tests and expectations / XCTestExpectation docs (expectedFulfillmentCount, assertForOverFulfill). [2]
- “XCTestExpectation Gotchas” (practical patterns: weak expectation, niling, guards). [3]
🏁 Script executed:
web_search
example.com HTTP redirects test behaviorRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 190
🏁 Script executed:
web_search
WKNavigationDelegate decidePolicyFor multiple calls redirects subframesRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 199
🌐 Web query:
WKNavigationDelegate decidePolicyFor called multiple times redirects subframes
💡 Result:
Short answer: decidePolicyForNavigationAction can be invoked multiple times because WKWebView reports navigations for the main frame, for subframes/iframes, and again for provisional redirects (and for new-window/targetFrame=nil cases). Check navigationAction.targetFrame?.isMainFrame (or navigationAction.request.mainDocumentURL / navigationAction.sourceFrame) to distinguish main-frame navigations and avoid double-handling, and ensure you call the decisionHandler exactly once per callback.
Notes / tips:
- Use navigationAction.targetFrame?.isMainFrame (or navigationAction.isMainFrame / WKFrameInfo) to ignore subframe calls when you only care about top-level navigation. [1]
- Redirects and provisional navigations produce additional delegate callbacks (so the same logical navigation may trigger several decidePolicyFor calls). Track request URLs or navigation identifiers if you need to deduplicate. [2]
- Never call decisionHandler more than once; guard paths (e.g. return after calling decisionHandler) to avoid the "completion handler ... was called more than once" crash. [1][2]
Sources:
- Stack Overflow discussion about multiple/duplicate decisionHandler calls and crashes. [1]
- WKNavigationDelegate API / docs describing navigation delegate callbacks (main-frame vs subframe / provisional redirects). [2]
🌐 Web query:
example.com HTTP 301 redirect test
💡 Result:
Short answer: example.com redirects HTTP -> HTTPS. I confirmed by requesting http://example.com which returned the site served at https://example.com. [1]
To verify yourself, run:
- curl -I http://example.com
- or for the full chain: curl -IL http://example.com
Expected output shows a 301/3xx status and a Location: https://example.com/ header. Use -L to follow the redirect chain. [2]
Sources:
[1] example.com homepage (requested via HTTP/HTTPS).
[2] Guide: using curl to test HTTP->HTTPS redirects.
🏁 Script executed:
rg "https://example.com" BrowserKit/Tests/WebEngineTests/ --type swiftRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1541
Guard against multiple expectation fulfillments from repeated navigation callbacks.
decidePolicyFor fires multiple times during real WKWebView navigation (for provisional navigations, redirects, and subframes). Calling expect.fulfill() unconditionally will cause a fatal error in XCTest. Add a flag to ensure fulfillment happens only once.
Proposed fix
class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate {
let expect: XCTestExpectation
var capturedFrame: WKFrameInfo?
var capturedOrigin: WKSecurityOrigin?
+ private var didFulfill = false
init(expect: XCTestExpectation) { self.expect = expect }
func webView(_ webView: WKWebView,
decidePolicyFor navigationAction: WKNavigationAction,
decisionHandler: `@escaping` (WKNavigationActionPolicy) -> Void) {
+ if !didFulfill {
+ let frame = navigationAction.targetFrame
+ capturedFrame = frame
+ capturedOrigin = frame?.securityOrigin
+ didFulfill = true
+ expect.fulfill()
+ }
decisionHandler(.allow)
- let frame = navigationAction.targetFrame
- capturedFrame = frame
- capturedOrigin = frame?.securityOrigin
- decisionHandler(.allow)
- expect.fulfill()
- 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.
| class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate { | |
| let expect: XCTestExpectation | |
| var capturedFrame: WKFrameInfo? | |
| var capturedOrigin: WKSecurityOrigin? | |
| init(expect: XCTestExpectation) { self.expect = expect } | |
| func webView(_ webView: WKWebView, | |
| decidePolicyFor navigationAction: WKNavigationAction, | |
| decisionHandler: @escaping (WKNavigationActionPolicy) -> Void) { | |
| let frame = navigationAction.targetFrame | |
| capturedFrame = frame | |
| capturedOrigin = frame?.securityOrigin | |
| decisionHandler(.allow) | |
| expect.fulfill() | |
| return | |
| class FakeWKNavigationDelegate: NSObject, WKNavigationDelegate { | |
| let expect: XCTestExpectation | |
| var capturedFrame: WKFrameInfo? | |
| var capturedOrigin: WKSecurityOrigin? | |
| private var didFulfill = false | |
| init(expect: XCTestExpectation) { self.expect = expect } | |
| func webView(_ webView: WKWebView, | |
| decidePolicyFor navigationAction: WKNavigationAction, | |
| decisionHandler: `@escaping` (WKNavigationActionPolicy) -> Void) { | |
| if !didFulfill { | |
| let frame = navigationAction.targetFrame | |
| capturedFrame = frame | |
| capturedOrigin = frame?.securityOrigin | |
| didFulfill = true | |
| expect.fulfill() | |
| } | |
| decisionHandler(.allow) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@BrowserKit/Tests/WebEngineTests/Utilities/WebKitTestHelpers.swift` around
lines 13 - 28, The FakeWKNavigationDelegate's
webView(_:decidePolicyFor:decisionHandler:) unconditionally calls
expect.fulfill(), which can be invoked multiple times and crash tests; add a
Bool property (e.g., didFulfill or fulfilled) to the FakeWKNavigationDelegate
class, initialize it false, and guard the call to expect.fulfill() so it only
executes once (set the flag to true immediately after fulfilling); keep
capturing capturedFrame and capturedOrigin as-is and still call
decisionHandler(.allow).
| func testRequestMediaCaptureSuccess() { | ||
| let subject = createSubject(isActive: true) | ||
|
|
||
| let expectation = expectation(description: "Wait for the decision handler to be called") | ||
| 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:
cat -n BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift | sed -n '35,60p'Repository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 1413
🏁 Script executed:
find . -name "WKUIHandlerTests.swift" -type fRepository: qodo-benchmark/firefox-ios-combined-coderabbit
Length of output: 146
Fix expectation lifecycle to prevent test timeout.
The expectation.fulfill() on line 54 is called after wait(for:) returns, which means the wait will always timeout. The expectation must be fulfilled inside the decisionHandler closure so it signals completion when the handler is invoked.
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()🤖 Prompt for AI Agents
In `@BrowserKit/Tests/WebEngineTests/WKUIHandlerTests.swift` around lines 35 - 55,
The testRequestMediaCaptureSuccess test creates an XCTestExpectation but
fulfills it after wait(for:) returns, causing a timeout; move the
expectation.fulfill() into the decisionHandler closure so the expectation is
satisfied when the permission callback runs. Concretely, keep the expectation
declaration as-is, update the decisionHandler closure (used in
subject.webView(... requestMediaCapturePermissionFor: ... type: ...
decisionHandler: decisionHandler)) to call XCTAssertEqual(decision, .prompt) and
then call expectation.fulfill() inside that closure, and remove the trailing
expectation.fulfill() after wait(for:).
Benchmark PR from qodo-benchmark#222
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.