[PATCH v8 1/3] robocopy/tests: Add basic conformance tests

[Zebediah Figura (she/her)]

On 10/31/21 15:34, Florian Eder wrote:
> Basic conformance tests create test source directories with
> files and sub folders, 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 <others.meder at gmail.com>
> ---
> v8: Added helper functions to check the source folder to reduce amount of
> duplicated code, use only the relative path for DeleteFileW / RemoveDirectoryW
> and used string arrays to reduce the amount of code necessary for the tests.
> 
> The remaining tests that are not using an array / loop structure either break not completly identical
> with the other tests or will not be fixed with the same patch (which would in turn break the tests when
> implementing the feature, as the test would succeed for most cmd strings, but not for all).
> 
> For the cmd return value, this could be fixed with a todo_wine_if and hard coding the array entries that fail,
> but then the check_*_test functions would still fail as there are also differences in the resulting files.
> 
> Another solution to this problem would be to also give the id of the specific array entry that is checked
> as an argument to the check_*_test functions, so that todo_wine_if could be used, but this would
> 
> a) require hardcoding array entry ids, which could easily change if tests are added from the top
>     instead of the end of the array,
> b) make the tests more complex, as an additional function is linked to the content of the cmd string array,
> c) deviate even further from other tests that are (AFAIK) seen as the "gold standard".
> 
> However, if you prefer this solution, feel free to tell me. :-)
> 
> Also, basic_copy_tests and absolute_copy_tests could be theoretically merged, as it should not hurt to
> use the strings in basic_copy_tests as format strings for swprintf (they don't contain any format specifiers,
> causing swprintf to do nothing).
> I think it's reasonable to seperate those tests anyway, as it's not directly visible what strings are added to the
> strings otherwise (it's IMO much more clear when talking about a test for absolute paths, which requires information only
> available at runtime), but if you think otherwise, I'll change it immediately. :-)

In general I personally don't think it's worth going to extremes to 
accommodate incremental removal of todo_wine. My rule of thumb is "order 
the tests before the patch unless it's hard to do so".

What you have right now already looks a lot easier to parse, I think, so 
it's fine as-is. What you might do to improve it even further is to do 
what most other tests do and separate each test into helper functions, 
e.g. test_basic_copy(), test_absolute_path(), test_wildcards().

Just a few other nitpicks below (sorry I keep missing things... the 
bigger a patch is, the harder it gets to find everything the first time...)

> +static void create_test_file(const WCHAR *relative_path, size_t size)
> +{
> +    HANDLE handle;
> +    WCHAR path[MAX_PATH];
> +    swprintf(path, ARRAY_SIZE(path), L"%s%s", temp_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());
> +    }
> +    CloseHandle(handle);
> +}
> +
> +static void create_test_folder(const WCHAR *relative_path)
> +{
> +    WCHAR path[MAX_PATH];
> +    swprintf(path, ARRAY_SIZE(path), L"%s%s", temp_path, relative_path);
> +
> +    CreateDirectoryW(path, NULL);
> +}
> +

I know I only said DeleteFile() and RemoveDirectory(), but, well, these 
ones too. Most Win32 functions are capable of taking relative paths; 
there's no reasons not to make use of that in places like this.

> +static void check_file(const WCHAR *relative_path, BOOL should_exist)

"relative_path" is somewhat redundant when you're not dealing with any 
other kind of path anymore.

> +{
> +    ok (DeleteFileW(relative_path) == should_exist, "file %s expected exist to be %d, but is %d\n", debugstr_w(relative_path), should_exist, !should_exist);

This ok message strikes me as a little more verbose than it needs to be. 
I'd suggest just "DeleteFile returned %d"; you could append "expected 
%d" to the end but it's pretty much implicit.

> +    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());
> +}

The way these lines wrap is very inconsistent. The first line in this 
function is very long; the last line wraps even where not particularly 
necessary.

The two ok() calls have inconsistent spacing before the parenthesis as well.

Same for check_folder() below of course.

> +
> +static void check_folder(const WCHAR *relative_path, BOOL should_exist)
> +{
> +    ok (RemoveDirectoryW(relative_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());
> +}
> +

...

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

"static const WCHAR *const invalid_syntax_tests[]" etc.