[PATCH 02/41] robocopy/tests: add stub

[Zebediah Figura]

On 9/6/21 9:54 AM, Florian Eder wrote:
> Basic files and required changes to configure(.ac) to create a scaffolding
> for the conformance tests for the robocopy utility
> 
> Signed-off-by: Florian Eder <others.meder at gmail.com>
> ---
>   configure                           |  1 +
>   configure.ac                        |  1 +
>   programs/robocopy/tests/Makefile.in |  5 ++
>   programs/robocopy/tests/robocopy.c  | 80 +++++++++++++++++++++++++++++
>   4 files changed, 87 insertions(+)
>   create mode 100644 programs/robocopy/tests/Makefile.in
>   create mode 100644 programs/robocopy/tests/robocopy.c
> 
> diff --git a/configure b/configure
> index 9e2fa9ef009..297b1783575 100755
> --- a/configure
> +++ b/configure
> @@ -21265,6 +21265,7 @@ wine_fn_config_makefile programs/regini enable_regini
>   wine_fn_config_makefile programs/regsvcs enable_regsvcs
>   wine_fn_config_makefile programs/regsvr32 enable_regsvr32
>   wine_fn_config_makefile programs/robocopy enable_robocopy
> +wine_fn_config_makefile programs/robocopy/tests enable_tests
>   wine_fn_config_makefile programs/rpcss enable_rpcss
>   wine_fn_config_makefile programs/rundll.exe16 enable_win16
>   wine_fn_config_makefile programs/rundll32 enable_rundll32
> diff --git a/configure.ac b/configure.ac
> index 88b9426260f..c8aa29b6628 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -3964,6 +3964,7 @@ WINE_CONFIG_MAKEFILE(programs/regini)
>   WINE_CONFIG_MAKEFILE(programs/regsvcs)
>   WINE_CONFIG_MAKEFILE(programs/regsvr32)
>   WINE_CONFIG_MAKEFILE(programs/robocopy)
> +WINE_CONFIG_MAKEFILE(programs/robocopy/tests)
>   WINE_CONFIG_MAKEFILE(programs/rpcss)
>   WINE_CONFIG_MAKEFILE(programs/rundll.exe16,enable_win16)
>   WINE_CONFIG_MAKEFILE(programs/rundll32)
> diff --git a/programs/robocopy/tests/Makefile.in b/programs/robocopy/tests/Makefile.in
> new file mode 100644
> index 00000000000..146246e4b1a
> --- /dev/null
> +++ b/programs/robocopy/tests/Makefile.in
> @@ -0,0 +1,5 @@
> +TESTDLL   = robocopy.exe
> +IMPORTS   = user32

Why does this import user32?

> +
> +C_SRCS = \
> +	robocopy.c
> diff --git a/programs/robocopy/tests/robocopy.c b/programs/robocopy/tests/robocopy.c
> new file mode 100644
> index 00000000000..ca3d8d7da42
> --- /dev/null
> +++ b/programs/robocopy/tests/robocopy.c
> @@ -0,0 +1,80 @@
> +/*
> + * 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>
> +
> +static DWORD execute_robocopy(const WCHAR *command_line, DWORD expected_exit_code)
> +{
> +    STARTUPINFOW startup_information;
> +    PROCESS_INFORMATION process_information;
> +    DWORD return_value;
> +    DWORD process_exit_code;
> +    WCHAR command_line_copy[2048];

This is a bit of a nitpick, but these variable names are more verbose 
than they need to be. E.g. "return_value" is usually just spelled "ret" 
elsewhere. "startup_information" can be "startup_info".

> +
> +    memset(&startup_information, 0, sizeof(STARTUPINFOW));
> +    startup_information.dwFlags = STARTF_USESTDHANDLES;
> +
> +    /* CreateProcess must not be called with static strings */
> +    wcscpy(command_line_copy, command_line);
> +
> +    if (!CreateProcessW(NULL, command_line_copy, NULL, NULL, TRUE, CREATE_NO_WINDOW, NULL, NULL, &startup_information, &process_information))

What's the purpose of CREATE_NO_WINDOW here?

This line is also very long. We don't have a set line length, but I'd 
recommend breaking after 120 characters, if not sooner.

> +    {
> +        ok(expected_exit_code == -1, "process execution of \"%S\" failed with error %d\n", command_line_copy, GetLastError());
> +        return -1;
> +    }
> +
> +    return_value = WaitForSingleObject(process_information.hProcess, 30000);
> +    if (return_value == WAIT_TIMEOUT)
> +    {
> +        TerminateProcess(process_information.hProcess, 1);
> +        ok(FALSE, "process (executing \"%S\") timed out\n", command_line_copy);

ok(FALSE) is something of an antipattern; you generally want 
ok(!return_value) instead.

Personally I also don't think it's worth trying to clean up like this if 
you're going to print a failure message anyway, especially if there's no 
reasonable way this should fail. A test failure is a test failure; it 
needs to be fixed either way.

> +        return -1;
> +    }
> +
> +    GetExitCodeProcess(process_information.hProcess, &process_exit_code);
> +
> +    CloseHandle(process_information.hThread);
> +    CloseHandle(process_information.hProcess);
> +
> +    /* also accept any exit code if expected exit code is -1 */
> +    ok((expected_exit_code == process_exit_code) || (expected_exit_code == -1),
> +       "process exit code was %d, but expected %d\n", process_exit_code, expected_exit_code);

Usually I'd prefer to just move the ok() to the caller.

> +    return process_exit_code;
> +}
> +
> +START_TEST(robocopy)
> +{
> +    WCHAR previous_cwd_path[4096], temp_path[4096];
> +
> +    ok(GetFullPathNameW(L".", ARRAY_SIZE(previous_cwd_path), previous_cwd_path, NULL) != 0, "couldn't get CWD path");
> +    ok(GetTempPathW(ARRAY_SIZE(temp_path), temp_path) != 0, "couldn't get temp folder path");
> +
> +    /* robocopy is only available from Vista onwards, abort test if not available */
> +    if (execute_robocopy(L"robocopy.exe", -1) == -1) return;
> +
> +    /* set CWD to temp folder */

This comment is redundant; I can see that from the next line.

> +    ok(SetCurrentDirectoryW(temp_path), "couldn't set CWD to temp folder \"%S\"", temp_path);
> +
> +    /* TODO: conformance tests here */
> +
> +    /* Reset CWD to previous folder */
> +    ok(SetCurrentDirectoryW(previous_cwd_path), "couldn't set CWD to previous CWD folder \"%S\"", previous_cwd_path);
> +}
> \ No newline at end of file
> 

Whitespace error.