Re: [PATCH v6 0/2] MR7060: scrrun/tests: Add drive_get_path test

[Quinn Rafferty (＠ramleejo)]

On Mon Dec 30 17:30:16 2024 +0000, Alex Henrie wrote:
> Thanks for the patches. The implementation looks good and it allows the
> Windows Search Index Analyzer to display its initial dialog. It's
> possible that drive_get_Path should really call GetVolumePathNameW and
> strip the trailing backslash, but Wine's implementation of the Drive
> class currently only supports letter drives, so a simpler implementation
> is fine for now.
> I'm not sure that all the tests are necessary. If it were me, I would
> just do this:
> ```diff
> diff --git a/dlls/scrrun/tests/filesystem.c b/dlls/scrrun/tests/filesystem.c
> index e53bd092ce9..aabec74b3f9 100644
> --- a/dlls/scrrun/tests/filesystem.c
> +++ b/dlls/scrrun/tests/filesystem.c
> @@ -1471,7 +1471,7 @@ static void test_DriveCollection(void)
>      while (IEnumVARIANT_Next(enumvar, 1, &var, &fetched) == S_OK) {
>          IDrive *drive = (IDrive*)V_DISPATCH(&var);
>          DriveTypeConst type;
> -        BSTR str;
> +        BSTR str, path;
>
> hr = IDrive_get_DriveType(drive, &type);
>          ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> @@ -1482,7 +1482,17 @@ static void test_DriveCollection(void)
>          hr = IDrive_get_DriveLetter(drive, &str);
>          ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
>          ok(SysStringLen(str) == 1, "got string %s\n", wine_dbgstr_w(str));
> +
> +        hr = IDrive_get_Path(drive, NULL);
> +        ok(hr == E_POINTER, "Unexpected hr %#lx.\n", hr);
> +
> +        hr = IDrive_get_Path(drive, &path);
> +        ok(hr == S_OK, "Unexpected hr %#lx.\n", hr);
> +        ok(SysStringLen(path) == 2 && path[0] == str[0] && path[1] == ':',
> +           "got string %s\n", wine_dbgstr_w(path));
> +
>          SysFreeString(str);
> +        SysFreeString(path);
>
> hr = IDrive_get_IsReady(drive, NULL);
>          ok(hr == E_POINTER, "Unexpected hr %#lx.\n", hr);
> ```
> However, when submitting tests I am often told that I have added either
> too few tests or too many tests, and I'm not sure what the preferred
> amount of tests is here.
> Thanks for the feedback; I'm happy I could help fix the issue for you. I had thought about adding my test to a previous test like in your example, but I wasn't sure what would be considered best practice so I went with the safe route. I'm open to changing the tests to what you suggested, it's much more concise than my solution.

Also, thanks for adding this to the Bugzilla report. I had meant to but had forgotten to as I was sick all last week.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7060#note_91195