Re: [PATCH 1/2] shell32: SHFileOperation: test added [try 2]

[James Hawkins]

2008/6/18 Vitaly Perov :
> Changelog:
> - shell32: test added. It check whether SHFileOperation could correctly work
>
> with input files mask '*.*'

Couple problems:

--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -120,6 +120,7 @@ static void init_shfo_tests(void)
     CreateDirectoryA("testdir2", NULL);
     CreateDirectoryA("testdir2\nested", NULL);
     createTestFile("testdir2\one.txt");
+    createTestFile("testdir2\one2.txt");
     createTestFile("testdir2\nested\two.txt");
 }

@@ -136,11 +137,14 @@ static void clean_after_shfo_tests(void)
     DeleteFileA("test4.txt\test3.txt");
     RemoveDirectoryA("test4.txt");
     DeleteFileA("testdir2\one.txt");
+    DeleteFileA("testdir2\one2.txt");
     DeleteFileA("testdir2\test1.txt");
     DeleteFileA("testdir2\test2.txt");
     DeleteFileA("testdir2\test3.txt");
     DeleteFileA("testdir2\test4.txt\test1.txt");
     DeleteFileA("testdir2\nested\two.txt");
+    DeleteFileA("testdir2\nested\one.txt");
+    DeleteFileA("testdir2\nested\one2.txt");
     RemoveDirectoryA("testdir2\test4.txt");
     RemoveDirectoryA("testdir2\nested");
     RemoveDirectoryA("testdir2");

Why are you adding another test file?  There are already multiple *.*
files created in the test directory.

+    /* try moving with MASK '*.*'  */
+    shfo.pFrom = "testdir2\*.*\0";
+    shfo.pTo = "testdir2\nested\0";
+    retval = SHFileOperationA(&shfo);
+    ok(retval == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", retval);
+    ok(!file_exists("testdir2\one.txt"), "Expected one.txt to not exist\n");
+    ok(!file_exists("testdir2\one2.txt"), "Expected one2.txt to not exist\n");
+    ok(file_exists("testdir2\nested\one.txt"), "Expected one.txt to
exist\n");
+    ok(file_exists("testdir2\nested\one2.txt"), "Expected one.txt
to exist\n");

This test is misleading.  We already test for moving multiple files to
a destination directory using a mask: line 1102 of the git version of
this file.  The misleading part is that what we don't test is moving
multiple files to a destination directory when the FOF_MULTIDESTFILES
flag is set (it's set in a previous test and never unset in your
patch).  This would be a much better addition to the tests (and easier
to understand because it follows from the previous test):

/* move files using glob to a dest dir with FOF_MULTIDESTFILES */
shfo.fFlags |= FOF_MULTIDESTFILES;
set_curr_dir_path(from, "test?.txt\0");
set_curr_dir_path(to, "testdir2\0");
ok(!file_exists("testdir2\test2.txt"), "The file is not moved yet\n");
ok(!file_exists("testdir2\test4.txt"), "The directory is not moved yet\n");
retval = SHFileOperationA(&shfo);
ok(retval == ERROR_SUCCESS, "Files and directories are moved to directory\n");
ok(file_exists("testdir2\test2.txt"), "The file is moved\n");
ok(file_exists("testdir2\test4.txt"), "The directory is moved\n");
ok(file_exists("testdir2\test4.txt\test1.txt"), "The file in
subdirectory is moved\n");

This should immediately follow the similar test on line 1102.