2014-01-28 Dmitry Timoshkov <dmitry@baikal.ru>

Zhenbo Li <litimetal@gmail.com> 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

--
Have a nice day!
Zhenbo Li