Skip to content

Conversation

@thePromger
Copy link
Contributor

@thePromger thePromger commented Oct 29, 2025

Description

In the WebSocket test, an intermittent assertion error occurs when the received data order does not match the order in which messages were sent by the server.
This issue appears mostly in CI environments due to timing or concurrency differences.

Summary

  • Fixed a race condition in the WebSocket test by ensuring that message order is validated deterministically.
  • The test no longer assumes strict ordering when it’s not guaranteed by the WebSocket protocol.
  • Improves test reliability and eliminates random assertion failures in CI.

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation
  • You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

Summary by CodeRabbit

  • Tests
    • Improved WebSocket integration tests to validate that a set of responses (in any order) matches expected messages.
    • Added assertion to verify the server reports connection closure after the final command.

@vercel
Copy link

vercel bot commented Oct 29, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
robyn Error Error Oct 29, 2025 7:42am

@coderabbitai
Copy link

coderabbitai bot commented Oct 29, 2025

Walkthrough

The WebSocket integration test was changed to use order-independent, set-based assertions for verifying three response messages (replacing three sequential recv checks in two places) and added an assertion that the server reports "Connection closed" after the final test command.

Changes

Cohort / File(s) Change Summary
Test Reliability Improvements
integration_tests/test_web_sockets.py
Replaced two occurrences of three sequential recv checks with order-independent set-based verification (via sorted lists) to assert the three responses match expected messages in any order. Added an assertion that the server reports "Connection closed" after the final test command.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single test file modified with consistent, localized changes.
  • Review focus: correctness of the set-based comparison and that the final "Connection closed" assertion matches server behavior.
  • Pay attention to both modified locations to ensure identical expectations and no off-by-one/missing recv handling.

Poem

🐰
I hopped through sockets, neat and spry,
Turned flaky checks to order-free.
Three replies now dance as one,
A final close—our test is done. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues Check ✅ Passed The linked issue #1188 requests fixing a recurring test-level problem that causes random CI failures, with the explicit goal of improving test reliability and helping contributors obtain green CI checks. The PR changes directly address this by fixing a race condition in the WebSocket test that was causing intermittent assertion failures due to non-deterministic message ordering. The fix replaces sequential recv checks with order-agnostic set-based validation and adds server connection closure verification, which aligns with the core objective of eliminating random CI test failures.
Out of Scope Changes Check ✅ Passed All changes in this PR are contained within integration_tests/test_web_sockets.py and are directly related to fixing the race condition in WebSocket message validation as described in linked issue #1188. The modifications—replacing sequential checks with set-based validation and adding connection closure assertion—are narrowly focused on improving test reliability and do not introduce unrelated changes or modifications outside the scope of addressing the intermittent CI test failures.
Title Check ✅ Passed The PR title "fix: Fixes assert issue in websocket integration tests" is directly related to the main change in the changeset. The raw summary confirms that the modifications address assertion failures by replacing sequential recv checks with set-based checks to handle non-deterministic message ordering, and the PR objectives specify that this fixes intermittent assertion errors in WebSocket tests caused by timing and concurrency issues. The title is concise, clear, and specific enough for teammates scanning the history to understand that this is fixing assertion-related problems in the websocket integration test suite, without being vague or generic.
Description Check ✅ Passed The PR description includes all required sections from the repository template: a clear description of the problem, a reference to the fixed issue (#1188), a detailed summary of the changes, a completed PR checklist, and pre-commit instructions. The description goes beyond the basic template by providing context about the intermittent failures in CI and explaining the fix deterministically validates message order without assuming strict ordering. While some checklist items remain unchecked (documentation and additional tests), these represent honest acknowledgments of what was not added rather than missing information from the description itself. The description effectively communicates the purpose and scope of the changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

Copy link

@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: 0

🧹 Nitpick comments (1)
integration_tests/test_web_sockets.py (1)

21-21: Excellent - this directly fixes the "Slim Shady" error from issue #1188.

The set-based assertion here mirrors the fix on line 15 and specifically addresses the intermittent failure mentioned in the linked issue. The test will now pass regardless of the order in which these three messages arrive.

Optional improvement for debugging: Consider adding a comment explaining why set-based comparison is used, or using sorted() instead of set() for clearer assertion failure messages:

-    assert set([ws.recv() for _ in range(3)]) == set(["hi", "hello", "*chika* *chika* Slim Shady."])
+    # Messages may arrive in any order due to WebSocket broadcast behavior
+    received = sorted([ws.recv() for _ in range(3)])
+    expected = sorted(["hi", "hello", "*chika* *chika* Slim Shady."])
+    assert received == expected

This produces more readable diffs when assertions fail, showing the sorted lists instead of set representations.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40a8bd1 and ed7b615.

📒 Files selected for processing (1)
  • integration_tests/test_web_sockets.py (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
  • GitHub Check: ubuntu tests with python 3.12
  • GitHub Check: ubuntu tests with python 3.13
  • GitHub Check: ubuntu tests with python 3.9
  • GitHub Check: ubuntu tests with python 3.10
  • GitHub Check: ubuntu tests with python 3.11
  • GitHub Check: windows tests with python 3.9
  • GitHub Check: windows tests with python 3.13
  • GitHub Check: windows tests with python 3.12
  • GitHub Check: lint
  • GitHub Check: linux (3.12, x86_64)
  • GitHub Check: linux (3.11, i686)
  • GitHub Check: macos (3.11)
  • GitHub Check: windows (3.9, x64)
  • GitHub Check: linux (3.11, x86_64)
  • GitHub Check: macos (3.10)
  • GitHub Check: macos (3.12)
  • GitHub Check: linux (3.9, i686)
  • GitHub Check: linux (3.9, x86_64)
  • GitHub Check: linux (3.10, x86_64)
  • GitHub Check: benchmarks
🔇 Additional comments (1)
integration_tests/test_web_sockets.py (1)

15-15: Good fix for the race condition!

The set-based comparison correctly addresses the intermittent test failure by removing the assumption of message ordering. This allows the three WebSocket responses to arrive in any order while still validating that all expected messages are received.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 29, 2025

CodSpeed Performance Report

Merging #1261 will not alter performance

Comparing thePromger:fix-tests (d867ef8) with main (118660f)1

Summary

✅ 150 untouched

Footnotes

  1. No successful run was found on main (40a8bd1) during the generation of this report, so 118660f was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

* This fixes, assert error occurred randomly in websocket test, due to getting data not in same order as sent from server
Copy link
Contributor Author

@thePromger thePromger left a comment

Choose a reason for hiding this comment

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

@sansyrox

Ready to be merged

@thePromger thePromger changed the title fix: Fixes a random bug in test_web_socket_raw_benchmark while benchmarking fix: Fixes assert issue in websocket integration tests Oct 30, 2025
Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

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

Love it. Good job @thePromger

@thePromger
Copy link
Contributor Author

thePromger commented Oct 31, 2025

@sansyrox

Do merge it bro

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.

Fix *chika* *chika* Slim Shady in codspeed ci

2 participants