Re: [PATCH v6 6/6] robocopy/tests: Add basic conformance tests

[Zebediah Figura (she/her)]

On 9/21/21 09:54, Florian Eder wrote:
> Basic conformance tests create a test source directory with
> files and execute robocopy on it, checking whether the resulting
> destination directory, the remaining source directory and the exit code
> is as expected
> Signed-off-by: Florian Eder
>
> v6: exit_code is now set as a pointer / argument to execute_robocopy, instead
> of being its return value
>
> programs/robocopy/tests/robocopy.c | 188 ++++++++++++++++++++++++++++-
>   1 file changed, 187 insertions(+), 1 deletion(-)
>
> diff --git a/programs/robocopy/tests/robocopy.c b/programs/robocopy/tests/robocopy.c
> index d5d3dee473a..0eb975861bc 100644
> --- a/programs/robocopy/tests/robocopy.c
> +++ b/programs/robocopy/tests/robocopy.c
> @@ -49,10 +49,153 @@ static BOOL execute_robocopy(const WCHAR *args, DWORD *exit_code)
>       return TRUE;
>   }
>
> +static void create_test_file(const WCHAR *relative_path, size_t size, LONGLONG fixed_filetime, long filetime_offset)
> +{
> - HANDLE handle;
> - WCHAR path[1024];
> - wcscpy(path, L"\\?\");
> - GetTempPathW(ARRAY_SIZE(path) - 4, &(path[4]));
> - wcscat(path, relative_path);
> - handle = CreateFileW(path, FILE_GENERIC_WRITE | FILE_GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_ALWAYS, 0, NULL);
> - ok(handle != INVALID_HANDLE_VALUE, "creation of %s failed (0x%08x)\n", debugstr_w(path), GetLastError());
> - if (size != 0)
> - {
> -
>    BYTE *data;
> -
>    DWORD bytes_written;
> -
>    data = calloc(size, sizeof(BYTE));
> -
>    ok(WriteFile(handle, data, size, &bytes_written, NULL), "writing to %s failed (%d)\n", debugstr_w(path), GetLastError());
> - }
> - if (fixed_filetime != 0)
> - {
> -
>    FILETIME time;
> -
>    LARGE_INTEGER time_as_integer;
> -
>    time_as_integer.QuadPart = fixed_filetime;
> -
>    time.dwHighDateTime = time_as_integer.HighPart;
> -
>    time.dwLowDateTime = time_as_integer.LowPart;
> -
>    ok(SetFileTime(handle, &time, &time, &time), "filetime manipulation of %s failed (%d)\n", debugstr_w(path), GetLastError());
> - }
> - if (filetime_offset != 0)
> - {
> -
>    FILETIME time, modified_time, access_time;
> -
>    LARGE_INTEGER time_as_integer;
> -
>    GetFileTime(handle, &time, &modified_time, &access_time);
> -
>    /* FILETIME is no union with LONGLONG / LONG64, casting could be unsafe */
> -
>    time_as_integer.HighPart = time.dwHighDateTime;
> -
>    time_as_integer.LowPart = time.dwLowDateTime;
> -
>    /* 1000 * 1000 * 60 * 60 * 24 = 86400000000ns per day */
> -
>    time_as_integer.QuadPart += 864000000000LL * filetime_offset;
> -
>    time.dwHighDateTime = time_as_integer.HighPart;
> -
>    time.dwLowDateTime = time_as_integer.LowPart;
> -
>    ok(SetFileTime(handle, &time, &time, &time), "filetime manipulation of %s failed (%d)\n", debugstr_w(path), GetLastError());
> - }

You don't actually test the file times yet; this probably deserves to be 
put off to a later patch.
> - CloseHandle(handle);
>
> +}
> -
>
> +static void create_test_folder(const WCHAR *relative_path)
> +{
> - WCHAR path[1024];
> - wcscpy(path, L"\\?\");
> - GetTempPathW(ARRAY_SIZE(path) - 4, &(path[4]));

For tests I think it's reasonable to assume everything fits in MAX_PATH.
> - wcscat(path, relative_path);
> - CreateDirectoryW(path, NULL);
>
> +}
> -
>
> +static void create_test_source_folder(void)
> +{
> - create_test_folder(L"robocopy_source");
> - create_test_folder(L"robocopy_source\folderA");
> - create_test_folder(L"robocopy_source\folderB");
> - create_test_folder(L"robocopy_source\folderC");
> - create_test_folder(L"robocopy_source\folderA\folderD");
> - create_test_folder(L"robocopy_source\folderA\folderE");
> - create_test_file(L"robocopy_source\fileA.a", 4000, 0, -10);
> - create_test_file(L"robocopy_source\fileB.b", 8000, 0, -2);
> - create_test_file(L"robocopy_source\folderA\fileC.c", 60, 0, -2);
> - create_test_file(L"robocopy_source\folderA\fileD.d", 80, 0, 0);
> - create_test_file(L"robocopy_source\folderA\folderD\fileE.e", 10000, 0, -10);
> - create_test_file(L"robocopy_source\folderB\fileF.f", 10000, 132223104000000000, 0);
> - create_test_file(L"robocopy_source\folderB\fileG.g", 200, 132223104000000000, 0);
>
> +}
> -
>
> +static void check_file_and_delete(const WCHAR *relative_path, BOOL should_exist)
> +{
> - WCHAR path[1024];
> - wcscpy(path, L"\\?\");
> - GetTempPathW(ARRAY_SIZE(path) - 4, &(path[4]));
> - wcscat(path, relative_path);
> - if (!DeleteFileW(path))
> - {
> -
>    if (GetLastError() == ERROR_FILE_NOT_FOUND)
> -
>        ok(!should_exist, "file \"%s\" does not exist, but should exist\n", debugstr_w(relative_path));
> -
>    else if (GetLastError() == ERROR_PATH_NOT_FOUND)
> -
>        ok(!should_exist, "file \"%s\" and the parent directory do not exist, but should exist\n", debugstr_w(relative_path));
> -
>    else
> -
>        ok(FALSE, "file \"%s\" DeleteFileW returned error %d\n", debugstr_w(relative_path), GetLastError());
> - }
> - else
> - {
> -
>    ok(should_exist, "file \"%s\" should not exist, but does exist\n", debugstr_w(relative_path));
> - }

This seems structured very awkwardly, and the fact that you have 
ok(FALSE) is good evidence of that. I'd suggest something like the 
following:

ret = DeleteFileW(path);
ok(ret == should_exist, ...);
if (!should_exist)
     ok(GetLastError() == ERROR_FILE_NOT_FOUND || GetLastError() == 
ERROR_PATH_NOT_FOUND, ...);

Similarly for check_folder_and_delete().

I might also get rid of the and_delete from the name.
> +}
> -
>
> +static void check_folder_and_delete(const WCHAR *relative_path, BOOL should_exist)
> +{
> - WCHAR path[1024];
> - wcscpy(path, L"\\?\");
> - GetTempPathW(ARRAY_SIZE(path) - 4, &(path[4]));
> - wcscat(path, relative_path);
> - if (!RemoveDirectoryW(path))
> - {
> -
>    if (GetLastError() == ERROR_FILE_NOT_FOUND)
> -
>        ok(!should_exist, "directory \"%s\" does not exist, but should exist\n", debugstr_w(relative_path));
> -
>    else if (GetLastError() == ERROR_PATH_NOT_FOUND)
> -
>        ok(!should_exist, "directory \"%s\" and the parent directory do not exist, but should exist\n", debugstr_w(relative_path));
> -
>    else if (GetLastError() == ERROR_DIR_NOT_EMPTY)
> -
>        ok(FALSE, "directory \"%s\" is unexpectedly not empty\n", debugstr_w(relative_path));
> -
>    else
> -
>        ok(FALSE, "directory \"%s\" DeleteFileW returned error %d\n", debugstr_w(relative_path), GetLastError());
> - }
> - else
> - {
> -
>    ok(should_exist, "directory \"%s\" should not exist, but does exist\n", debugstr_w(relative_path));
> - }
>
> +}
> -
>
> +static void check_basic_copy_test(void)
> +{
> - check_file_and_delete(L"robocopy_source\fileA.a", TRUE);
> - check_file_and_delete(L"robocopy_source\fileB.b", TRUE);
> - check_file_and_delete(L"robocopy_source\folderA\fileC.c", TRUE);
> - check_file_and_delete(L"robocopy_source\folderA\fileD.d", TRUE);
> - check_file_and_delete(L"robocopy_source\folderA\folderD\fileE.e", TRUE);
> - check_file_and_delete(L"robocopy_source\folderB\fileF.f", TRUE);
> - check_file_and_delete(L"robocopy_source\folderB\fileG.g", TRUE);
> - check_folder_and_delete(L"robocopy_source\folderA\folderD", TRUE);
> - check_folder_and_delete(L"robocopy_source\folderA\folderE", TRUE);
> - check_folder_and_delete(L"robocopy_source\folderA", TRUE);
> - check_folder_and_delete(L"robocopy_source\folderB", TRUE);
> - check_folder_and_delete(L"robocopy_source\folderC", TRUE);
> - check_folder_and_delete(L"robocopy_source", TRUE);
> -
> - check_file_and_delete(L"robocopy_destination\fileA.a", TRUE);
> - check_file_and_delete(L"robocopy_destination\fileB.b", TRUE);
> - todo_wine check_file_and_delete(L"robocopy_destination\folderA\fileC.c", FALSE);
> - todo_wine check_file_and_delete(L"robocopy_destination\folderA\fileD.d", FALSE);
> - todo_wine check_file_and_delete(L"robocopy_destination\folderA\folderD\fileE.e", FALSE);
> - todo_wine check_file_and_delete(L"robocopy_destination\folderB\fileF.f", FALSE);
> - todo_wine check_file_and_delete(L"robocopy_destination\folderB\fileG.g", FALSE);
> - todo_wine check_folder_and_delete(L"robocopy_destination\folderA\folderD", FALSE);
> - check_folder_and_delete(L"robocopy_destination\folderA\folderE", FALSE);
> - todo_wine check_folder_and_delete(L"robocopy_destination\folderA", FALSE);
> - todo_wine check_folder_and_delete(L"robocopy_destination\folderB", FALSE);
> - check_folder_and_delete(L"robocopy_destination\folderC", FALSE);
> - check_folder_and_delete(L"robocopy_destination", TRUE);
>
> +}
> - START_TEST(robocopy)
> {
>     DWORD exit_code;
> - WCHAR temp_path[MAX_PATH];
> -
>
> WCHAR temp_cmd[512], temp_path[MAX_PATH];
>
> /* robocopy is only available from Vista onwards, abort test if not available */
>  if (!execute_robocopy(L"", &exit_code)) return;
>
> @@ -100,4 +243,47 @@ START_TEST(robocopy)
>       execute_robocopy(L"invalid_folder /?", &exit_code);
>       ok(exit_code == 16, "unexpected exit code %d\n", exit_code);
>       winetest_pop_context();
> -
> - winetest_push_context("basic copy test 1");
> - create_test_source_folder();
> - execute_robocopy(L"robocopy_source robocopy_destination /r:1 /w:0", &exit_code);

You might consider shortening things by changing the working directory 
to a generated subdir of %temp% instead of %temp% itself, and then you 
don't have to worry about prepending "robocopy" to everything.
> - ok(exit_code == 1, "unexpected exit code %d\n", exit_code);
> - check_basic_copy_test();
> - winetest_pop_context();
> -
> - winetest_push_context("basic copy test 2");
> - create_test_source_folder();
> - execute_robocopy(L"./robocopy_source third_folder/../robocopy_destination /r:1 /w:0", &exit_code);
> - ok(exit_code == 1, "unexpected exit code %d\n", exit_code);
> - check_basic_copy_test();
> - winetest_pop_context();
> -
> - winetest_push_context("basic copy test 3");
> - create_test_source_folder();
> - create_test_folder(L"robocopy_destination");
> - create_test_file(L"robocopy_destination\fileA.a", 9000, 0, -10);
> - execute_robocopy(L"./robocopy_source robocopy_source/../robocopy_destination /r:1 /w:0", &exit_code);
> - ok(exit_code == 1, "unexpected exit code %d\n", exit_code);
> - check_basic_copy_test();
> - winetest_pop_context();
> -
> - winetest_push_context("basic copy test 4");
> - create_test_source_folder();
> - swprintf(temp_cmd, ARRAY_SIZE(temp_cmd),
> -
>            L"%s\\robocopy_source %s\\robocopy_destination /r:1 /w:0",
> -
>            temp_path, temp_path);
> - execute_robocopy(temp_cmd, &exit_code);
> - ok(exit_code == 1, "unexpected exit code %d\n", exit_code);
> - check_basic_copy_test();
> - winetest_pop_context();
> -
> - winetest_push_context("basic copy test 5");
> - create_test_source_folder();
> - swprintf(temp_cmd, ARRAY_SIZE(temp_cmd),
> -
>            L"%s\\third_folder\\..\\robocopy_source %s\\third_folder\\..\\robocopy_destination /r:1 /w:0",
> -
>            temp_path, temp_path);
> - execute_robocopy(temp_cmd, &exit_code);
> - ok(exit_code == 1, "unexpected exit code %d\n", exit_code);
> - check_basic_copy_test();
> - winetest_pop_context();
> }

There's a lot of behaviour covered in patch 5 that isn't covered here. 
For example:

* There are no tests for path parameters (i.e. after the source and 
dest). When adding such tests, I would recommend making sure you test 
the following:

- specifying the same path more than once;

- specifying an absolute path;

- specifying a path that doesn't exist in the source;

- specifying both valid and invalid paths, in both orders, to see 
whether partial copies are done;

- specifying wildcard paths [also, wildcard paths followed by a 
backslash, like a/*/b? I'm assuming this is why we need to use 
PathMatchSpec()...]

* What happens if the source folder itself doesn't exist?

* Testing what happens if the source and destination are the same seems 
helpful.

I believe you have most of these tests, somewhere, but ideally they 
should all be before patch 5/6 in this series :-)