Re: [PATCH 1/2] kernel32/tests: Add UTF-7 non-encoded surrounding characters tests.

[Alexandre Julliard]

Alex Henrie  writes:
> -
>    SetLastError(0xdeadbeef);
> -
>    len = WideCharToMultiByte(CP_UTF7, 0, input, 4, output, sizeof(output) - 1, NULL, NULL);
> -
>    ok(GetLastError() == 0xdeadbeef,
> -
>       "i=0x%04x: expected error=0xdeadbeef, got error=0x%x\n", i, GetLastError());

Testing last error on success is not useful and should be avoided.

[PATCH 1/2] kernel32/tests: Add UTF-7 non-encoded surrounding characters tests.

[Sebastian Lackner]

On 05.11.2014 15:38, Alexandre Julliard wrote:
> Alex Henrie  writes:
> > -
> >    SetLastError(0xdeadbeef);
> > -
> >    len = WideCharToMultiByte(CP_UTF7, 0, input, 4, output, sizeof(output) - 1, NULL, NULL);
> > -
> >    ok(GetLastError() == 0xdeadbeef,
> > -
> >       "i=0x%04x: expected error=0xdeadbeef, got error=0x%x\n", i, GetLastError());
>
> Testing last error on success is not useful and should be avoided.

Whats the reason for this Wine decision? Its not a very useful one at my opinion.

Besides the fact that a lot of existing tests check that GetLastError() was unmodified (20 matches for "== 0xdeadbeef" in kernel32/tests/codepage.c), its important to know the exact behaviour when trying to propagate the error code, similar to how HeapFree(...) should keep the original error code on success.

The fact that there are also API functions where the error code is indeed modified on success (for example CreateMutexExW) makes me think that such a test can basically never hurt, to avoid hunting down weird failures in the future. It would not be the first example where programs check for success the wrong way, and only look at the error code.

Even if native would behave differently, and we probably would decide not to reproduce the behaviour - the tests are a good documentation of how it is supposed to work. The next person who wants to know if WideCharToMultibyte(CP_UTF7, ...) changes the error code, will have to write those tests just again.

Any good reason behind this?

Regards,
Sebastian

[PATCH 1/2] kernel32/tests: Add UTF-7 non-encoded surrounding characters tests.

[Alexandre Julliard]

Sebastian Lackner  writes:
> Even if native would behave differently, and we probably would decide
> not to reproduce the behaviour - the tests are a good documentation of
> how it is supposed to work. The next person who wants to know if
> WideCharToMultibyte(CP_UTF7, ...) changes the error code, will have to
> write those tests just again.
>
> Any good reason behind this?

With a few exceptions, Windows doesn't alter last error on success, so
its value depends more or less randomly on internal code paths, and on
whether there was a failure in one of the subfunctions.

Since we don't replicate the internal code paths, the only way to pass
the tests would be to explicitly set last error on success. Of course,
as soon as we start doing that, we change the last error behavior of all
the callers of that function, which then need to set last error too, or
worse, save/restore it around subfunction calls.

Pretty soon we'll be setting last error in every single code path, for
no benefit except passing a few misguided tests.

[PATCH 1/2] kernel32/tests: Add UTF-7 non-encoded surrounding characters tests.

[Sebastian Lackner]

On 06.11.2014 06:17, Alexandre Julliard wrote:
> Sebastian Lackner  writes:
> > Even if native would behave differently, and we probably would decide
> > not to reproduce the behaviour - the tests are a good documentation of
> > how it is supposed to work. The next person who wants to know if
> > WideCharToMultibyte(CP_UTF7, ...) changes the error code, will have to
> > write those tests just again.
> >
> > Any good reason behind this?
>
> With a few exceptions, Windows doesn't alter last error on success, so
> its value depends more or less randomly on internal code paths, and on
> whether there was a failure in one of the subfunctions.
>
> Since we don't replicate the internal code paths, the only way to pass
> the tests would be to explicitly set last error on success. Of course,
> as soon as we start doing that, we change the last error behavior of all
> the callers of that function, which then need to set last error too, or
> worse, save/restore it around subfunction calls.
>
> Pretty soon we'll be setting last error in every single code path, for
> no benefit except passing a few misguided tests.

Thanks for the explanation. I have to agree that it definitely doesn't make sense to reproduce always the native error behaviour at all costs, but I personally don't see any big problem when tests check it anyway, especially when the behaviour is very consistent like in this example (error only modified when the function runs out of buffer and fails). What if these functions would for example change the error code when encountering invalid utf16 surrogates in the parsed data? I know that its not the case because its covered by the tests. Nevertheless, its your decision of course, and if this is the only issue which prevents you from accepting this patch, I am pretty sure Alex will be fine with changing that. ;)

Regards,
Sebastian