Update test_waitinglist.py #597
Conversation
. Added a new class `WaitingList` that manages a queue of users using a priority queue to ensure FIFO order.
- Methods:
- `add_user(user_id)`: Adds a user to the waiting list with a timestamp.
- `remove_user()`: Removes and returns the next user from the waiting list.
- `peek_next_user()`: Returns the next user without removing them from the list.
2. Introduced a new test function `test_waiting_list()` to validate the functionality of the `WaitingList` class.
- Tests include adding and removing users, checking the next user, and handling an empty queue case.
- Validates that users are removed in the correct order based on their addition time.
The new functionality is integrated at the end of the existing test file, ensuring it does not interfere with the existing tests for the `WaitingListEntry` model.
Reviewer's Guide by SourceryThis pull request introduces a No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @td15 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider creating a separate file for the
WaitingListclass instead of adding it to the test file. - Instead of running the test directly at the end of the file, integrate it with the existing test framework.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert wl.peek_next_user() == "User1", "Test Failed: User1 should be first" | ||
| assert wl.remove_user() == "User1", "Test Failed: User1 should be removed first" | ||
| assert wl.remove_user() == "User2", "Test Failed: User2 should be removed second" | ||
| assert wl.remove_user() == "User3", "Test Failed: User3 should be removed third" |
There was a problem hiding this comment.
suggestion (testing): Consider adding more comprehensive tests for edge cases.
It would be beneficial to include tests for concurrent access to the waiting list to ensure thread safety. For example, simulate multiple users being added and removed from the list concurrently using multiple threads. This would help uncover potential race conditions or deadlocks.
Suggested implementation:
# Test: Adding and removing users
wl.add_user("User1")
wl.add_user("User2")
wl.add_user("User3")
assert wl.peek_next_user() == "User1", "Test Failed: User1 should be first"
assert wl.remove_user() == "User1", "Test Failed: User1 should be removed first"
assert wl.remove_user() == "User2", "Test Failed: User2 should be removed second" # Test: Concurrent access tests for WaitingList
import threading
def test_concurrent_access():
concurrent_wl = WaitingList()
num_users = 20
# Function to add a user concurrently
def add_user(i):
concurrent_wl.add_user(f"User{i}")
add_threads = []
for i in range(num_users):
t = threading.Thread(target=add_user, args=(i,))
add_threads.append(t)
t.start()
for t in add_threads:
t.join()
# Function to remove a user concurrently
removed_users = []
def remove_user():
user = concurrent_wl.remove_user()
if user is not None:
removed_users.append(user)
remove_threads = []
for _ in range(num_users):
t = threading.Thread(target=remove_user)
remove_threads.append(t)
t.start()
for t in remove_threads:
t.join()
# Verify that all users have been removed in FIFO order
expected_users = [f"User{i}" for i in range(num_users)]
assert removed_users == expected_users, "Test Failed: Concurrent removal did not maintain FIFO order"
test_concurrent_access()Ensure that your WaitingList.add_user method properly increments the counter and that it uses self.lock to synchronize access when adding or removing users.
| wl.add_user("User1") | ||
| wl.add_user("User2") | ||
| wl.add_user("User3") |
There was a problem hiding this comment.
suggestion (testing): Use a more robust method for testing FIFO order.
Instead of relying solely on time.sleep(), consider using a monotonic clock like time.monotonic() to generate timestamps. This avoids potential issues with system clock adjustments during testing. Also, consider a test case where you add a large number of users (e.g., 1000) with very small time differences between additions to thoroughly validate the FIFO ordering.
Suggested implementation:
import time
# (Other existing imports) # Test: Users waiting for different durations using a large number of users and time.monotonic for precise timestamps
# Continue the current test by adding a single user
wl.add_user("UserA")
assert wl.peek_next_user() == "UserA", "Test Failed: UserA should be next"
# Now, add 1000 users using monotonic timestamps to simulate very small time differences
for i in range(1000):
# Capture monotonic time (if WaitingList internally relies on timestamps, this simulates a small delay)
current_time = time.monotonic()
# Add a user; position in queue is determined by the order of addition
wl.add_user(f"LargeUser{i}")
# Validate FIFO ordering for the 1000 users
for i in range(1000):
expected = f"LargeUser{i}"
actual = wl.remove_user()
assert actual == expected, f"Test Failed: Expected {expected} but got {actual}"Ensure that WaitingList uses the appropriate timestamp source (e.g., time.monotonic()) if it requires precise timing comparisons. This test case assumes that the WaitingList implementation orders users by the time they were added. Adjust the code accordingly if the internal implementation differs.
src/tests/api/test_waitinglist.py
Outdated
| # Run tests | ||
| test_waiting_list() |
There was a problem hiding this comment.
suggestion (testing): Integrate the test_waiting_list function with the test runner.
Directly calling test_waiting_list() at the end of the file bypasses the test runner. Rename the function to start with test_ (if it isn't already) and let the test runner discover and execute it. This ensures proper test discovery, reporting, and integration with CI/CD pipelines. Remove the direct call to test_waiting_list().
key update points: Single Test File: Keeping all tests related to the WaitingList class in one file makes it easier to maintain and understand. Comprehensive Testing: The tests now cover basic functionality, concurrent access, and handling a large number of users, ensuring that the WaitingList class behaves correctly under various conditions. No Direct Calls: As noted in your original code, there are no direct calls to the test functions. This allows pytest to discover and run the tests automatically.
| actual = wl.remove_user() | ||
| assert actual == expected, f"Test Failed: Expected {expected} but got {actual}" | ||
|
|
||
| # Note: No direct call to test_waiting_list() here; pytest will discover and run the tests. |
There was a problem hiding this comment.
Next Steps:
Create a Separate File for the WaitingList Class: If you haven't already, create a separate file for the WaitingList class to keep your code organized.
Run Your Tests: Use pytest to run your tests and ensure everything works as expected.
Review and Refactor: After running the tests, review the results and refactor any code as necessary based on the outcomes.
. Added a new class
WaitingListthat manages a queue of users using a priority queue to ensure FIFO order.add_user(user_id): Adds a user to the waiting list with a timestamp. -remove_user(): Removes and returns the next user from the waiting list. -peek_next_user(): Returns the next user without removing them from the list.test_waiting_list()to validate the functionality of theWaitingListclass.The new functionality is integrated at the end of the existing test file, ensuring it does not interfere with the existing tests for the
WaitingListEntrymodel.Summary by Sourcery
Implement a thread-safe WaitingList class with priority queue-based user management for the waiting list functionality
New Features:
Tests: