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.

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

[Vitaly Perov]

> /* 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");

Yes! It's exactly what I want to test!
But in last version the source is quite different:

shfo.lpszProgressTitle = NULL;

set_curr_dir_path(from, "test1.txt\0");
    set_curr_dir_path(to, "test4.txt\0");
    ok(!SHFileOperationA(&shfo), "Prepare test to check how directories are 
moved recursively\n");
    ok(!file_exists("test1.txt"), "test1.txt should not exist\n");
    ok(file_exists("test4.txt\test1.txt"), "The file is not moved\n");

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");
    ok(!SHFileOperationA(&shfo), "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");

It's passes! If I insert the following line:
> shfo.fFlags |= FOF_MULTIDESTFILES;

It fails!

I was unable to find when these changes have been done.
So, may be it's your local uncommited changes?

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

[James Hawkins]

On Thu, Jun 19, 2008 at 6:24 AM, Vitaly Perov  wrote:
> > /* 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");
>
> Yes! It's exactly what I want to test!
> But in last version the source is quite different:
>
> shfo.lpszProgressTitle = NULL;
>
> set_curr_dir_path(from, "test1.txt\0");
>    set_curr_dir_path(to, "test4.txt\0");
>    ok(!SHFileOperationA(&shfo), "Prepare test to check how directories are
> moved recursively\n");
>    ok(!file_exists("test1.txt"), "test1.txt should not exist\n");
>    ok(file_exists("test4.txt\test1.txt"), "The file is not moved\n");
>
> 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");
>    ok(!SHFileOperationA(&shfo), "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");
>
> It's passes! If I insert the following line:
> > shfo.fFlags |= FOF_MULTIDESTFILES;
>
> It fails!

That's exactly what I just said in my previous email.
> I was unable to find when these changes have been done.
> So, may be it's your local uncommited changes?

I don't know what you're talking about.