[PATCH v4 2/6] robocopy/tests: Add stub

[Hugh McMaster]

On Sat, 18 Sept 2021 at 17:48, Florian Eder wrote:
>
[snip]
> diff --git a/programs/robocopy/tests/robocopy.c b/programs/robocopy/tests/robocopy.c
> new file mode 100644
> index 00000000000..2093a990f03
> --- /dev/null
> +++ b/programs/robocopy/tests/robocopy.c
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright 2021 Florian Eder
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + */
> +
> +#include <wine/test.h>
> +#define WIN32_LEAN_AND_MEAN
> +#include <windows.h>
> +#include <wchar.h>

Traditionally, we put Windows headers first, then Wine headers after.

Is there a reason you decided to use WCHARs? You might find using
plain char easier to work as you build your unit tests.

> +static DWORD execute_robocopy(const WCHAR *cmd_line)
> +{
> +    STARTUPINFOW startup_info;
> +    PROCESS_INFORMATION process_info;
> +    DWORD exit_code;
> +    WCHAR cmd_line_copy[2048];

Why so much memory? And you can shorten the variable name to cmd or similar.

> +    memset(&startup_info, 0, sizeof(STARTUPINFOW));
> +    startup_info.dwFlags = STARTF_USESTDHANDLES;

Where are you setting hStdInput, hStdOutput, and hStdError?

> +    /* CreateProcess must not be called with static strings */

We don't need this comment.

> +    wcscpy(cmd_line_copy, cmd_line);
> +
> +    if (!CreateProcessW(NULL, cmd_line_copy, NULL, NULL, TRUE, 0, NULL, NULL, &startup_info, &process_info))
> +        return -1;
> +
> +    if (WaitForSingleObject(process_info.hProcess, 30000) == WAIT_TIMEOUT)
> +        return -1;

Why are you returning a negative integer from a function returning
type DWORD? While this will work, it's better not to do this.

> +    GetExitCodeProcess(process_info.hProcess, &exit_code);
> +
> +    CloseHandle(process_info.hThread);
> +    CloseHandle(process_info.hProcess);
> +
> +    return exit_code;
> +}

You'll probably find it a lot harder to constantly check the exit code
from the caller with if statements. You might change this later, I
haven't looked yet.

> +START_TEST(robocopy)
> +{
> +    WCHAR temp_path[4096];
> +
> +    ok(GetTempPathW(ARRAY_SIZE(temp_path), temp_path) != 0, "couldn't get temp folder path");

I don't see why GetTempPath() and SetCurrentDirectory() are in this
patch, as they're not used.

> +    /* robocopy is only available from Vista onwards, abort test if not available */
> +    if (execute_robocopy(L"robocopy.exe") == -1) return;

This sort of test should be first. Stylistically, naming a function
'execute_robocopy', then adding 'robocopy' as a command is going to be
cumbersome. (And I know, the reg.exe tests are similar... if shorter
in name).

> +    ok(SetCurrentDirectoryW(temp_path), "couldn't set CWD to temp folder \"%S\"", temp_path);
> +    /* TODO: conformance tests here */

Okay. Perhaps you could add some basic syntax tests instead of a comment?
> +}
> --
> 2.32.0
>
>