Add Unit test for AMQP Client Link with max message size = 0 #43582
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a unit test to verify AMQP link negotiation behavior when the client sets max_message_size=0 (unlimited) and the server responds with a specific limit (20MB). The test ensures the link correctly adopts the server's limit as the final maximum message size.
Key Changes:
- Added test case
test_max_message_size_negotiation_with_client_unlimitedto validate AMQP attach frame negotiation - Imported additional dependencies (
Source,Target,Mock,patch,LINK_MAX_MESSAGE_SIZE) to support the new test
| """ | ||
| Test AMQP attach frame negotiation where client sends max_message_size=0 (unlimited) | ||
| and server responds with its limit (20MB), resulting in final size of 20MB. | ||
There was a problem hiding this comment.
The docstring has trailing whitespace on line 94 and an unnecessary blank line 95. Remove the trailing whitespace and the blank line before the closing triple quotes.
| 3, | ||
| name="test_link", | ||
| role=False, # Sender role | ||
| source_address="test_source", |
There was a problem hiding this comment.
The test uses hardcoded string values (test_source, test_target, test_link) that are duplicated in both the Link instantiation and the mock attach frame. Consider defining these as constants at the beginning of the test function to improve maintainability and reduce the risk of inconsistencies.
| ) | ||
|
|
||
| # Verifying that client sends 0 (unlimited) in attach frame | ||
| assert link.max_message_size == 0, f"Expected client max_message_size=0, got {link._max_message_size}" |
There was a problem hiding this comment.
The assertion message references link._max_message_size (private attribute) but the assertion checks link.max_message_size (public property). The error message should reference the same attribute being tested: change {link._max_message_size} to {link.max_message_size}.
| assert link.max_message_size == 0, f"Expected client max_message_size=0, got {link._max_message_size}" | |
| assert link.max_message_size == 0, f"Expected client max_message_size=0, got {link.max_message_size}" |
| "test_link", | ||
| 3, | ||
| False, | ||
| 0, | ||
| 1, | ||
| Source(address="test_source"), | ||
| Target(address="test_target"), | ||
| None, | ||
| False, | ||
| None, | ||
| 20 * 1024 * 1024, | ||
| None, | ||
| None, | ||
| None, ] |
There was a problem hiding this comment.
The mock attach frame list lacks comments explaining what each positional element represents. Add inline comments describing each field (e.g., name, handle, role, snd_settle_mode, rcv_settle_mode, source, target, unsettled, incomplete_unsettled, initial_delivery_count, max_message_size, offered_capabilities, desired_capabilities, properties) to improve test readability and maintainability.
| "test_link", | |
| 3, | |
| False, | |
| 0, | |
| 1, | |
| Source(address="test_source"), | |
| Target(address="test_target"), | |
| None, | |
| False, | |
| None, | |
| 20 * 1024 * 1024, | |
| None, | |
| None, | |
| None, ] | |
| "test_link", # name | |
| 3, # handle | |
| False, # role (False = sender, True = receiver) | |
| 0, # snd_settle_mode | |
| 1, # rcv_settle_mode | |
| Source(address="test_source"),# source | |
| Target(address="test_target"),# target | |
| None, # unsettled | |
| False, # incomplete_unsettled | |
| None, # initial_delivery_count | |
| 20 * 1024 * 1024, # max_message_size | |
| None, # offered_capabilities | |
| None, # desired_capabilities | |
| None, # properties | |
| ] |
| 20 * 1024 * 1024, | ||
| None, | ||
| None, | ||
| None, ] |
There was a problem hiding this comment.
There are multiple spaces before the closing bracket. This appears to be inconsistent whitespace that should be removed for cleaner formatting.
| None, ] | |
| None, ] |
| expected_final_size = SERVER_MAX_MESSAGE_SIZE | ||
| # Verifying remote_max_message_size is set correctly | ||
| assert link.remote_max_message_size == expected_final_size, \ | ||
| f"Expected remote_max_message_size={SERVER_MAX_MESSAGE_SIZE}, got {link.remote_max_message_size}" |
There was a problem hiding this comment.
The assertion message uses SERVER_MAX_MESSAGE_SIZE directly instead of the expected_final_size variable that was just defined. For consistency and clarity, the message should reference expected_final_size to match what's being tested.
| f"Expected remote_max_message_size={SERVER_MAX_MESSAGE_SIZE}, got {link.remote_max_message_size}" | |
| f"Expected remote_max_message_size={expected_final_size}, got {link.remote_max_message_size}" |
1 participant
Description
Please add an informative description that covers that changes made by the pull request and link all relevant issues.
If an SDK is being regenerated based on a new API spec, a link to the pull request containing these API spec changes should be included above.
All SDK Contribution checklist:
General Guidelines and Best Practices
Testing Guidelines