[PATCH v5 2/6] robocopy/tests: Add basic syntax tests
Hugh McMaster
hugh.mcmaster at outlook.com
Tue Sep 21 08:33:54 CDT 2021
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
>
>
More information about the wine-devel
mailing list