Sync changes from internal dev branch.#89
Conversation
PiperOrigin-RevId: 896685330
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the existing SCC 2G Wi-Fi Direct (WFD) STA test with a new XCC 2G WFD STA test, updating the test suite and incrementing the suite version. The new xcc_2g_wfd_sta_test.py file introduces a test designed to cover both SCC and MCC scenarios for WFD medium with 2G channel and JP country code. The review highlights a critical issue where mcc_file_size_kb and scc_file_size_kb constants are swapped in transfer_file_for_unknown_concurrency_mode, which could lead to incorrect test parameters. Additionally, there are suggestions to correct the docstring for test_xcc_2g_wfd_sta to accurately reflect "Wifi XCC", address a logical inconsistency where WifiConcurrencyMode.UNKNOWN is listed as a valid mode despite being an unexpected state, and fix a typo in an error message.
| mcc_file_size_kb=nc_constants.NC_SCC_2G_TRANSFER_FILE_SIZE_KB, | ||
| mcc_timeout=constants.WIFI_2G_20M_PAYLOAD_TRANSFER_TIMEOUT, | ||
| scc_file_size_kb=nc_constants.NC_MCC_5G_D2D_2G_STA_TRANSFER_FILE_SIZE_KB, | ||
| scc_timeout=constants.WIFI_100M_PAYLOAD_TRANSFER_TIMEOUT, |
There was a problem hiding this comment.
The arguments passed to transfer_file_for_unknown_concurrency_mode appear to be swapped. mcc_file_size_kb is receiving an SCC constant, while scc_file_size_kb is receiving an MCC constant. This will lead to incorrect test parameters being used if the concurrency mode is unknown.
| mcc_file_size_kb=nc_constants.NC_SCC_2G_TRANSFER_FILE_SIZE_KB, | |
| mcc_timeout=constants.WIFI_2G_20M_PAYLOAD_TRANSFER_TIMEOUT, | |
| scc_file_size_kb=nc_constants.NC_MCC_5G_D2D_2G_STA_TRANSFER_FILE_SIZE_KB, | |
| scc_timeout=constants.WIFI_100M_PAYLOAD_TRANSFER_TIMEOUT, | |
| mcc_file_size_kb=nc_constants.NC_MCC_5G_D2D_2G_STA_TRANSFER_FILE_SIZE_KB, | |
| mcc_timeout=constants.WIFI_100M_PAYLOAD_TRANSFER_TIMEOUT, | |
| scc_file_size_kb=nc_constants.NC_SCC_2G_TRANSFER_FILE_SIZE_KB, | |
| scc_timeout=constants.WIFI_2G_20M_PAYLOAD_TRANSFER_TIMEOUT, |
| max_consecutive_error=_MAX_CONSECUTIVE_ERROR, | ||
| ) | ||
| def test_xcc_2g_wfd_sta(self): | ||
| """Test the performance for Wifi SCC with 2G WFD and STA.""" |
There was a problem hiding this comment.
The docstring for test_xcc_2g_wfd_sta mentions "Wifi SCC", but this test class is designed for "Wifi XCC" and covers both SCC and MCC scenarios as per the class docstring. The method docstring should be updated for accuracy.
| """Test the performance for Wifi SCC with 2G WFD and STA.""" | |
| """Test the performance for Wifi XCC with 2G WFD and STA.""" |
| valid_concurrency_modes=[ | ||
| constants.WifiConcurrencyMode.SCC_2G, | ||
| constants.WifiConcurrencyMode.MCC_5G_P2P_2G_STA, | ||
| constants.WifiConcurrencyMode.UNKNOWN, | ||
| ], |
There was a problem hiding this comment.
WifiConcurrencyMode.UNKNOWN is included in the valid_concurrency_modes list, but later in the code (line 255), it is stated that this mode "should not happen". If UNKNOWN is considered a failure state or an unexpected condition, it should be removed from the valid modes list to allow the assertion to catch it.
valid_concurrency_modes=[
constants.WifiConcurrencyMode.SCC_2G,
constants.WifiConcurrencyMode.MCC_5G_P2P_2G_STA,
],| ], | ||
| test_result=self.current_test_result, | ||
| additional_error_message=( | ||
| 'If the device can detected as indoor, the concurrency mode should' |
There was a problem hiding this comment.
Typo in error message: "can detected" should be "is detected" or "can be detected".
| 'If the device can detected as indoor, the concurrency mode should' | |
| 'If the device is detected as indoor, the concurrency mode should' |
2 participants
No description provided.