Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions Daqifi.Desktop.Test/Device/DisplayIdentifierTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,33 @@ public void SerialStreamingDevice_ConnectionType_ShouldBeUsb()
{
// Arrange
var device = new SerialStreamingDevice("COM1");

// Act & Assert
Assert.AreEqual(ConnectionType.Usb, device.ConnectionType,
"SerialStreamingDevice should have USB connection type");
}


[TestMethod]
public void SerialStreamingDevice_Connect_WithMissingPort_ReturnsFalseWithoutThrowing()
{
// Regression for issue #524: SerialPort.Open() throws FileNotFoundException on Windows
// when a syntactically-valid but absent COM port (e.g. "COM250") is opened. Connect()
// must catch and return false rather than letting the exception escape — previously
// this surfaced as a Sentry "error" for what is a user/environmental condition.
// "COM250" is intentionally chosen so it parses as a real COM port name (so we hit
// the FileNotFoundException path on Windows) but is unlikely to be present on a CI
// runner. On non-Windows hosts SerialPort.Open() throws PlatformNotSupportedException
// or similar; the generic Exception catch still suffices to satisfy this assertion.
var device = new SerialStreamingDevice("COM250");

var result = device.Connect();

Assert.IsFalse(result,
"Connect() against a nonexistent port should report failure, not throw.");
Assert.IsFalse(device.IsConnected,
"Device should not be marked connected when the underlying port could not be opened.");
Comment on lines +93 to +100
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Unit tests use real serialport 📘 Rule violation ▣ Testability

New tests execute code paths that call SerialPort.Open() / SerialPort.GetPortNames(), which
depend on the host environment and can be flaky on CI or non-Windows runners. This violates the
requirement to mock external dependencies in unit tests.
Agent Prompt
## Issue description
New unit tests depend on real serial-port APIs (OS/hardware/environment). This can introduce flakiness and violates the requirement to mock external dependencies.

## Issue Context
- `SerialStreamingDevice_Connect_WithMissingPort_ReturnsFalseWithoutThrowing()` calls `SerialStreamingDevice.Connect()` which will ultimately attempt to open a real port.
- `ConnectManualSerialCommand_WithNonexistentPort_SetsManualPortError()` exercises `SerialPort.GetPortNames()` via the ViewModel validation.

## Fix Focus Areas
- Daqifi.Desktop.Test/Device/DisplayIdentifierTests.cs[93-101]
- Daqifi.Desktop.Test/ViewModels/ConnectionDialogViewModelCloseTests.cs[77-92]
- Daqifi.Desktop/ViewModels/ConnectionDialogViewModel.cs[273-288]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}

#endregion

#region DaqifiStreamingDevice (WiFi) Tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ namespace Daqifi.Desktop.Test.ViewModels;
[TestClass]
public class ConnectionDialogViewModelCloseTests
{
// A name guaranteed to be absent from SerialPort.GetPortNames() on every CI runner —
// Windows uses COM1..COMn, macOS/Linux use /dev/tty.* paths.
private const string NonexistentPortName = "COM_DOES_NOT_EXIST_524";

private Func<DuplicateDeviceCheckResult, DuplicateDeviceAction>? _originalDuplicateDeviceHandler;

[TestInitialize]
Expand Down Expand Up @@ -66,6 +70,37 @@ public async Task ConnectManualSerialCommand_WithBlankPort_DoesNotRaiseCloseRequ
await viewModel.ConnectManualSerialCommand.ExecuteAsync(null);

Assert.IsFalse(closeRaised(), "CloseRequested should not fire when the manual port is blank.");
Assert.IsNull(viewModel.ManualPortError,
"Blank input should not surface the missing-port error message.");
}

[TestMethod]
public async Task ConnectManualSerialCommand_WithNonexistentPort_SetsManualPortError()
{
var viewModel = CreateViewModel();
viewModel.ManualPortName = NonexistentPortName;
var closeRaised = SubscribeToClose(viewModel);

await viewModel.ConnectManualSerialCommand.ExecuteAsync(null);

Assert.IsFalse(closeRaised(),
"CloseRequested should not fire when the entered port is not present on the system.");
Assert.IsNotNull(viewModel.ManualPortError,
"ManualPortError should be set when the entered port is not present on the system.");
StringAssert.Contains(viewModel.ManualPortError, NonexistentPortName,
"Error message should mention the offending port name.");
}

[TestMethod]
public void ManualPortError_ClearsWhenManualPortNameChanges()
{
var viewModel = CreateViewModel();
viewModel.ManualPortError = "stale validation message";

viewModel.ManualPortName = "COM1";

Assert.IsNull(viewModel.ManualPortError,
"Editing the manual port name should clear any prior validation error.");
}

[TestMethod]
Expand Down
1 change: 1 addition & 0 deletions Daqifi.Desktop/App.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
<converters:ListToStringConverter x:Key="ListToStringConverter"/>
<converters:BrushColorMatchConverter x:Key="BrushColorMatch"/>
<converters:OxyColorToBrushConverter x:Key="OxyColorToBrushConverter"/>
<converters:NotNullToVisibilityConverter x:Key="NotNullToVis"/>
</ResourceDictionary>
</Application.Resources>
</Application>
21 changes: 20 additions & 1 deletion Daqifi.Desktop/Device/SerialDevice/SerialStreamingDevice.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.IO.Ports;
using System.IO;
using System.IO.Ports;
using Daqifi.Core.Device;
using Daqifi.Core.Communication.Transport;
using Daqifi.Core.Communication.Messages;
Expand Down Expand Up @@ -112,6 +113,24 @@ public override bool Connect()
WaitForInitialStatusMessage();
return true;
}
catch (FileNotFoundException ex)
{
// .NET's SerialPort.Open throws FileNotFoundException when the COM port no
// longer exists (device unplugged, never present, or renamed). Treat as a
// user/environmental condition, not an app bug — log a warning instead of
// capturing to Sentry.
AppLogger.Warning($"Cannot connect on {PortName}: port is not available ({ex.Message})");
CleanupConnection();
return false;
}
catch (UnauthorizedAccessException ex)
{
// SerialPort.Open throws UnauthorizedAccessException when another process
// already holds the port open. Same classification as above.
AppLogger.Warning($"Cannot connect on {PortName}: port is in use by another process ({ex.Message})");
CleanupConnection();
return false;
}
catch (Exception ex)
{
AppLogger.Error(ex, $"Failed to connect on {PortName}");
Expand Down
4 changes: 4 additions & 0 deletions Daqifi.Desktop/View/ConnectionDialog.xaml
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@
<TextBlock Text="Enter the COM port name of the DAQiFi device (e.g. COM3)."
FontSize="10" Foreground="{StaticResource TextTertiary}"
Margin="0,6,0,0" TextWrapping="Wrap"/>
<TextBlock Text="{Binding ManualPortError}"
FontSize="11" Foreground="{StaticResource StatusRed}"
Margin="0,8,0,0" TextWrapping="Wrap"
Visibility="{Binding ManualPortError, Converter={StaticResource NotNullToVis}}"/>
</StackPanel>

<Border Grid.Row="1"
Expand Down
68 changes: 66 additions & 2 deletions Daqifi.Desktop/ViewModels/ConnectionDialogViewModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Daqifi.Desktop.View;
using System.Collections;
using System.Collections.ObjectModel;
using System.IO.Ports;
using System.Net;
using System.Net.Sockets;
using CommunityToolkit.Mvvm.ComponentModel;
Expand Down Expand Up @@ -55,13 +56,31 @@ public partial class ConnectionDialogViewModel : ObservableObject
public ObservableCollection<SerialStreamingDevice> AvailableSerialDevices { get; } = [];
public ObservableCollection<CoreDeviceInfo> AvailableHidDevices { get; } = [];

public string ManualPortName { get; set; }
[ObservableProperty]
private string? _manualPortName;

/// <summary>
/// User-facing validation message for the Manual USB tab. Non-null when the entered
/// COM port failed pre-flight validation (e.g. port not present on the system).
/// Cleared automatically when the user edits <see cref="ManualPortName"/>.
/// </summary>
[ObservableProperty]
private string? _manualPortError;

public SerialStreamingDevice ManualSerialDevice { get; set; }

public string ManualIpAddress { get; set; }
#endregion

partial void OnManualPortNameChanged(string? value)
{
// Clear stale validation error as soon as the user starts editing the port name.
if (!string.IsNullOrEmpty(ManualPortError))
{
ManualPortError = null;
}
}

#region Constructor
public ConnectionDialogViewModel() : this(ServiceLocator.Resolve<IDialogService>()) { }

Expand Down Expand Up @@ -215,14 +234,59 @@ private async Task ConnectSerialAsync(object? selectedItems)

private async Task ConnectManualSerialAsync()
{
ManualPortError = null;

if (string.IsNullOrWhiteSpace(ManualPortName)) { return; }

ManualSerialDevice = new SerialStreamingDevice(ManualPortName);
var portName = ManualPortName.Trim();

if (!IsPortAvailable(portName))
{
// Avoid the FileNotFoundException round-trip from SerialPort.Open by
// pre-checking against the system's enumerated ports. Surface a friendly
// message in the dialog instead of silently closing.
ManualPortError =
$"Port '{portName}' is not available. Plug in the device or check Device Manager for the correct port name.";
Common.Loggers.AppLogger.Instance.Warning(
$"Manual serial connect rejected: port '{portName}' is not present on the system.");
return;
}

ManualSerialDevice = new SerialStreamingDevice(portName);
await ConnectionManager.Instance.Connect(ManualSerialDevice);

// Post-connect status check covers failures the pre-flight enumeration cannot
// catch — most notably "port exists but is held by another process", which
// SerialStreamingDevice.Connect now classifies as a Warning (no Sentry capture).
// Without this, the dialog would close silently and the user would have no idea
// the connection failed.
if (ConnectionManager.Instance.ConnectionStatus == DAQiFiConnectionStatus.Error)
{
ManualPortError =
$"Could not connect to '{portName}'. The port may be in use by another application or the device is not responding.";
return;
}

RaiseCloseRequested();
}

private static bool IsPortAvailable(string portName)
{
try
{
return SerialPort.GetPortNames()
.Any(p => string.Equals(p, portName, StringComparison.OrdinalIgnoreCase));
}
catch (Exception ex)
{
// If port enumeration itself fails, fall back to the connect attempt rather
// than blocking the user — Connect() will surface its own error path.
Common.Loggers.AppLogger.Instance.Warning(
$"Failed to enumerate serial ports during manual-connect validation: {ex.Message}");
return true;
}
}

private async Task ConnectManualWifiAsync()
{
if (string.IsNullOrWhiteSpace(ManualIpAddress)) { return; }
Expand Down
Loading