-
Notifications
You must be signed in to change notification settings - Fork 178
feat(examples): websocket autobahn test suit integration (IDF-14864) #960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
|
338e6c6 to
0b8ebbf
Compare
c6f9902 to
20bbe35
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on December 20
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
components/esp_websocket_client/examples/autobahn-testsuite/testee/main/autobahn_testee.c
Outdated
Show resolved
Hide resolved
components/esp_websocket_client/tests/autobahn-testsuite/testee/main/autobahn_testee.c
Show resolved
Hide resolved
components/esp_websocket_client/examples/autobahn-testsuite/run_tests.sh
Outdated
Show resolved
Hide resolved
components/esp_websocket_client/tests/autobahn-testsuite/testee/main/autobahn_testee.c
Show resolved
Hide resolved
components/esp_websocket_client/examples/autobahn-testsuite/scripts/quick_test.sh
Outdated
Show resolved
Hide resolved
20bbe35 to
138574c
Compare
components/esp_websocket_client/examples/autobahn-testsuite/scripts/monitor_serial.py
Outdated
Show resolved
Hide resolved
components/esp_websocket_client/tests/autobahn-testsuite/testee/main/autobahn_testee.c
Show resolved
Hide resolved
138574c to
473ca9f
Compare
components/esp_websocket_client/examples/autobahn-testsuite/scripts/analyze_results.py
Outdated
Show resolved
Hide resolved
components/esp_websocket_client/tests/autobahn-testsuite/testee/main/autobahn_testee.c
Show resolved
Hide resolved
875d3b8 to
048b0c5
Compare
| #define BUFFER_SIZE 16384 // Reduced from 32768 to free memory for accumulator buffer | ||
| #define START_CASE 1 | ||
| #define END_CASE 300 | ||
| // Configure test range here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is misplaced as it refers to the range above. We could have esp_console for this and have this as runtime option.
| @@ -0,0 +1,10 @@ | |||
| # Conditionally require components based on target | |||
| if(${IDF_TARGET} STREQUAL "linux") | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified and more maintenable if we create a base list with common components and extend it with the specific and add that list to the requirements.
| # COMPONENTS must be set before include() to limit component scanning | ||
| if(${IDF_TARGET} STREQUAL "linux") | ||
| set(common_component_dir ../../../../../common_components) | ||
| set(EXTRA_COMPONENT_DIRS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move this to be handled by component manager.
| esp_websocket_client_error(client, "esp_transport_connect() failed with %d, " | ||
| "transport_error=%s, tls_error_code=%i, tls_flags=%i, esp_ws_handshake_status_code=%d, errno=%d", | ||
| result, esp_err_to_name(error_handle->last_error), error_handle->esp_tls_error_code, | ||
| result, error_name ? error_name : "UNKNOWN", error_handle->esp_tls_error_code, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? esp_err_to_name handles inexistent code and from quick code inspection always returns a valid pointer.
| ``` | ||
| This generates a detailed console summary and an HTML summary at `reports/summary.html`. | ||
|
|
||
| **Option 3: Direct File Access** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This are unnecessary instructions.
eefab9a to
bff826f
Compare
bff826f to
6a50440
Compare
| CONFIG_WS_OVER_TLS_MUTUAL_AUTH=n | ||
| CONFIG_WS_OVER_TLS_SERVER_AUTH=n | ||
| CONFIG_EXAMPLE_WIFI_SSID="ImpactHUB Guest" | ||
| CONFIG_EXAMPLE_WIFI_PASSWORD="bethechange1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Hardcoded WiFi credentials committed to repository
Real WiFi credentials are hardcoded in sdkconfig.defaults: CONFIG_EXAMPLE_WIFI_SSID="ImpactHUB Guest" and CONFIG_EXAMPLE_WIFI_PASSWORD="bethechange1". These appear to be actual network credentials that should not be committed to a public repository. The file should use placeholder values like "YourWiFiSSID" and "YourWiFiPassword" instead, matching what the README documentation suggests.
| runs-on: ubuntu-22.04 | ||
| container: espressif/idf:${{ matrix.idf_ver }} | ||
| env: | ||
| TEST_DIR: components/esp_websocket_client/examples/autobahn-testsuite/testee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Workflow uses wrong path for testee directory
The target-test workflow defines TEST_DIR as components/esp_websocket_client/examples/autobahn-testsuite/testee, but all the actual files are added under components/esp_websocket_client/tests/autobahn-testsuite/testee/. This path mismatch will cause the active build_websocket_autobahn job to fail because the directory doesn't exist. The Linux test workflow correctly uses the tests/ path.
|
|
||
| # Use monitor_serial.py to monitor and detect completion | ||
| # Only send URI if CONFIG_WEBSOCKET_URI_FROM_STDIN is enabled | ||
| if python3 scripts/monitor_serial.py --port "$ESP_PORT" ${URI_FROM_STDIN:+--uri "$URI_FROM_STDIN"} --timeout 2400; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Script references monitor_serial.py from wrong directory
run_tests.sh in tests/autobahn-testsuite/ calls scripts/monitor_serial.py expecting it to be at tests/autobahn-testsuite/scripts/monitor_serial.py. However, the file is actually added at examples/autobahn-testsuite/scripts/monitor_serial.py - a completely different directory. The script will fail with "file not found" when executed. The file needs to be moved to the tests/ directory structure or the path in run_tests.sh corrected.
Note
Adds Autobahn WebSocket testsuite (configs, scripts, testee app) and CI workflows, plus minor logging/build tweaks.
components/esp_websocket_client/tests/autobahn-testsuitewithdocker-compose.yml, server configs, and scripts (run_tests.sh,monitor_serial.py,generate_summary.py).testee/withCMakeLists.txt,Kconfig.projbuild,autobahn_testee.c,sdkconfig.*)..github/workflows/autobahn__linux-test.yml) and target build (.github/workflows/autobahn__target-test.yml).esp_err_to_name()with null-checks in write/read/connect/poll paths.esp_timercomponent nowREQUIRES esp_common; header includesesp_err.h.tests/unit/CMakeLists.txt(EXTRA_COMPONENT_DIRS).Written by Cursor Bugbot for commit 6a50440. This will update automatically on new commits. Configure here.