fix(tests): commissioning + mdns helper compile against current API#455
Closed
glennswest wants to merge 2 commits into
Closed
fix(tests): commissioning + mdns helper compile against current API#455glennswest wants to merge 2 commits into
glennswest wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors device discovery by replacing the single discriminator parameter with a comprehensive CommissionableFilter in the discover_device function, allowing for more flexible search criteria. It also updates the AstroMdnsResponder API usage in the test suite. Feedback suggests enhancing the diagnostic warning message in discover_device to log the entire filter object instead of only the discriminator field, which provides better context when discovery fails.
|
|
||
| devices.into_iter().next().ok_or_else(|| { | ||
| warn!("No devices found matching discriminator {discriminator}"); | ||
| warn!("No devices found matching discriminator {:?}", filter.discriminator); |
Contributor
There was a problem hiding this comment.
Since the discover_device function now accepts a full CommissionableFilter and passes it to the discovery process, the warning message should reflect the entire filter criteria rather than just the discriminator. This provides better diagnostic information if discovery fails due to other filter fields (like Vendor ID or Product ID) that might have been specified by the caller.
Suggested change
| warn!("No devices found matching discriminator {:?}", filter.discriminator); | |
| warn!("No devices found matching filter {:?}", filter); |
|
PR #455: Size comparison from da2d79e to 75db789 Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
The 'commissioning' integration test fails to build on main with three
errors when the astro-dnssd feature is selected:
1. rs-matter/tests/common/mdns.rs: AstroMdnsResponder constructor
changed from new(matter) -> new() with matter passed to run(matter).
The test helper still calls the old shape.
2. rs-matter/tests/commissioning.rs: the test's local discover_device
helper still takes a u16 discriminator, but its caller now passes
a &CommissionableFilter (matching the zbus/zeroconf variant). The
astro-dnssd variant also internally constructs a filter from just
the discriminator, dropping any other filter fields the caller
specified.
Repro on main (verified):
cargo test -p rs-matter --test commissioning \
--no-default-features \
--features 'std,rustcrypto,log,astro-dnssd' --no-run
error[E0308]: mismatched types ... expected u16, found &CommissionableFilter
error[E0061]: this method takes 1 argument but 0 were supplied (AstroMdnsResponder::run)
Fix
---
- mdns.rs: use the current AstroMdnsResponder::new().run(matter) shape.
- commissioning.rs: unify discover_device(...) across feature variants
to take &CommissionableFilter directly, so the caller's full filter
passes through and both branches have the same signature.
After this patch the test builds clean under astro-dnssd.
glennswest
force-pushed
the
fix/commissioning-test-api-drift
branch
from
75db789 to
f7df677
Compare
|
PR #455: Size comparison from da2d79e to f7df677 Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
|
|
|
PR #455: Size comparison from da2d79e to 86dc302 Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
Contributor
|
Same here - please assess the code review feedback from Gemini, then I'll review. |
Contributor
|
No reply from author, closing. |
glennswest
added a commit
to glennswest/rs-matter
that referenced
this pull request
May 29, 2026
Addresses Gemini review (project-chip#455): the no-devices-found warning printed only filter.discriminator; now logs the entire CommissionableFilter so vendor/ product/other criteria are visible when discovery fails.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
commissioningintegration test currently fails to build onmainwhen theastro-dnssdfeature is selected — three compile errors from API drift between the test helpers and the library.Repro (verified on
main):Fix
rs-matter/tests/common/mdns.rs—AstroMdnsRespondermoved from::new(matter).run()to::new().run(matter). The test helper still used the old shape.rs-matter/tests/commissioning.rs— thediscover_devicehelper kept au16 discriminatorparameter for theastro-dnssdbuild while the caller now passes&CommissionableFilter(matching the zbus/zeroconf build). Also: theastro-dnssdbranch was internally re-constructing a filter from only the discriminator, silently dropping any other filter fields the caller specified.Unified
discover_device(filter: &CommissionableFilter, timeout_ms: u32)across both feature variants — caller now passes the full filter through toastro::discover_commissionable.Test plan
cargo test -p rs-matter --test commissioning --no-default-features --features 'std,rustcrypto,log,astro-dnssd' --no-run— builds clean after the patch (failed cleanly onmainbefore).