Skip to content

[tests] fix discovery task payload in api_network_diagnostic_task_test - #3656

Open
jwhui wants to merge 1 commit into
openthread:mainfrom
jwhui:fix-rest-diag-test
Open

jwhui wants to merge 1 commit into
openthread:mainfrom
jwhui:fix-rest-diag-test

Conversation

@jwhui

@jwhui jwhui commented Oct 23, 2026

Copy link
Copy Markdown
Member

In api_network_diagnostic_task_test() (added in #3632), POST /api/actions used the invalid action type 'updateNetworkDevicesTask' and omitted the required 'deviceCount' attribute, causing RestWebServer::ApiActionsPostHandler to reject the request with HTTP 422. Because tests/rest/test-rest-server previously checked only Expect's OS wait error flag ($error) and not the process exit status ($status), the unhandled HTTPError in test_rest.py was silently ignored.

This commit:

  • Uses 'updateDeviceCollectionTask' with 'deviceCount': 1 in api_network_diagnostic_task_test().
  • Checks $status != 0 (and adds an explicit timeout) when waiting for test_rest.py in tests/rest/test-rest-server.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the REST functional tests by adding timeout handling to the test server and modifying the device collection update task parameters. The review feedback suggests increasing the expect timeout in the test server to 150 seconds to prevent flaky failures and increasing the wait_for_action timeout in the Python test script to 15 seconds to account for potential latency.

Comment thread tests/rest/test-rest-server Outdated
Comment thread tests/rest/test_rest.py Outdated
@codecov

codecov Bot commented Oct 23, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.42%. Comparing base (2b41187) to head (b23abbb).
⚠️ Report is 1708 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3656      +/-   ##
==========================================
- Coverage   55.77%   48.42%   -7.36%     
==========================================
  Files          87      153      +66     
  Lines        6890    20460   +13570     
  Branches        0     1625    +1625     
==========================================
+ Hits         3843     9907    +6064     
- Misses       3047     9931    +6884     
- Partials        0      622     +622     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jwhui
jwhui force-pushed the fix-rest-diag-test branch from 174b8df to 9c9869e Compare October 23, 2026 03:40
@jwhui
jwhui marked this pull request as ready for review October 23, 2026 23:21
- Use `updateDeviceCollectionTask` with `"deviceCount": 1` in
  `api_network_diagnostic_task_test()` in `tests/rest/test_rest.py`
  instead of the invalid action type `updateNetworkDevicesTask`.
- Exit with `$status` in `tests/rest/test-rest-server` so `test_rest.py`
  failures are not masked by `kill $AGENT_PID`.
- Initialize TLV arrays and counts in `NetworkDiagHandler`, reset
  `mDiagReqTlvsOmitableCount` in `SetDefaultTlvs()`, and bound query
  iteration by `mDiagQueryTlvsCount` in `HandleNextDiagQuery()`.
@jwhui
jwhui force-pushed the fix-rest-diag-test branch from 9c9869e to b23abbb Compare October 23, 2026 23:24
@jwhui

jwhui commented Oct 23, 2026

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the NetworkDiagHandler class by properly initializing several member variables in its constructor, setting default TLV values, and changing a range-based loop to an index-based loop using mDiagQueryTlvsCount. Additionally, it updates the REST API integration tests by adjusting the test task type to updateDeviceCollectionTask, increasing timeouts, and handling test execution status and timeouts more robustly. There are no review comments to address, so no further feedback is provided.

@jwhui
jwhui requested a review from abtink October 24, 2026 06:43

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant