Allow overriding OneWire sysfs paths for testing#2478
Allow overriding OneWire sysfs paths for testing#2478florianraffl wants to merge 1 commit intodotnet:mainfrom
Conversation
Changed SysfsBusDevicesPath and SysfsDevicesPath from const to static string fields to enable runtime modification. Added OverwriteSysfsBusDevicesPath and OverwriteSysfsDevicesPath methods to allow setting custom sysfs directories, with validation. This facilitates testing and use in non-standard environments.
dotnet-policy-service
bot
added
the
area-device-bindings
|
@dotnet-policy-service agree |
|
@copilot review the pull-request according to the repo policies |
There was a problem hiding this comment.
Pull request overview
This pull request enables testing of OneWire device bindings by making sysfs paths configurable at runtime. The changes address issue #2477, which requested the ability to override hardcoded sysfs paths for integration testing scenarios.
Changes:
- Converted SysfsBusDevicesPath and SysfsDevicesPath from const to static string fields to allow runtime modification
- Added two public static methods (OverwriteSysfsBusDevicesPath and OverwriteSysfsDevicesPath) with directory validation to set custom paths for testing
| { | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); |
There was a problem hiding this comment.
The parameter name in the exception message should use nameof(path) instead of a string literal for consistency with the repository's error handling patterns. This ensures that parameter names stay synchronized if they are ever renamed during refactoring.
| public static void OverwriteSysfsBusDevicesPath(string path) | ||
| { | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); | ||
| } | ||
|
|
||
| SysfsBusDevicesPath = path; | ||
| } |
There was a problem hiding this comment.
Missing null validation for the path parameter. The repository consistently uses ArgumentNullException for null parameters (see examples in Ads1115, Adxl345, etc.). Add a null check before the Directory.Exists check.
| public static void OverwriteSysfsDevicesPath(string path) | ||
| { | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); | ||
| } | ||
|
|
||
| SysfsDevicesPath = path; | ||
| } |
There was a problem hiding this comment.
Missing null validation for the path parameter. The repository consistently uses ArgumentNullException for null parameters (see examples in Ads1115, Adxl345, etc.). Add a null check before the Directory.Exists check.
| @@ -71,5 +71,35 @@ internal static void ScanForDeviceChangesInternal(OneWireBus bus, int numDevices | |||
|
|
|||
| File.WriteAllText(Path.Combine(SysfsDevicesPath, bus.BusId, "w1_master_search"), numScans.ToString()); | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Sets the bus path to the specified directory for testing purpose. | |||
| /// </summary> | |||
| /// <param name="path">The path to the directory that will be set as the bus path. This path must exist and be a valid directory.</param> | |||
| /// <exception cref="ArgumentException">Thrown if the specified path does not exist or is not a directory.</exception> | |||
| public static void OverwriteSysfsBusDevicesPath(string path) | |||
| { | |||
| if (!Directory.Exists(path)) | |||
| { | |||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); | |||
| } | |||
|
|
|||
| SysfsBusDevicesPath = path; | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Sets the devices path to the specified directory for testing purpose. | |||
| /// </summary> | |||
| /// <param name="path">The path to the directory that will be set as the devices path. This path must exist and be a valid directory.</param> | |||
| /// <exception cref="ArgumentException">Thrown if the provided path does not exist or is not a directory.</exception> | |||
| public static void OverwriteSysfsDevicesPath(string path) | |||
| { | |||
| if (!Directory.Exists(path)) | |||
| { | |||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); | |||
| } | |||
|
|
|||
| SysfsDevicesPath = path; | |||
| } | |||
There was a problem hiding this comment.
The static mutable fields SysfsBusDevicesPath and SysfsDevicesPath present thread safety concerns. Multiple threads could read these paths while they are being modified, leading to race conditions. Additionally, there is no mechanism to restore the paths to their original values after testing. Consider: (1) using volatile or Interlocked operations for thread-safe updates, (2) providing Reset methods to restore defaults, or (3) documenting that these methods should only be called before any OneWire operations in a single-threaded test setup context.
| /// <summary> | ||
| /// Sets the bus path to the specified directory for testing purpose. | ||
| /// </summary> | ||
| /// <param name="path">The path to the directory that will be set as the bus path. This path must exist and be a valid directory.</param> | ||
| /// <exception cref="ArgumentException">Thrown if the specified path does not exist or is not a directory.</exception> | ||
| public static void OverwriteSysfsBusDevicesPath(string path) | ||
| { | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); | ||
| } | ||
|
|
||
| SysfsBusDevicesPath = path; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Sets the devices path to the specified directory for testing purpose. | ||
| /// </summary> | ||
| /// <param name="path">The path to the directory that will be set as the devices path. This path must exist and be a valid directory.</param> | ||
| /// <exception cref="ArgumentException">Thrown if the provided path does not exist or is not a directory.</exception> | ||
| public static void OverwriteSysfsDevicesPath(string path) | ||
| { | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); | ||
| } | ||
|
|
||
| SysfsDevicesPath = path; | ||
| } |
There was a problem hiding this comment.
Consider adding methods to reset paths to their default values for proper test isolation. Tests that modify these static paths should be able to restore the original state to avoid test interdependencies. For example, add ResetSysfsBusDevicesPath() and ResetSysfsDevicesPath() methods that restore the default "/sys/bus/w1/devices" and "/sys/devices" values respectively.
| { | ||
| if (!Directory.Exists(path)) | ||
| { | ||
| throw new ArgumentException($"Provided path {path} does not exist or is not a directory."); |
There was a problem hiding this comment.
The parameter name in the exception message should use nameof(path) instead of a string literal for consistency with the repository's error handling patterns. This ensures that parameter names stay synchronized if they are ever renamed during refactoring.
Fixes #2477
Changed SysfsBusDevicesPath and SysfsDevicesPath from const to static string fields to enable runtime modification. Added OverwriteSysfsBusDevicesPath and OverwriteSysfsDevicesPath methods to allow setting custom sysfs directories, with validation. This facilitates testing and use in non-standard environments.
Microsoft Reviewers: Open in CodeFlow