Re: [advapi32/tests #2] Don't set last error to 0 so we can check if it has changed

[Alexandre Julliard]

Paul Vriens  writes:
> We test for a last error of 0, so we shouldn't set it to 0 before the
> function is called. This however introduces a todo_wine.

Is there any reason to believe that last error should be 0 on success?
This looks more like a broken test to me.

[advapi32/tests #2] Don't set last error to 0 so we can check if it has changed

[Paul Vriens]

On Feb 18, 2008 2:17 PM, Alexandre Julliard  wrote:
> Paul Vriens  writes:
> > We test for a last error of 0, so we shouldn't set it to 0 before the
> > function is called. This however introduces a todo_wine.
>
> Is there any reason to believe that last error should be 0 on success?
> This looks more like a broken test to me.

Well no.

I agree it looks more like a broken test me as well. That's why I said
I didn't see any of the tests enter this
'if' statement.

This if statement was introduced by Rob Shearman:

http://source.winehq.org/git/wine.git/?a=commit;h=1731df4ca0460c3ef15b274503...

I have no clue what the test does and what's wrong with it so can we
leave it like this? Or should we ask Rob to go
way back in his memory (June 2005) ;-)

Cheers,

Paul.

[advapi32/tests #2] Don't set last error to 0 so we can check if it has changed

[Alexandre Julliard]

"Paul Vriens"  writes:
> I have no clue what the test does and what's wrong with it so can we
> leave it like this? Or should we ask Rob to go
> way back in his memory (June 2005) ;-)

Probably the last error check should simply be removed, since we expect
the call to succeed.

[advapi32/tests #2] Don't set last error to 0 so we can check if it has changed

[Robert Shearman]

Alexandre Julliard wrote:
> "Paul Vriens"  writes:
> > I have no clue what the test does and what's wrong with it so can we
> > leave it like this? Or should we ask Rob to go
> > way back in his memory (June 2005) ;-)

I've sent a patch to cause this part of the test to now be run.
> Probably the last error check should simply be removed, since we expect
> the call to succeed.

AccessCheck can return TRUE but still set the last error. AccessCheck 
only returns FALSE if an invalid parameter is input or there is 
something wrong with the token being checked against. It can return TRUE 
but set last error if access to the object secured by the security 
descriptor was denied.

[advapi32/tests #2] Don't set last error to 0 so we can check if it has changed

[Alexandre Julliard]

Robert Shearman  writes:
> AccessCheck can return TRUE but still set the last error. AccessCheck
> only returns FALSE if an invalid parameter is input or there is
> something wrong with the token being checked against. It can return
> TRUE but set last error if access to the object secured by the
> security descriptor was denied.

Yes, you have to check both the return value and the AccessStatus value,
but if both succeed there's no reason to expect last error to be set.
If it is really set in that case on Windows then the test needs to be
fixed to reflect that fact.