Re: [PATCH v7 2/4] robocopy/tests: Add basic conformance tests

[Zebediah Figura]

On 10/16/21 6:13 AM, Florian Eder wrote:
> +static void check_file(const WCHAR *relative_path, BOOL should_exist)
> +{
> - WCHAR path[MAX_PATH];
> - swprintf(path, ARRAY_SIZE(path), L"%s%s", temp_path, relative_path);
> -
> - ok (DeleteFileW(path) == should_exist, "file %s expected exist to be %d, but is %d\n", debugstr_w(relative_path), should_exist, !should_exist);

Do we need to pass an absolute path to DeleteFile() and 
RemoveDirectory(), or can we just pass the relative path?
> - if (!should_exist)
> -
>    ok(GetLastError() == ERROR_FILE_NOT_FOUND || GetLastError() == ERROR_PATH_NOT_FOUND,
> -
>       "file %s DeleteFileW returned error %d, should not exist\n",
> -
>       debugstr_w(relative_path),
> -
>       GetLastError());
>
> +}
> -
>
> +static void check_folder(const WCHAR *relative_path, BOOL should_exist)
> +{
> - WCHAR path[MAX_PATH];
> - swprintf(path, ARRAY_SIZE(path), L"%s%s", temp_path, relative_path);
> -
> - ok (RemoveDirectoryW(path) == should_exist, "folder %s expected exist to be %d, but is %d\n", debugstr_w(relative_path), should_exist, !should_exist);
> - if (!should_exist)
> -
>    ok(GetLastError() == ERROR_FILE_NOT_FOUND || GetLastError() == ERROR_PATH_NOT_FOUND,
> -
>       "folder %s RemoveDirectoryW returned error %d, should not exist\n",
> -
>       debugstr_w(relative_path),
> -
>       GetLastError());
>
> +}
> -
>
> +static void check_basic_copy_1_test(void)
> +{
> - check_file(L"source\fileA.a", TRUE);
> - check_file(L"source\fileB.b", TRUE);
> - check_file(L"source\folderA\fileC.c", TRUE);
> - check_file(L"source\folderA\fileD.d", TRUE);
> - check_file(L"source\folderA\folderD\fileE.e", TRUE);
> - check_file(L"source\folderB\fileF.f", TRUE);
> - check_file(L"source\folderB\fileG.g", TRUE);
> - check_folder(L"source\folderA\folderD", TRUE);
> - check_folder(L"source\folderA\folderE", TRUE);
> - check_folder(L"source\folderA", TRUE);
> - check_folder(L"source\folderB", TRUE);
> - check_folder(L"source\folderC", TRUE);
> - check_folder(L"source", TRUE);

All of these so far are copy operations, so this block ends up getting 
duplicated. Could that use a helper?
> -
> - todo_wine check_file(L"destination\fileA.a", TRUE);
> - todo_wine check_file(L"destination\fileB.b", TRUE);
> - check_file(L"destination\folderA\fileC.c", FALSE);
> - check_file(L"destination\folderA\fileD.d", FALSE);
> - check_file(L"destination\folderA\folderD\fileE.e", FALSE);
> - check_file(L"destination\folderB\fileF.f", FALSE);
> - check_file(L"destination\folderB\fileG.g", FALSE);
> - check_folder(L"destination\folderA\folderD", FALSE);
> - check_folder(L"destination\folderA\folderE", FALSE);
> - check_folder(L"destination\folderA", FALSE);
> - check_folder(L"destination\folderB", FALSE);
> - check_folder(L"destination\folderC", FALSE);
> - todo_wine check_folder(L"destination", TRUE);
>
> +}
> -

...
> +START_TEST(robocopy)
> +{
> - DWORD exit_code;
> - WCHAR temp_cmd[512];
> - int i;
> - const WCHAR *invalid_syntax_tests[] =

This should be static const as well (i.e. "static const WCHAR *const").
> - {
> -
>    L"robocopy",
> -
>    L"robocopy invalid_folder",
> -
>    L"robocopy -flag invalid_folder",
> -
>    L"robocopy invalid_folder destination",
> -
>    L"robocopy -?",
> -
>    L"robocopy invalid_folder -?",
> -
>    L"robocopy invalid_folder /?",
> - };
> -
> - /* robocopy is only available from Vista onwards, abort test if not available */
> - if (!run_cmd(L"robocopy", &exit_code)) return;
> -
> - ok(GetTempPathW(ARRAY_SIZE(temp_path), temp_path) != 0, "couldn't get temp folder path\n");
> - wcscat(temp_path, L"robocopy_test\");
> - ok(CreateDirectoryW(temp_path, NULL), "couldn't create temp test folder %s, error %d\n", debugstr_w(temp_path), GetLastError());
> - ok(SetCurrentDirectoryW(temp_path), "couldn't set CWD to temp folder %s\n", debugstr_w(temp_path));
> -
> - for (i = 0; i < ARRAY_SIZE(invalid_syntax_tests); i++)
> - {
> -
>    run_cmd(invalid_syntax_tests[i], &exit_code);
> -
>    ok(exit_code == 16, "unexpected exit code %d from command %s\n", exit_code, debugstr_w(invalid_syntax_tests[i]));
> - }
> -
> - winetest_push_context("basic copy test 1");
> - create_test_source_folder();
> - run_cmd(L"robocopy source destination /r:1 /w:0", &exit_code);
> - todo_wine ok(exit_code == 1, "unexpected exit code %d\n", exit_code);
> - check_basic_copy_1_test();
> - winetest_pop_context();
> -

There's a lot of tests like this one that have the same result; should 
we put those in a loop as well? You won't be able to catch them all this 
way, of course (e.g. not the absolute paths), but many of the wildcard 
and flag parsing tests could be deduplicated.

A couple more tests I'd like to see that occur to me:

* non-wildcard path spec,

* multiple path specs,

* absolute path spec.