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