feat: full depth on expand_light_groups — tests + Lightener-aware warning#13
Conversation
Adds 5 unit tests filling the P2 gap from /plan-eng-review on PR #12. The new option's non-default branch had zero coverage; these lock in behavior before users start flipping it (e.g. for Lightener, Hue rooms, or Zigbee2MQTT groups). - test_expand_light_groups_const_wiring: default True, VALIDATION_TUPLES, STEP_OPTIONS[workarounds] UI wiring, DOCS entry — catches the exact class of bug that required the follow-up STEP_OPTIONS fix in #12. - test_expand_light_groups_false_keeps_group_entity: group stays in manager.lights, members are NOT tracked individually. - test_expand_light_groups_true_expands_to_members: default behavior still produces the member-level tracking, preventing accidental regression. - test_expand_light_groups_false_non_group_unaffected: no-op for non-group lights — safe for users who flip the flag without groups. - test_expand_light_groups_false_group_turn_on_reaches_group: the core behavioral guarantee — a user turn_on on a group entity must reach the group, not get silently rerouted to members. This is what unblocks Lightener. Full suite: 35 pre-existing failures (unchanged baseline), 0 new regressions, 1 intentional xfail from PR basnijholt#1379.
📝 Walkthrough
WalkthroughAdds new tests for the Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_switch.py`:
- Around line 3230-3232: The test currently only asserts that members
"light.light_4" and "light.light_5" exist in switch.manager.lights; add a
negative assertion to ensure the group itself was removed in the True path —
e.g. after the two existing asserts add an assertion like assert "<group_id>"
not in switch.manager.groups (replace <group_id> with the actual group
identifier used earlier in the test) so the test fails if the group is retained
alongside its members.
- Around line 3283-3307: The assertion currently counts the original
user-triggered light.turn_on (from _turn_on_and_track_event_contexts) as
evidence the group was targeted, allowing regressions; fix by excluding the
initial user call when searching events: capture the original context id (e.g.,
orig_ctx = events[0].context.id) and change the group_targeted check to look for
any event where e.data.get("service")==SERVICE_TURN_ON and "light.light_group"
in _targets(e) and e.context.id != orig_ctx (or e.context.parent_id != orig_ctx
if the adaptation should appear as a child), using the existing events,
_targets, SERVICE_TURN_ON, ATTR_ENTITY_ID, and "light.light_group" symbols to
locate and update the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9baf0edc-29e2-439f-a206-8c46a3de4eb0
📒 Files selected for processing (1)
tests/test_switch.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Run pytest (2025.10.4, 3.13.2)
- GitHub Check: Run pytest (2025.9.4, 3.13.2)
- GitHub Check: Run pytest (2025.5.3, 3.13.2)
- GitHub Check: Run pytest (2025.2.5, 3.13.0)
- GitHub Check: Run pytest (2025.8.3, 3.13.2)
- GitHub Check: Run pytest (dev, 3.14.2)
- GitHub Check: Run pytest (2025.3.4, 3.13.0)
- GitHub Check: Run pytest (2025.11.3, 3.13.2)
- GitHub Check: Run pytest (2025.7.4, 3.13.2)
- GitHub Check: Run pytest (2025.1.4, 3.12.0)
- GitHub Check: Run pytest (2024.12.5, 3.12.0)
- GitHub Check: Run pytest (2026.1.3, 3.13.2)
- GitHub Check: Run pytest (2026.2.2, 3.13.2)
- GitHub Check: Run pytest (2025.4.4, 3.13.0)
- GitHub Check: Run pytest (2025.12.5, 3.13.2)
- GitHub Check: Run pytest (2025.6.3, 3.13.2)
- GitHub Check: build (linux/arm64)
- GitHub Check: markdown-code-runner
- GitHub Check: build (linux/amd64)
- GitHub Check: pre-commit
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
Focus on: error handling, type safety, async correctness, and missing edge cases. Flag any hardcoded credentials or API keys.
Files:
tests/test_switch.py
**/tests/**
⚙️ CodeRabbit configuration file
Prioritize missing edge case coverage and regression risks over style nits.
Files:
tests/test_switch.py
🪛 Ruff (0.15.10)
tests/test_switch.py
[warning] 3159-3160: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
[warning] 3161-3167: import should be at the top-level of a file
(PLC0415)
[warning] 3193-3193: import should be at the top-level of a file
(PLC0415)
[warning] 3216-3216: import should be at the top-level of a file
(PLC0415)
[warning] 3235-3236: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
[warning] 3237-3237: import should be at the top-level of a file
(PLC0415)
[warning] 3260-3260: import should be at the top-level of a file
(PLC0415)
🔇 Additional comments (2)
tests/test_switch.py (2)
3158-3184: Strong wiring regression coverage for the new option.This test correctly locks default value, validation tuple wiring, config-step exposure, and docs presence in one place.
3186-3212: Good branch coverage forexpand_light_groups=Falsebehavior.These tests meaningfully protect the non-default path (group retained vs non-group no-op), which is the high-risk regression surface.
Also applies to: 3234-3252
| assert "light.light_4" in switch.manager.lights | ||
| assert "light.light_5" in switch.manager.lights | ||
|
|
There was a problem hiding this comment.
Add the missing negative assertion for group removal in the True path.
At Line 3230-3232 you only assert member presence. The test still passes if the group is incorrectly retained alongside members.
Proposed tightening
assert "light.light_4" in switch.manager.lights
assert "light.light_5" in switch.manager.lights
+ assert "light.light_group" not in switch.manager.lights📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert "light.light_4" in switch.manager.lights | |
| assert "light.light_5" in switch.manager.lights | |
| assert "light.light_4" in switch.manager.lights | |
| assert "light.light_5" in switch.manager.lights | |
| assert "light.light_group" not in switch.manager.lights |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_switch.py` around lines 3230 - 3232, The test currently only
asserts that members "light.light_4" and "light.light_5" exist in
switch.manager.lights; add a negative assertion to ensure the group itself was
removed in the True path — e.g. after the two existing asserts add an assertion
like assert "<group_id>" not in switch.manager.groups (replace <group_id> with
the actual group identifier used earlier in the test) so the test fails if the
group is retained alongside its members.
| events = await _turn_on_and_track_event_contexts( | ||
| hass, | ||
| "user-triggered-on", | ||
| "light.light_group", | ||
| return_full_events=True, | ||
| ) | ||
|
|
||
| def _targets(ev): | ||
| target = ev.data.get("service_data", {}).get(ATTR_ENTITY_ID) | ||
| if isinstance(target, str): | ||
| return [target] | ||
| if isinstance(target, list): | ||
| return target | ||
| return [] | ||
|
|
||
| group_targeted = any( | ||
| "light.light_group" in _targets(e) | ||
| for e in events | ||
| if e.data.get("service") == SERVICE_TURN_ON | ||
| ) | ||
| assert group_targeted, ( | ||
| "Expected at least one light.turn_on targeting the group entity when " | ||
| "expand_light_groups=False, but the group was never addressed. " | ||
| f"Events: {[(e.data.get('service'), _targets(e)) for e in events]}" | ||
| ) |
There was a problem hiding this comment.
Current event assertion can pass on regressions (false positive).
At Line 3283 and Line 3298-3303, the check accepts any group-targeted light.turn_on. That includes the original user call emitted by this test itself, so it can still pass even if adaptation is rerouted to members.
Proposed fix to make the assertion regression-proof
events = await _turn_on_and_track_event_contexts(
hass,
"user-triggered-on",
"light.light_group",
return_full_events=True,
)
+ await asyncio.gather(*switch.manager.adaptation_tasks)
+ await hass.async_block_till_done()
def _targets(ev):
target = ev.data.get("service_data", {}).get(ATTR_ENTITY_ID)
if isinstance(target, str):
return [target]
if isinstance(target, list):
return target
return []
- group_targeted = any(
- "light.light_group" in _targets(e)
- for e in events
- if e.data.get("service") == SERVICE_TURN_ON
- )
- assert group_targeted, (
- "Expected at least one light.turn_on targeting the group entity when "
- "expand_light_groups=False, but the group was never addressed. "
- f"Events: {[(e.data.get('service'), _targets(e)) for e in events]}"
- )
+ turn_on_events = [e for e in events if e.data.get("service") == SERVICE_TURN_ON]
+ user_turn_on_events = [e for e in turn_on_events if e.context.id == "user-triggered-on"]
+ assert any("light.light_group" in _targets(e) for e in user_turn_on_events)
+
+ al_turn_on_events = [e for e in turn_on_events if is_our_context_id(e.context.id)]
+ assert not any(
+ any(member in _targets(e) for member in ("light.light_4", "light.light_5"))
+ for e in al_turn_on_events
+ ), f"Unexpected AL member reroute events: {[(e.context.id, _targets(e)) for e in al_turn_on_events]}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_switch.py` around lines 3283 - 3307, The assertion currently
counts the original user-triggered light.turn_on (from
_turn_on_and_track_event_contexts) as evidence the group was targeted, allowing
regressions; fix by excluding the initial user call when searching events:
capture the original context id (e.g., orig_ctx = events[0].context.id) and
change the group_targeted check to look for any event where
e.data.get("service")==SERVICE_TURN_ON and "light.light_group" in _targets(e)
and e.context.id != orig_ctx (or e.context.parent_id != orig_ctx if the
adaptation should appear as a child), using the existing events, _targets,
SERVICE_TURN_ON, ATTR_ENTITY_ID, and "light.light_group" symbols to locate and
update the logic.
Given the Lightener curve card work depends on expand_light_groups=False being honored everywhere, this commit closes three silent-failure gaps from the follow-up review on PR #13: Gap 1 — adapt-cycle dispatch test test_expand_light_groups_false_adapt_cycle_dispatches_to_group Spies on _adapt_light to lock in that the periodic adaptation cycle dispatches to the GROUP entity_id, not its members, when flag=False. Without this, the intercept path could behave correctly while the time-interval-driven adaptation silently bypasses Lightener curves on every adapt cycle. Gap 2 — manual-control attribution test test_expand_light_groups_false_no_false_manual_control With take_over_control=True + flag=False, internal member state changes (Lightener fan-out, Hue bridge sync) must NOT populate manager.manual_control. Otherwise every adapt cycle would mark members as manually controlled and the next cycle would skip them entirely — a slow death spiral with curves that eventually never fire. Gap 3 — one-time warning when AL expands a light group switch.py: AdaptiveSwitch._expand_light_groups now emits a WARNING (debounced via manager.expand_light_groups_warned set) pointing the user at the expand_light_groups: false flag. Fires only from the switch-configured code path; utility expansions in _switches_with_lights stay silent so matching doesn't spam the log. test_expand_light_groups_true_emits_warning_once asserts the warning fires exactly once per entity per HA run. test_expand_light_groups_false_does_not_warn asserts the warning stays silent for users who already opted in to the fix. Full suite: 35 pre-existing failures (unchanged baseline), 0 new regressions. 9/9 new expand_light_groups tests passing. 1 intentional xfail from basnijholt#1379 untouched.
florianhorner
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_switch.py`:
- Around line 3464-3485: The test currently never exercises the "warn once"
dedupe branch because switch.lights is mutated during setup; change the first
assertion to require exactly one WARNING (assert len(warnings) == 1) and before
calling switch._expand_light_groups() the second time, re-seed the group
membership by restoring switch.lights to include the original group member ID
(e.g., "light.light_group") so the expansion path runs again and
expand_light_groups_warned logic is exercised; keep using caplog.clear(), call
await hass.async_block_till_done() after the second expansion, then assert that
repeat_warnings (filtered by lowering the message and matching "expanding light
group" and "light.light_group") is empty.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2b13c642-341d-4830-a233-d06115f1129a
📒 Files selected for processing (2)
custom_components/adaptive_lighting/switch.pytests/test_switch.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: build
- GitHub Check: Run pytest (2026.2.2, 3.13.2)
- GitHub Check: Run pytest (2025.12.5, 3.13.2)
- GitHub Check: Run pytest (2025.9.4, 3.13.2)
- GitHub Check: Run pytest (2025.5.3, 3.13.2)
- GitHub Check: Run pytest (2026.1.3, 3.13.2)
- GitHub Check: Run pytest (2025.6.3, 3.13.2)
- GitHub Check: Run pytest (2025.11.3, 3.13.2)
- GitHub Check: Run pytest (2025.4.4, 3.13.0)
- GitHub Check: Run pytest (2025.1.4, 3.12.0)
- GitHub Check: pre-commit
- GitHub Check: Run pytest (2025.8.3, 3.13.2)
- GitHub Check: Run pytest (2025.7.4, 3.13.2)
- GitHub Check: Run pytest (dev, 3.14.2)
- GitHub Check: Run pytest (2025.2.5, 3.13.0)
- GitHub Check: Run pytest (2025.3.4, 3.13.0)
- GitHub Check: Run pytest (2024.12.5, 3.12.0)
- GitHub Check: Run pytest (2025.10.4, 3.13.2)
- GitHub Check: build (linux/amd64)
- GitHub Check: markdown-code-runner
- GitHub Check: build (linux/arm64)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
⚙️ CodeRabbit configuration file
Focus on: error handling, type safety, async correctness, and missing edge cases. Flag any hardcoded credentials or API keys.
Files:
custom_components/adaptive_lighting/switch.pytests/test_switch.py
**/tests/**
⚙️ CodeRabbit configuration file
Prioritize missing edge case coverage and regression risks over style nits.
Files:
tests/test_switch.py
🪛 Ruff (0.15.10)
tests/test_switch.py
[warning] 3159-3160: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
[warning] 3161-3167: import should be at the top-level of a file
(PLC0415)
[warning] 3193-3193: import should be at the top-level of a file
(PLC0415)
[warning] 3216-3216: import should be at the top-level of a file
(PLC0415)
[warning] 3235-3236: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
[warning] 3237-3237: import should be at the top-level of a file
(PLC0415)
[warning] 3260-3260: import should be at the top-level of a file
(PLC0415)
[warning] 3324-3324: import should be at the top-level of a file
(PLC0415)
[warning] 3394-3394: import should be at the top-level of a file
(PLC0415)
[warning] 3441-3441: import should be at the top-level of a file
(PLC0415)
[warning] 3443-3443: import should be at the top-level of a file
(PLC0415)
[warning] 3490-3492: Multi-line docstring closing quotes should be on a separate line
Move closing quotes to new line
(D209)
[warning] 3493-3493: import should be at the top-level of a file
(PLC0415)
[warning] 3495-3495: import should be at the top-level of a file
(PLC0415)
🔇 Additional comments (1)
custom_components/adaptive_lighting/switch.py (1)
1046-1069: Good warning scope and dedupe placement.Keeping the warned-entity set on the shared
AdaptiveLightingManagermatches the intended “once per HA run” behavior, and the warning stays limited to switch-configured expansion instead of helper-only lookups.Also applies to: 1837-1840
| assert len(warnings) >= 1, ( | ||
| "Expected a WARNING about silent group expansion to surface the " | ||
| "expand_light_groups flag to users. Records: " | ||
| f"{[r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING]}" | ||
| ) | ||
|
|
||
| # Trigger another expansion — warning must NOT repeat for the same entity. | ||
| caplog.clear() | ||
| switch._expand_light_groups() | ||
| await hass.async_block_till_done() | ||
|
|
||
| repeat_warnings = [ | ||
| rec | ||
| for rec in caplog.records | ||
| if rec.levelno == logging.WARNING | ||
| and "expanding light group" in rec.getMessage().lower() | ||
| and "light.light_group" in rec.getMessage() | ||
| ] | ||
| assert not repeat_warnings, ( | ||
| "Warning must fire at most once per entity — repeated expansion " | ||
| f"emitted {len(repeat_warnings)} additional warnings: " | ||
| f"{[r.getMessage() for r in repeat_warnings]}" |
There was a problem hiding this comment.
This “warn once” test never re-enters the warning path.
After setup, switch.lights has already been replaced with member IDs, so the second _expand_light_groups() call stays silent even if expand_light_groups_warned is broken or removed. Tighten the first assertion to exactly one warning, then re-seed the group entity before the second call so the dedupe branch is actually exercised.
Suggested test tightening
- assert len(warnings) >= 1, (
+ assert len(warnings) == 1, (
"Expected a WARNING about silent group expansion to surface the "
"expand_light_groups flag to users. Records: "
f"{[r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING]}"
)
# Trigger another expansion — warning must NOT repeat for the same entity.
caplog.clear()
+ switch.lights = ["light.light_group"]
switch._expand_light_groups()
await hass.async_block_till_done()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert len(warnings) >= 1, ( | |
| "Expected a WARNING about silent group expansion to surface the " | |
| "expand_light_groups flag to users. Records: " | |
| f"{[r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING]}" | |
| ) | |
| # Trigger another expansion — warning must NOT repeat for the same entity. | |
| caplog.clear() | |
| switch._expand_light_groups() | |
| await hass.async_block_till_done() | |
| repeat_warnings = [ | |
| rec | |
| for rec in caplog.records | |
| if rec.levelno == logging.WARNING | |
| and "expanding light group" in rec.getMessage().lower() | |
| and "light.light_group" in rec.getMessage() | |
| ] | |
| assert not repeat_warnings, ( | |
| "Warning must fire at most once per entity — repeated expansion " | |
| f"emitted {len(repeat_warnings)} additional warnings: " | |
| f"{[r.getMessage() for r in repeat_warnings]}" | |
| assert len(warnings) == 1, ( | |
| "Expected a WARNING about silent group expansion to surface the " | |
| "expand_light_groups flag to users. Records: " | |
| f"{[r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING]}" | |
| ) | |
| # Trigger another expansion — warning must NOT repeat for the same entity. | |
| caplog.clear() | |
| switch.lights = ["light.light_group"] | |
| switch._expand_light_groups() | |
| await hass.async_block_till_done() | |
| repeat_warnings = [ | |
| rec | |
| for rec in caplog.records | |
| if rec.levelno == logging.WARNING | |
| and "expanding light group" in rec.getMessage().lower() | |
| and "light.light_group" in rec.getMessage() | |
| ] | |
| assert not repeat_warnings, ( | |
| "Warning must fire at most once per entity — repeated expansion " | |
| f"emitted {len(repeat_warnings)} additional warnings: " | |
| f"{[r.getMessage() for r in repeat_warnings]}" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_switch.py` around lines 3464 - 3485, The test currently never
exercises the "warn once" dedupe branch because switch.lights is mutated during
setup; change the first assertion to require exactly one WARNING (assert
len(warnings) == 1) and before calling switch._expand_light_groups() the second
time, re-seed the group membership by restoring switch.lights to include the
original group member ID (e.g., "light.light_group") so the expansion path runs
again and expand_light_groups_warned logic is exercised; keep using
caplog.clear(), call await hass.async_block_till_done() after the second
expansion, then assert that repeat_warnings (filtered by lowering the message
and matching "expanding light group" and "light.light_group") is empty.
1 participant
Summary
Full testing depth for `expand_light_groups` plus a one-time warning that points
users at the flag when AL is about to silently expand a light group. Motivated by
the curve card work in the Lightener fork — if AL silently bypasses Lightener,
the curves render correctly in the UI but never take effect, and users blame the
wrong codebase.
Tests (9 total)
Initial coverage — PR #12 follow-up
Added in the second pass — silent-failure gaps
Production change — one-time warning
`switch.py: AdaptiveSwitch._expand_light_groups` now emits a `_LOGGER.warning`
(debounced via `manager.expand_light_groups_warned` set) pointing the user at
the `expand_light_groups: false` flag. Fires only from the switch-configured
code path; utility expansions in `_switches_with_lights` stay silent so
light-to-switch matching doesn't spam the log.
Message format:
```
Adaptive Lighting switch 'default' is expanding light group 'light.living_room'
to its members ['light.ceiling', 'light.led_strip', 'light.sofa_lamp']. If this
entity is managed by an integration with its own brightness logic (e.g. Lightener,
Hue rooms, Zigbee2MQTT groups), set 'expand_light_groups: false' on this switch
to preserve that logic.
```
Fires once per (switch, group entity) pair per HA run. Low noise, high signal —
the exact breadcrumb someone needs to find the fix without a multi-codebase
debug session.
Why this matters especially for Lightener
Lightener's `LightenerLight` inherits from HA's `LightGroup`, so it exposes
`entity_id` in its state attributes — which makes AL's `_is_light_group()`
return True. With the default `expand_light_groups=True`, AL fans out per-member
`light.turn_on` calls and Lightener's brightness curve logic is bypassed entirely.
The curve card renders curves correctly, the user adjusts them, the lights ignore
everything. Classic silent failure.
Post-merge, Lightener users have two good paths: set `expand_light_groups: false`
on the AL switch (discovered via the new warning), or document the flag prominently
in the Lightener README.
Test plan
Generated with Claude Code.