Fix web FetchRequest does not respect disableConnectivityCheck client option
#1925
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
warning eslint@7.32.0: This version is no longer supported. Please see https://eslint.org/version-support for other options. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
test/browser/connection.test.js (1)
458-458: Clarify the comment for better readabilityThe comment could be rephrased to more explicitly explain the purpose of these tests and how they differ from existing ones.
Suggested change:
- // this is identical to disable_connectivity_check test in connectivity, but here we specifically test browser supported request implementations + // Similar to the 'disable_connectivity_check' test in 'connectivity', but specifically testing browser-supported request implementations (XHR and Fetch)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/platform/web/lib/http/http.ts(1 hunks)test/browser/connection.test.js(2 hunks)
🔇 Additional comments (3)
src/platform/web/lib/http/http.ts (2)
121-124: LGTM: Proper implementation of disableConnectivityCheck option
The implementation correctly respects the disableConnectivityCheck client option by immediately returning true when enabled, consistent with the XHR implementation.
120-138: Verify test coverage for connectivity checks
Let's ensure that the test suite adequately covers both XHR and Fetch implementations, particularly for custom connectivity check URLs.
✅ Verification successful
Let me gather more information about the test implementations to ensure proper coverage.
Let me check the actual test cases for both success and failure scenarios to ensure complete coverage.
Test coverage for connectivity checks is comprehensive
The test suite adequately covers all aspects of the connectivity check implementation:
- Tests for both success and failure scenarios with custom URLs
- Coverage for
disableConnectivityCheckoption - Specific tests for both XHR and Fetch implementations
- Tests for connectivity check with different URL configurations
The changes in the connectivity check implementation are well-supported by existing tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for connectivity check related tests
echo "Searching for connectivity check related tests..."
rg -l "connectivityCheck|checkConnectivity" test/
# Search for custom URL configurations in tests
echo "Searching for custom connectivity URL configurations in tests..."
rg "connectivityCheckUrl" test/
Length of output: 1319
Script:
#!/bin/bash
# Check the connectivity test implementations
echo "Checking connectivity.test.js implementation:"
rg -A 10 "describe.*connectivity" test/realtime/connectivity.test.js
# Check transport test implementations
echo "Checking transport test implementations:"
rg -A 10 "describe.*connectivity" test/realtime/transports.test.js
# Check connection test implementations
echo "Checking connection test implementations:"
rg -A 10 "describe.*connectivity" test/browser/connection.test.js
# Look for disableConnectivityCheck usage in tests
echo "Checking disableConnectivityCheck usage:"
rg "disableConnectivityCheck" test/
Length of output: 2086
Script:
#!/bin/bash
# Check the specific test cases in connectivity tests
echo "Checking connectivity test cases:"
rg -B 2 -A 10 "it\(.*connectivity" test/realtime/connectivity.test.js
# Check the specific test cases in connection tests
echo "Checking connection test cases:"
rg -B 2 -A 10 "it\(.*connectivity" test/browser/connection.test.js
# Check for any XHR specific connectivity tests
echo "Checking XHR specific tests:"
rg -B 2 -A 10 "XHR.*connectivity|xhr.*connectivity" test/
Length of output: 2430
test/browser/connection.test.js (1)
3-3: Dependency Update: Added 'Ably' module to test dependencies
Including 'Ably' in the module dependencies ensures that the tests have access to Ably.Rest.Platform.Config, which is necessary for configuring and manipulating platform-specific settings in the new test scenarios.
| this.checkConnectivity = async function () { | ||
| Logger.logAction( | ||
| this.logger, | ||
| Logger.LOG_MICRO, | ||
| '(Fetch)Http.checkConnectivity()', | ||
| 'Sending; ' + connectivityCheckUrl, | ||
| ); | ||
| const requestResult = await this.doUri(HttpMethods.Get, connectivityCheckUrl, null, null, null); | ||
| const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes'; | ||
| Logger.logAction(this.logger, Logger.LOG_MICRO, '(Fetch)Http.checkConnectivity()', 'Result: ' + result); | ||
| return result; | ||
| }; |
There was a problem hiding this comment.
Inconsistent handling of custom connectivity check URLs between XHR and Fetch implementations
The Fetch implementation doesn't handle custom connectivity check URLs (connectivityUrlIsDefault) in the same way as the XHR implementation. This could lead to different behaviors when using custom connectivity check URLs.
XHR implementation:
if (!connectivityUrlIsDefault) {
result = !requestResult.error && isSuccessCode(requestResult.statusCode as number);
} else {
result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
}Fetch implementation only uses the body check:
const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';Apply this diff to make the implementations consistent:
- const result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
+ let result = false;
+ if (!connectivityUrlIsDefault) {
+ result = !requestResult.error && isSuccessCode(requestResult.statusCode as number);
+ } else {
+ result = !requestResult.error && (requestResult.body as string)?.replace(/\n/, '') == 'yes';
+ }| for (const scenario of [ | ||
| { | ||
| description: 'XHR request disable connectivity check', | ||
| ctx: { initialFetchSupported: Ably.Rest.Platform.Config.fetchSupported }, | ||
| before: (ctx) => { | ||
| Ably.Rest.Platform.Config.fetchSupported = false; | ||
| }, | ||
| after: (ctx) => { | ||
| Ably.Rest.Platform.Config.fetchSupported = ctx.initialFetchSupported; | ||
| }, | ||
| }, | ||
| { | ||
| description: 'Fetch request disable connectivity check', | ||
| ctx: { initialXhrSupported: Ably.Rest.Platform.Config.xhrSupported }, | ||
| before: (ctx) => { | ||
| Ably.Rest.Platform.Config.xhrSupported = false; | ||
| }, | ||
| after: (ctx) => { | ||
| Ably.Rest.Platform.Config.xhrSupported = ctx.initialXhrSupported; | ||
| }, | ||
| }, | ||
| ]) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating global configuration to prevent side effects in tests
Directly modifying global configurations like Ably.Rest.Platform.Config.fetchSupported and xhrSupported may lead to side effects affecting other tests, especially if tests are run in parallel. To prevent unintended interactions:
- Use stubbing or mocking libraries (e.g., Sinon.js) to temporarily override these properties within the test scope.
- Ensure that any modifications are safely isolated and restored, even if the test encounters an error.
| it(scenario.description, async function () { | ||
| const helper = this.test.helper; | ||
| scenario.before(scenario.ctx); | ||
|
|
||
| helper.recordPrivateApi('pass.clientOption.connectivityCheckUrl'); | ||
| helper.recordPrivateApi('pass.clientOption.disableConnectivityCheck'); | ||
| const options = { | ||
| connectivityCheckUrl: helper.unroutableHost, | ||
| disableConnectivityCheck: true, | ||
| autoConnect: false, | ||
| }; | ||
|
|
||
| let thrownError = null; | ||
| let res = null; | ||
| try { | ||
| helper.recordPrivateApi('call.http.checkConnectivity'); | ||
| res = await helper.AblyRealtime(options).http.checkConnectivity(); | ||
| } catch (error) { | ||
| thrownError = error; | ||
| } | ||
|
|
||
| scenario.after(scenario.ctx); | ||
| expect( | ||
| res && !thrownError, | ||
| 'Connectivity check completed ' + | ||
| (thrownError && | ||
| (helper.recordPrivateApi('call.Utils.inspectError'), helper.Utils.inspectError(thrownError))), | ||
| ).to.be.ok; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ensure proper cleanup in tests by using 'finally' block or test hooks
If an exception occurs before scenario.after(scenario.ctx); is called, the global configurations may not be restored, potentially affecting other tests. To guarantee cleanup:
- Place
scenario.after(scenario.ctx);inside afinallyblock to ensure it executes regardless of errors. - Alternatively, use
afterEachhooks provided by the testing framework to handle teardown.
Suggested change:
try {
helper.recordPrivateApi('call.http.checkConnectivity');
res = await helper.AblyRealtime(options).http.checkConnectivity();
} catch (error) {
thrownError = error;
+} finally {
+ scenario.after(scenario.ctx);
}
-scenario.after(scenario.ctx);📝 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.
| it(scenario.description, async function () { | |
| const helper = this.test.helper; | |
| scenario.before(scenario.ctx); | |
| helper.recordPrivateApi('pass.clientOption.connectivityCheckUrl'); | |
| helper.recordPrivateApi('pass.clientOption.disableConnectivityCheck'); | |
| const options = { | |
| connectivityCheckUrl: helper.unroutableHost, | |
| disableConnectivityCheck: true, | |
| autoConnect: false, | |
| }; | |
| let thrownError = null; | |
| let res = null; | |
| try { | |
| helper.recordPrivateApi('call.http.checkConnectivity'); | |
| res = await helper.AblyRealtime(options).http.checkConnectivity(); | |
| } catch (error) { | |
| thrownError = error; | |
| } | |
| scenario.after(scenario.ctx); | |
| expect( | |
| res && !thrownError, | |
| 'Connectivity check completed ' + | |
| (thrownError && | |
| (helper.recordPrivateApi('call.Utils.inspectError'), helper.Utils.inspectError(thrownError))), | |
| ).to.be.ok; | |
| }); | |
| } | |
| it(scenario.description, async function () { | |
| const helper = this.test.helper; | |
| scenario.before(scenario.ctx); | |
| helper.recordPrivateApi('pass.clientOption.connectivityCheckUrl'); | |
| helper.recordPrivateApi('pass.clientOption.disableConnectivityCheck'); | |
| const options = { | |
| connectivityCheckUrl: helper.unroutableHost, | |
| disableConnectivityCheck: true, | |
| autoConnect: false, | |
| }; | |
| let thrownError = null; | |
| let res = null; | |
| try { | |
| helper.recordPrivateApi('call.http.checkConnectivity'); | |
| res = await helper.AblyRealtime(options).http.checkConnectivity(); | |
| } catch (error) { | |
| thrownError = error; | |
| } finally { | |
| scenario.after(scenario.ctx); | |
| } | |
| expect( | |
| res && !thrownError, | |
| 'Connectivity check completed ' + | |
| (thrownError && | |
| (helper.recordPrivateApi('call.Utils.inspectError'), helper.Utils.inspectError(thrownError))), | |
| ).to.be.ok; | |
| }); | |
| } |
See internal slack thread for more context.
Summary by CodeRabbit
New Features
disableConnectivityCheckoption is enabled.Bug Fixes
Tests