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@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.