Strengthen WLED request payload tests#2050
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a new test helper Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 58 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2050 +/- ##
===========================================
+ Coverage 58.61% 97.20% +38.59%
===========================================
Files 6 8 +2
Lines 662 1075 +413
Branches 143 112 -31
===========================================
+ Hits 388 1045 +657
+ Misses 270 20 -250
- Partials 4 10 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_wled.py (1)
33-44: ⚡ Quick winScope this helper to payload semantics, not transport defaults.
assert_post_payload()also locks header values andallow_redirects, which can cause unrelated failures when HTTP defaults evolve. Since this PR is about payload regression checks, keep this helper focused on URL/method/body and validate transport defaults in dedicated request tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_wled.py` around lines 33 - 44, The helper assert_post_payload currently asserts transport details (headers and allow_redirects) which couples payload tests to HTTP defaults; update assert_post_payload(mocked, path, expected) to only assert the URL, method ("POST") and serialized body (orjson.dumps(expected)) on the mocked call and remove the headers and allow_redirects checks, so payload semantics remain scoped to URL/method/body; keep transport/header assertions to separate, dedicated request-level tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_wled.py`:
- Around line 33-44: The helper assert_post_payload currently asserts transport
details (headers and allow_redirects) which couples payload tests to HTTP
defaults; update assert_post_payload(mocked, path, expected) to only assert the
URL, method ("POST") and serialized body (orjson.dumps(expected)) on the mocked
call and remove the headers and allow_redirects checks, so payload semantics
remain scoped to URL/method/body; keep transport/header assertions to separate,
dedicated request-level tests.
There was a problem hiding this comment.
Pull request overview
This PR strengthens the WLED client request tests by asserting the actual serialized POST payloads sent to /json/state, reducing the chance of regressions that still return “valid” responses but send incorrect data.
Changes:
- Added a shared
assert_post_payloadhelper for POST request assertions. - Updated request-focused tests (master/segment/preset/playlist/transition/live/sync/nightlight) to verify the exact outgoing request payload.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert_post_payload( | ||
| mocked, "http://example.com/json/state", {"ps": 2, "v": True} | ||
| ) |
There was a problem hiding this comment.
The playlist tests assert the outgoing payload uses {"ps": ...}. Elsewhere in the codebase the playlist field is consistently aliased as pl (e.g., State.playlist_id uses alias "pl" while presets use "ps"), so these assertions may be locking in an incorrect request contract. Please double-check the correct WLED API field for activating a playlist and update the expected payloads (and client code if needed) to match.
| assert_post_payload( | ||
| mocked, "http://example.com/json/state", {"ps": 2, "v": True} | ||
| ) |
There was a problem hiding this comment.
The playlist tests assert the outgoing payload uses {"ps": ...}. Elsewhere in the codebase the playlist field is consistently aliased as pl (e.g., State.playlist_id uses alias "pl" while presets use "ps"), so these assertions may be locking in an incorrect request contract. Please double-check the correct WLED API field for activating a playlist and update the expected payloads (and client code if needed) to match.
| assert_post_payload( | ||
| mocked, "http://example.com/json/state", {"ps": 2, "v": True} | ||
| ) |
There was a problem hiding this comment.
The playlist tests assert the outgoing payload uses {"ps": ...}. Elsewhere in the codebase the playlist field is consistently aliased as pl (e.g., State.playlist_id uses alias "pl" while presets use "ps"), so these assertions may be locking in an incorrect request contract. Please double-check the correct WLED API field for activating a playlist and update the expected payloads (and client code if needed) to match.
mik-laj
force-pushed
the
fix/wled-request-payload-tests
branch
from
cf42b5c to
5bd612b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Proposed Changes
These tests originally only verified that the request succeeded and returned the expected response, but they did not verify what was actually sent to WLED. That left room for regressions where the client could send the wrong payload, omit required fields, or serialize data incorrectly while the tests still passed. Stronger assertions make the tests check the request contract itself, so they catch breaking changes in the outgoing payload, not just in the response handling.
Additionally, the test code was refactored to make fuller use of pytest fixtures and helper functions (like prepare_wled_with_device), which significantly reduces indentation and improves readability. The tests are now easier to maintain and follow, with less boilerplate setup code.
This is also the first step toward a refactor that uses models.State to build the payloads sent to WLED. models.State is already used for deserialization today, but it can also be used for serialization. That would remove a lot of magic field names like cct, sel, sx, and similar, making the request-building code easier to maintain and less error-prone.
Related Issues
Summary by CodeRabbit