HWW Validate String Path Platform specific fixes #12781
Conversation
|
Tests are failing, this PR is also unreviewable |
| /// <returns><c>true</c> if the path matches the model's regex, <c>false</c> otherwise.</returns> | ||
| public static bool ValidatePathString(HardwareWalletModels model, string path) | ||
| public static bool ValidatePathString(HardwareWalletModels model, string path, OSPlatform? osPlatform = null) |
There was a problem hiding this comment.
I don't think we need this new parameter anymore, it can be reverted. Same for the mocked tests.
Also, you can remove these extra checks || (osPlatform != null && osPlatform == OSPlatform.Windows).
No need for them.
There was a problem hiding this comment.
Mock tests are only compatible with Windows operating system. You can check predefined paths. In that case, osplatform parameter is needed as the mocktest may fail.
It's important to note that at this time we do not have the capability to separate mock tests for multiple platforms.
There was a problem hiding this comment.
We do not write code only for the tests, please remove this parameter,
If the test is working only on Windows, then disable it on other platforms:
if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
return;
}Here you are forcing the tests to act like they were running on Windows which is an extremely bad practice.
There was a problem hiding this comment.
The HwiKatas test is compatible with all platforms. However, the predefined data used in the mock test is specifically designed for Windows.
Initially, I thought that passing Windows as a parameter in the mocktest would display the expected results since it's only a mock test and not real data.
However, if required, I could expand the mock bridge test to a platform-specific approach.
Co-authored-by: adamPetho <[email protected]>
| @@ -562,7 +563,7 @@ public async Task JadeMockTestsAsync(Network network) | |||
| IEnumerable<HwiEnumerateEntry> enumerate = await client.EnumerateAsync(cts.Token); | |||
| HwiEnumerateEntry entry = Assert.Single(enumerate); | |||
| Assert.Equal(HardwareWalletModels.Jade, entry.Model); | |||
| Assert.True(HwiValidationHelper.ValidatePathString(entry.Model, "COM3")); | |||
| Assert.True(HwiValidationHelper.ValidatePathString(entry.Model, "COM3", OSPlatform.Windows)); | |||
related: #12713.
While the main idea is commendable, HWI has assigned different device paths to different platforms. At present, ValidatePathString only functions on Windows, except for Trezor, which has the same device path on all platforms.
This pull request has undergone testing on all platforms with a range of branded HWWs.