Match name and preferred address case-insensitively in build_log_name#1693
Conversation
mDNS / DNS labels are case-insensitive (RFC 6762 §16, RFC 4343). The previous case-sensitive startswith check meant a supplied name 'myesp' with a resolved 'MYESP.local' produced 'myesp @ MYESP.local' instead of dropping the redundant prefix. Adds the first test coverage for build_log_name, including the mixed- case case that exercises the fix.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1693 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 26 26
Lines 4165 4165
=========================================
Hits 4165 4165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
In practice ESPHome always uses lowercase but this is a minor correctness improvement |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe PR fixes case-insensitive address matching in ChangesCase-insensitive address matching in build_log_name
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
✨ 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. Comment |
There was a problem hiding this comment.
Pull request overview
Updates build_log_name to treat DNS/mDNS labels case-insensitively when deciding whether to include the "name @ " prefix, aligning it with existing case-normalization behavior elsewhere in util.py. Adds initial unit test coverage for build_log_name to lock in expected output shapes across common address/name combinations.
Changes:
- Make
build_log_namecomparenamevspreferred_addresscase-insensitively (including thestartswith(f"{name}.")suppression check). - Add parametrized tests covering
build_log_namebehavior across.local, hostname, IPv4, bracketed IPv6, multiple-address, and mixed-case scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
aioesphomeapi/util.py |
Lowercase-based comparisons for case-insensitive prefix/equality matching in build_log_name. |
tests/test_util.py |
Adds first test coverage for build_log_name via a parametrized test matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ( | ||
| name | ||
| and name != preferred_address | ||
| and not preferred_address.startswith(f"{name}.") | ||
| and name.lower() != preferred_address.lower() | ||
| and not preferred_address.lower().startswith(f"{name.lower()}.") | ||
| ): |
What does this implement/fix?
build_log_namebuilds the log label that identifies a device session("name @ address"). It already suppresses the prefix when the resolved
address is
name.local, but thestartswithcheck is case-sensitive— so a name
myespwith a resolvedMYESP.localcame out asmyesp @ MYESP.localinstead of justMYESP.local.mDNS / DNS labels are case-insensitive (RFC 6762 §16, RFC 4343), and
the rest of
util.pyalready normalises with.lower()(
address_is_local, fixed in #1681). This bringsbuild_log_nameinto line.
Also adds the first test coverage for
build_log_name. The functionwas previously untested; the new parametrized cases pin the canonical
shapes (bare hostname,
.local, IP-only, with/withoutconnected_address,multiple addresses, bracketed IPv6) plus the mixed-case rows that
exercise the fix.
Types of changes
Related issue or feature (if applicable):
Pull request in esphome (if applicable):
Checklist:
tests/folder).Quality Report
Changes: 2 files changed, 42 insertions(+), 3 deletions(-)
Code scan: clean
Tests: failed (FAILED)
Branch hygiene: clean
Generated by Kōan post-mission quality pipeline