Fix failing connection recovery tests #1957
Conversation
It seems like the Realtime connection key format, which as the removed comment implies is not guaranteed to be stable, has changed on sandbox. Stop using our knowledge of it.
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/browser/connection.test.js (1)
Line range hint
342-360: Consider improving the assertion message.While the change to use connection ID is good, the assertion message could be more descriptive.
- expect(newRealtime.connection.id).to.equal(connectionId, 'Check new realtime recovered the old'); + expect(newRealtime.connection.id).to.equal(connectionId, 'Check connection was recovered using the recovery key');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/browser/connection.test.js(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (3)
test/browser/connection.test.js (3)
Line range hint
234-253: LGTM! Good change to use connection ID instead of key.The switch from connection key to connection ID makes the test more reliable since it doesn't depend on the unstable connection key format. The connection ID is a more stable identifier for verifying connection recovery.
Line range hint
278-297: LGTM! Consistent use of connection ID for denied recovery test.The change maintains consistency with other tests by using connection ID instead of key, while correctly verifying that a new connection is created when recovery is denied.
Line range hint
1-1: Verify safe removal of connection key processing functions.The PR removes
connectionHmacandsameConnectionfunctions. Let's verify they're not used elsewhere in the codebase.✅ Verification successful
Functions safely removed - no remaining references found
Thorough codebase analysis confirms that
connectionHmacandsameConnectionfunctions have been completely removed with no remaining references, indicating a safe removal.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the removed functions echo "Searching for connectionHmac usage..." rg "connectionHmac" echo "Searching for sameConnection usage..." rg "sameConnection"Length of output: 209
Script:
#!/bin/bash # Perform a more thorough search with case-insensitive matching and word boundaries echo "Searching for connectionHmac usage (case-insensitive, word boundaries)..." rg -iw "connectionHmac" echo "Searching for sameConnection usage (case-insensitive, word boundaries)..." rg -iw "sameConnection" # Also search in the specific test file to see the context of removal echo "Checking the test file context..." rg -A 5 -B 5 "connectionHmac|sameConnection" "test/browser/connection.test.js" || echo "No matches in test file"Length of output: 574
It seems like the Realtime connection key format, which as the removed comment implies is not guaranteed to be stable, has changed on sandbox. Stop using our knowledge of it.
Summary by CodeRabbit