Re: [PATCH v5 2/6] robocopy/tests: Add basic syntax tests

[Hugh McMaster]

Hi Florian,

On Tue, 21 Sept 2021 at 04:52, Florian Eder wrote:
[snip]
> -
>
> +static DWORD execute_robocopy(const WCHAR *args)
> +{
> - STARTUPINFOW startup_info;
> - PROCESS_INFORMATION process_info;
> - DWORD exit_code;
> - WCHAR *cmd;
> -
> - memset(&startup_info, 0, sizeof(STARTUPINFOW));
> - startup_info.dwFlags = STARTF_USESTDHANDLES;
> - startup_info.hStdInput = INVALID_HANDLE_VALUE;
> - startup_info.hStdOutput = INVALID_HANDLE_VALUE;
> - startup_info.hStdError = INVALID_HANDLE_VALUE;
> -
> - cmd = calloc(ARRAY_SIZE(L"robocopy.exe ") + wcslen(args), sizeof(WCHAR));
> - wcscpy(cmd, L"robocopy.exe ");
> - wcscat(cmd, args);

Dynamically allocating memory is a little unnecessary, although I do
like your thinking. Why don't you start with an array of 256 WCHARs?
You can always increase the size if needed later.
I quite like copying "robocopy.exe", so you could keep that.
Alternatively, rename the function to something like run_test, run_exe
or run_rc and put "robocopy.exe" back in the cmdline you pass in.
> - if (!CreateProcessW(NULL, cmd, NULL, NULL, TRUE, 0, NULL, NULL, &startup_info, &process_info))
> -
>    return MAXDWORD;
> -
> - if (WaitForSingleObject(process_info.hProcess, 30000) == WAIT_TIMEOUT)
> -
>    return MAXDWORD;
> -
> - GetExitCodeProcess(process_info.hProcess, &exit_code);
> -
> - CloseHandle(process_info.hThread);
> - CloseHandle(process_info.hProcess);
> -
> - return exit_code;
>
> +}
> -
>
> +START_TEST(robocopy)
> +{
> - DWORD exit_code;
> - WCHAR temp_path[MAX_PATH];
> -
> - /* robocopy is only available from Vista onwards, abort test if not available */
> - if (execute_robocopy(L"") == MAXDWORD) return;

This still seems overly cumbersome. I strongly suggest
execute_robocopy returns BOOL, and takes a pointer to a DWORD argument
for the exit code.
> - ok(GetTempPathW(ARRAY_SIZE(temp_path), temp_path) != 0, "couldn't get temp folder path");
> -
> - ok(SetCurrentDirectoryW(temp_path), "couldn't set CWD to temp folder "%s"", debugstr_w(temp_path));
> -
> - winetest_push_context("syntax test 1");

Out of interest, do you need a context for every test? Or do you plan
to group tests?
> - RemoveDirectoryW(L"invalid_folder");

Why RemoveDirectoryW()? What's that doing here?
> - exit_code = execute_robocopy(L"");
> - todo_wine ok(exit_code == 16, "unexpected exit code %d\n", exit_code);

This goes back to my earlier point. IMHO, I find assigning to
exit_code makes this harder to read than necessary.
> - winetest_pop_context();

Can you add "robocopy.exe" (no flags), "robocopy.exe /?" and
"robocopy.exe -?" as well?
> - winetest_push_context("syntax test 2");
> - exit_code = execute_robocopy(L"invalid_folder");
> - todo_wine ok(exit_code == 16, "unexpected exit code %d\n", exit_code);
> - winetest_pop_context();
> -
> - winetest_push_context("syntax test 3");
> - exit_code = execute_robocopy(L"-flag invalid_folder");
> - todo_wine ok(exit_code == 16, "unexpected exit code %d\n", exit_code);
> - winetest_pop_context();
> -
> - winetest_push_context("syntax test 4");
> - exit_code = execute_robocopy(L"invalid_folder robocopy_destination");
> - todo_wine ok(exit_code == 16, "unexpected exit code %d\n", exit_code);
> - winetest_pop_context();
> +}
>
> 2.32.0