Re: shell32/tests: Add test for GetLastError in shlfileop

[Dmitry Timoshkov]

Zhenbo Li  wrote:
>  ret = SHFileOperationA(&shfo);
>  todo_wine
>  ok(ret == 1026 ||
>     ret == ERROR_FILE_NOT_FOUND || /* Vista */
>     broken(ret == ERROR_SUCCESS), /* NT4 */
>     "Expected 1026 or ERROR_FILE_NOT_FOUND, got %d\n", ret);
> - todo_wine
> - ok(GetLastError() == 0 ||
> -
>    broken(GetLastError()==ERROR_FILE_NOT_FOUND), /* win 2000 */
> -
>    "gle = %d\n", GetLastError() );

'ret == ERROR_SUCCESS (0)' case is marked as broken above, so using broken
for 'GetLastError() == 0' instead of ERROR_FILE_NOT_FOUND would match that
behaviour. Same comment applies for other added tests.

In general I don't think that adding last error tests for SHFileOperation
is very useful because 1) this API returns error codes directly 2) according
to the tests both returned error codes and set last error values differ in
almost every Windows version, so it's very unlikely that there is an app
that depends on this.

shell32/tests: Add test for GetLastError in shlfileop

[Zhenbo Li]

2014-01-28 Dmitry Timoshkov
> Zhenbo Li  wrote:
> >  ret = SHFileOperationA(&shfo);
> >  todo_wine
> >  ok(ret == 1026 ||
> >     ret == ERROR_FILE_NOT_FOUND || /* Vista */
> >     broken(ret == ERROR_SUCCESS), /* NT4 */
> >     "Expected 1026 or ERROR_FILE_NOT_FOUND, got %d\n", ret);
> > - todo_wine
> > - ok(GetLastError() == 0 ||
> > -
> >    broken(GetLastError()==ERROR_FILE_NOT_FOUND), /* win 2000 */
> > -
> >    "gle = %d\n", GetLastError() );
>
> Thank you for reviewing my patch.
> 'ret == ERROR_SUCCESS (0)' case is marked as broken above, so using broken
> for 'GetLastError() == 0' instead of ERROR_FILE_NOT_FOUND would match that
> behaviour. Same comment applies for other added tests.
>
> I don't think so.

I think we can divide windows versions to 2 group:
a) 2000 or before  b) xp or later

For group a, they behave as
> broken(ret == ERROR_SUCCESS)

implied, but their GetLastError() were remained as ERROR_FILE_NOT_FOUND

For group b, their 'ret' value isn't 0,
but GetLastError() is set to 0.
> In general I don't think that adding last error tests for SHFileOperation
> is very useful because 1) this API returns error codes directly 2)
> according
> to the tests both returned error codes and set last error values differ in
> almost every Windows version, so it's very unlikely that there is an app
> that depends on this.

You're right. As wine's documents[1] said,
> However, undocumented behavior should not be tested for unless there is

an application
 that relies on this behavior, and in that case the test should mention
that application, or
unless one can strongly expect applications to rely on this behavior

There are many 'todo_wine' marks in shell32 test, and it really caused
bugs(like 34324).
Maybe such test cases can contribute to further development?

[1]: http://www.winehq.org/docs/winedev-guide/testing-what

shell32/tests: Add test for GetLastError in shlfileop

[Dmitry Timoshkov]

Zhenbo Li  wrote:
> There are many 'todo_wine' marks in shell32 test, and it really caused
> bugs(like 34324).

According to the comments and the attached hack in the bug 34324 the problem
there is not related to the last error set but to the returned error value,
so you probably should add the tests for that particular behaviour.
> Maybe such test cases can contribute to further development?

Adding the tests is good in general, but the tests should be added very
carefully, and check something that real applications could depend upon.
Adding blanket tests just for everything is a waste of time IMHO.

shell32/tests: Add test for GetLastError in shlfileop

[Zhenbo Li]

2014-01-28 Dmitry Timoshkov
> > There are many 'todo_wine' marks in shell32 test, and it really caused
> > bugs(like 34324).
>
> According to the comments and the attached hack in the bug 34324 the problem
> there is not related to the last error set but to the returned error value,
> so you probably should add the tests for that particular behaviour.
> > Maybe such test cases can contribute to further development?
>
> Adding the tests is good in general, but the tests should be added very
> carefully, and check something that real applications could depend upon.
> Adding blanket tests just for everything is a waste of time IMHO.

Thanks for checking that.
As MSDN[1] said, "Do not use GetLastError with the return values of this
(SHFileOperation) function."

I think wine's implement of SHFileOperation needs a big change.
Before hacking it, I hope there could be sufficient testcases to
help me understand that function.

I had to admit that I'm lack of experience, and I don't know
how to judge if a testcase is necessary.

Thank you very much.

[1]: http://msdn.microsoft.com/en-us/library/windows/desktop/bb762164%28v=vs.85%2...

shell32/tests: Add test for GetLastError in shlfileop

[Dmitry Timoshkov]

Zhenbo Li  wrote:
> As MSDN[1] said, "Do not use GetLastError with the return values of this
> (SHFileOperation) function."
>
> I think wine's implement of SHFileOperation needs a big change.
> Before hacking it, I hope there could be sufficient testcases to
> help me understand that function.

It doesn't matter what MSDN says or doesn't say, what matters is what
applications depend upon.
> I think wine's implement of SHFileOperation needs a big change.
> Before hacking it, I hope there could be sufficient testcases to
> help me understand that function.

Before starting to change anything just explain (to yourself first)
'what you are trying to achieve' and even before that 'whether you have
investigated the problem, understand what is going on, and what exactly
the problem is that you are trying to fix'.

shell32/tests: Add test for GetLastError in shlfileop

[Zhenbo Li]

2014-01-28 Dmitry Timoshkov :
> Before starting to change anything just explain (to yourself first)
> 'what you are trying to achieve' and even before that 'whether you have
> investigated the problem, understand what is going on, and what exactly
> the problem is that you are trying to fix'.

Thank you for your advice.