Re: kernel32/tests: Add test for FindFirstFileA()

[Dmitry Timoshkov]

Zhenbo Li <litimetal(a)gmail.com> wrote:
> +    strcpy(buffer2, nonexistent);
> +    handle = CreateFileA(buffer2, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0 );

There is no need for a strcpy().
> +    ok(handle != INVALID_HANDLE_VALUE, "failed to create a file\n");
> +    handle = FindFirstFileA(buffer2, &data);
> +    ok(handle != FindFirstFileA, "failed to find an existent file\n");

It would be useful to actually test what FindFirstFileA() returns.
Also not leaking the handle *twice* would be a bonus point.

-- 
Dmitry.

kernel32/tests: Add test for FindFirstFileA()

[Zhenbo Li]

Thank you.

2014-02-06 15:56 GMT+08:00 Dmitry Timoshkov <dmitry(a)baikal.ru>:
> Zhenbo Li <litimetal(a)gmail.com> wrote:
> > +    strcpy(buffer2, nonexistent);
> > +    handle = CreateFileA(buffer2, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0 );
>
> There is no need for a strcpy().
> Well, I'll add more tests later(like trailing \\).
> > +    ok(handle != INVALID_HANDLE_VALUE, "failed to create a file\n");
> > +    handle = FindFirstFileA(buffer2, &data);
> > +    ok(handle != FindFirstFileA, "failed to find an existent file\n");
>
> It would be useful to actually test what FindFirstFileA() returns.
> Also not leaking the handle *twice* would be a bonus point.

Thank you for mentioning that.
Most of the previous testcases leak the handle.
After this patch, is it necessary to fix them?



-- 
Have a nice day!
Zhenbo Li

kernel32/tests: Add test for FindFirstFileA()

[Dmitry Timoshkov]

Zhenbo Li <litimetal(a)gmail.com> wrote:
> > > +    strcpy(buffer2, nonexistent);
> > > +    handle = CreateFileA(buffer2, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0 );
> >
> > There is no need for a strcpy().
> > Well, I'll add more tests later(like trailing \\).

strcpy() could be added later when new tests get added.
> > > +    ok(handle != INVALID_HANDLE_VALUE, "failed to create a file\n");
> > > +    handle = FindFirstFileA(buffer2, &data);
> > > +    ok(handle != FindFirstFileA, "failed to find an existent file\n");
> >
> > It would be useful to actually test what FindFirstFileA() returns.
> > Also not leaking the handle *twice* would be a bonus point.
> > Thank you for mentioning that.
> > Most of the previous testcases leak the handle.
> > After this patch, is it necessary to fix them?

Every handle leak is bad on its own, and may lead to unwanted side
effects like inability to destroy an object or delete a file.

Another thing that could be improved in this test is printing last error
code in the failure message. Also you may find that making failure messages
shorter and simpler makes your life a bit easier. For axample:
"CreateFile(%s) error %d\n", name, GetLastError()
and
"FindFirstFile(%s) error %d\n", name, GetLastError()

-- 
Dmitry.