test: pin the default output-device entry as the clear-the-override path - #1930
Merged
Merged
Conversation
The Volume Control picker holds real endpoints only — there is no "System
default" item — so the entry flagged IsDefault carries that meaning instead, and
an empty endpoint id is the pivot in both directions. Reading, an empty id
resolves to that entry; writing, selecting it sends an empty id, which
SetPersistedDefaultAudioEndpoint treats as CLEAR THE OVERRIDE. That makes the
default entry the only way a user can stop routing an app.
Neither direction was pinned by a test. `value.IsDefault ? string.Empty :
value.Id` reads like a needless special case, and simplifying it to `value.Id`
compiles, keeps every pre-existing test green, and silently removes that
capability: the override would stay in Windows forever, re-pinned to whichever
device happened to be default at the time of the pick. Same
invisible-capability-loss class as a command bound by no XAML.
Three tests, four cases. Selecting a device and then putting the app back on the
default must produce ("{hdst}") then (""); an ordinary pick must keep its own
endpoint id; and a route the service cannot resolve — an empty one, or one naming
a device that is gone — must select the default entry while writing nothing back,
because a refresh that echoed its own snapshot would re-assert a route every pass
and, on the failure branch, report an error the user never caused.
The clear test goes headset-then-default rather than selecting the default
directly: the row is constructed with the default already selected, so assigning
it again is not a property change, the write path would never run, and the test
would pass while asserting nothing.
Also corrects the note on GetPersistedDefaultEndpoint. It claimed the UI shows
"System default", which is not what happens — the null becomes an empty id and
the picker preselects the current default device BY NAME, so an app genuinely
routed elsewhere is indistinguishable from one following the default (#1585).
Calls are captured inside the stub rather than asserted with Received(n), for the
reason the neighbouring test already gives: an NSubstitute substitute is not
thread-safe.
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
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.
What does this PR do?
Volume Control's output picker holds real endpoints only — there is no "System default" item — so
the entry flagged
IsDefaultcarries that meaning instead, and an empty endpoint id is the pivot inboth directions:
AudioSessionRowViewModel.SetOutputDeviceFromServiceresolves an empty id to theIsDefaultentry (:238-241).OnSelectedOutputDeviceChangedsendsstring.Emptywhen the picked entryIsDefault(
:209), andSetPersistedDefaultAudioEndpointtreats empty as clear the override(
AudioPolicyConfig.cs:97-100).So the default entry is the only way a user can stop routing an app, and nothing pinned it.
value.IsDefault ? string.Empty : value.Idreads like a needless special case; simplifying it tovalue.Idcompiles, keeps every pre-existing test green, and silently removes that capability — theoverride would stay in Windows forever, re-pinned to whichever device was default at the time of the
pick. Same invisible-capability-loss class as a command bound by no XAML.
Three tests, four cases:
PuttingAnAppBackOnTheDefaultDevice_ClearsTheOverride("{hdst}")then("")RoutingAnAppToANonDefaultDevice_SendsThatDevicesEndpointIdARouteTheServiceCannotResolve_SelectsTheDefaultEntry_AndWritesNothing×2The clear test goes headset-then-default rather than selecting the default directly: the row is
constructed with the default already selected (the route-read is a stub returning empty), so
assigning it again is not a property change, the write path would never run, and the test would pass
while asserting nothing.
Also corrects the note on
GetPersistedDefaultEndpoint. It claimed "the UI showsSystem default",which is not what happens — the null becomes an empty id and the picker preselects the current
default device by name, so an app genuinely routed elsewhere is indistinguishable from one
following the default. That is the live half of #1585 and the comment was pointing away from it.
Related issues
Partially addresses #1658's neighbour #1585 — see the note below. Not closed.
Type of change
test:)Checklist
maindotnet format --verify-no-changespasses on both touched projectsAudioPolicyConfigTests+
ArchitectureTests, 105 greenHow this was verified
Five mutations, each expected test red, file restored byte-for-byte:
value.IsDefault ? string.Empty : value.Id→value.Idstring.Emptynullinstead of the default entry""only{unplugged}onlyMutations 1, 3 and 4 each fire exactly one case, so every branch is individually pinned rather than
covered by one blanket assertion.
What this deliberately does NOT do
AudioPolicyConfigFactory.GetPersistedDefaultEndpointis still a stub returningnull(
AudioPolicyConfig.cs:90-95), so the picker still cannot show an app's actual persisted route —the live half of #1585. Implementing it means adding an undocumented COM vtable slot whose
out-string marshaling is the most build-variant part of the interface, which is exactly why it was
skipped, and it cannot be exercised on a machine that never runs the app. The alternative — showing a
neutral placeholder instead of a device name — changes what the picker means and where the
clear-the-override action lives, which is a product decision rather than a mechanical fix. This PR
pins the contract that any such change must preserve, and #1585 keeps the decision.