[msbuild] Support devicectl JSON version 5. Fixes #26482. - #26643
rolfbjarne wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The parser update and accurate v5 fixture coverage are needed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds regression coverage for devicectl JSON version 5 in GetAvailableDevices.
Changes:
- Adds a JSON v5 fixture and parsing test.
- Exercises nested device properties.
- Requires the parser update, corrected v5 field shapes, and
OSVersion/UDIDassertions.
File summaries
| File | Summary |
|---|---|
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/GetAvailableDevicesTest.cs |
Adds JSON v5 coverage, but the parser update is missing and the fixture and metadata assertions need correction. |
Review details
Suppressed comments (2)
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/GetAvailableDevicesTest.cs:781
- ❌ bug — The fixture does not reflect the actual
jsonVersion: 5shape fromdevicectl list devices: the new dictionary puts the name underproperties.state, pairing underproperties.connection, and the OS version underproperties.software.osVersionNumber(an object withstringValue). Usingproperties.deviceandstate.osVersionNumberhere can let the regression test pass while the parser still fails on Xcode 27 output; base this fixture on a captured v5 payload and cover the actual value shapes.
"device": {
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/GetAvailableDevicesTest.cs:120
⚠️ Test coverage — This regression test does not assert theOSVersionorUDIDmetadata, so it could pass even if the new parser reads enough data forDescriptionandItemSpecbut fails to populate metadata consumed by MSBuild callers. Add explicit assertions forOSVersion(27.0) andUDID(as the legacyDeviceCtl1test does).
Assert.That (task.Devices [0].GetMetadata ("Description"), Is.EqualTo ("Rolf's iPhone 15 - iOS 27.0"), "Device description mismatch.");
Assert.That (task.Devices [0].GetMetadata ("RuntimeIdentifier"), Is.EqualTo ("ios-arm64"), "Device RuntimeIdentifier mismatch.");
Assert.That (task.Devices [0].GetMetadata ("Status"), Is.EqualTo ("Paired"), "Device Status mismatch.");
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
🔵 Needs a closer look
The parser update and simulator filtering are missing, and test coverage needs additional assertions.
Review details
Suppressed comments (3)
mk/xamarin.mk:3
- ❌ bug —
MLAUNCH_NUGET_VERSIONcontrols the unrelated mlaunch NuGet download;GetAvailableDevicesreferencesDeviceCtlOutputParserfrom theexternal/Xamarin.MacDevproject (msbuild/Xamarin.MacDev.Tasks/Xamarin.MacDev.Tasks.csproj:40). The JSON v5 parser is in the macios-devtools submodule, so without updating that gitlink the task still uses the legacy parser, the new fixture is discarded, and the production bug remains. Please update the Xamarin.MacDev submodule to the parser commit (and its version-check state) instead of only changing the mlaunch version.
MLAUNCH_NUGET_VERSION=1.1.153
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/GetAvailableDevicesTest.cs:119
⚠️ Testing — This verifies the version-5 OS value only indirectly throughDescription.GetAvailableDevicesalso publishes theOSVersionmetadata, so add a direct assertion for"27.0"here to catch a regression where the description remains correct but the task output metadata is not.
Assert.That (task.Devices [0].GetMetadata ("Description"), Is.EqualTo ("Rolf's iPhone 15 - iOS 27.0"), "Device description mismatch.");
Assert.That (task.Devices [0].GetMetadata ("RuntimeIdentifier"), Is.EqualTo ("ios-arm64"), "Device RuntimeIdentifier mismatch.");
tests/msbuild/Xamarin.MacDev.Tasks.Tests/TaskTests/GetAvailableDevicesTest.cs:113
- ❌ bug — Xcode 27
devicectl list devicescan include simulator entries markedvisibilityClass: "simulators", while this task also queriessimctlseparately. The updated parser does not preserve/filtervisibilityClass, so those simulators will be returned a second time (without the simulator state metadata). Please filter simulator entries in the parser/task and add a v5 fixture covering this case.
public void DeviceCtlJsonVersion5 ()
{
var platform = ApplePlatform.iOS;
var task = CreateTask (platform, "", DEVICECTL_JSON_VERSION_5);
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
✅ API diff for current PR / commitNET (empty diffs)✅ API diff vs stableNET (empty diffs)ℹ️ Generator diffGenerator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes) Pipeline on Agent |
🚀 [CI Build #9061b97] Test results 🚀Test results✅ All tests passed on VSTS: test results. 🎉 All 264 tests passed 🎉 Tests counts✅ assembly-processing: All 1 tests passed. Html Report (VSDrops) Download macOS tests✅ Tests on macOS Sonoma (14): All 5 tests passed. Html Report (VSDrops) Download Linux Build VerificationPipeline on Agent |
Update Xamarin.MacDev and verify GetAvailableDevices consumes the new properties dictionary.
Fixes #26482.