Re: [3/4] secur32/tests: Add a test for QueryCredentialsAttributes(SECPKG_CRED_ATTR_NAMES). Take 2.
On 12/12/2013 09:13, Dmitry Timoshkov wrote:
+ + st = pQueryCredentialsAttributesA(&cred, SECPKG_CRED_ATTR_NAMES, &names); + ok(st == SEC_E_NO_CREDENTIALS || st == SEC_E_UNSUPPORTED_FUNCTION /* before Vista */, "expected SEC_E_NO_CREDENTIALS, got %08x\n", st); + Second possible return code should be broken(), otherwise your code change in next patch won't make any effect on tests.
Nikolay Sivov <bunglehead(a)gmail.com> wrote:
+ st = pQueryCredentialsAttributesA(&cred, SECPKG_CRED_ATTR_NAMES, &names); + ok(st == SEC_E_NO_CREDENTIALS || st == SEC_E_UNSUPPORTED_FUNCTION /* before Vista */, "expected SEC_E_NO_CREDENTIALS, got %08x\n", st); + Second possible return code should be broken(), otherwise your code change in next patch won't make any effect on tests.
SEC_E_UNSUPPORTED_FUNCTION is a perfectly valid error code, broken is not appropriate here. My next patch just changes Wine code to follow more recent Windows behaviour. -- Dmitry.
On 12/12/2013 10:21, Dmitry Timoshkov wrote:
Nikolay Sivov <bunglehead(a)gmail.com> wrote:
+ st = pQueryCredentialsAttributesA(&cred, SECPKG_CRED_ATTR_NAMES, &names); + ok(st == SEC_E_NO_CREDENTIALS || st == SEC_E_UNSUPPORTED_FUNCTION /* before Vista */, "expected SEC_E_NO_CREDENTIALS, got %08x\n", st); + Second possible return code should be broken(), otherwise your code change in next patch won't make any effect on tests. SEC_E_UNSUPPORTED_FUNCTION is a perfectly valid error code, broken is not appropriate here. My next patch just changes Wine code to follow more recent Windows behaviour.
broken() is a way to mark it as undesired code for wine implementation, it doesn't necessary mean that something changed significantly from error to success and such. Old return code will remain valid when running on windows.
Nikolay Sivov <bunglehead(a)gmail.com> wrote:
+ st = pQueryCredentialsAttributesA(&cred, SECPKG_CRED_ATTR_NAMES, &names); + ok(st == SEC_E_NO_CREDENTIALS || st == SEC_E_UNSUPPORTED_FUNCTION /* before Vista */, "expected SEC_E_NO_CREDENTIALS, got %08x\n", st); + Second possible return code should be broken(), otherwise your code change in next patch won't make any effect on tests. SEC_E_UNSUPPORTED_FUNCTION is a perfectly valid error code, broken is not appropriate here. My next patch just changes Wine code to follow more recent Windows behaviour.
broken() is a way to mark it as undesired code for wine implementation, it doesn't necessary mean that something changed significantly from error to success and such. Old return code will remain valid when running on windows.
That's just another way of action. There are numerous places in Wine tests that accept several different error codes and none of them is marked as broken, my patch just does the same thing. -- Dmitry.
On 12/12/2013 10:43, Dmitry Timoshkov wrote:
Nikolay Sivov <bunglehead(a)gmail.com> wrote:
+ st = pQueryCredentialsAttributesA(&cred, SECPKG_CRED_ATTR_NAMES, &names); + ok(st == SEC_E_NO_CREDENTIALS || st == SEC_E_UNSUPPORTED_FUNCTION /* before Vista */, "expected SEC_E_NO_CREDENTIALS, got %08x\n", st); + Second possible return code should be broken(), otherwise your code change in next patch won't make any effect on tests. SEC_E_UNSUPPORTED_FUNCTION is a perfectly valid error code, broken is not appropriate here. My next patch just changes Wine code to follow more recent Windows behaviour.
broken() is a way to mark it as undesired code for wine implementation, it doesn't necessary mean that something changed significantly from error to success and such. Old return code will remain valid when running on windows. That's just another way of action. I'm just pointing out that a particular improvement could be made to ensure code change is actually tested, that's all. There are numerous places in Wine tests that accept several different error codes and none of them is marked as broken, my patch just does the same thing.
Well, that's irrelevant, there's some places where it's done like this and some where it's not. But I get that you're not willing to change a patch, so I'll just wait for possible comments from others.
Nikolay Sivov <bunglehead(a)gmail.com> wrote:
+ st = pQueryCredentialsAttributesA(&cred, SECPKG_CRED_ATTR_NAMES, &names); + ok(st == SEC_E_NO_CREDENTIALS || st == SEC_E_UNSUPPORTED_FUNCTION /* before Vista */, "expected SEC_E_NO_CREDENTIALS, got %08x\n", st); + Second possible return code should be broken(), otherwise your code change in next patch won't make any effect on tests. SEC_E_UNSUPPORTED_FUNCTION is a perfectly valid error code, broken is not appropriate here. My next patch just changes Wine code to follow more recent Windows behaviour.
broken() is a way to mark it as undesired code for wine implementation, it doesn't necessary mean that something changed significantly from error to success and such. Old return code will remain valid when running on windows. That's just another way of action. I'm just pointing out that a particular improvement could be made to ensure code change is actually tested, that's all.
Thanks, I believe that I've answered that part.
There are numerous places in Wine tests that accept several different error codes and none of them is marked as broken, my patch just does the same thing.
Well, that's irrelevant, there's some places where it's done like this and some where it's not.
And what is the difference between those places? Isn't that up to the patch author or there are some technical reasons?
But I get that you're not willing to change a patch, so I'll just wait for possible comments from others.
My impression was that you were trying to clarify the reason why my patch doesn't use broken() and see my explanation, but now I see that actual reason was to "prove" my "willingless" to change the patches I send. -- Dmitry.
participants (2)
-
Dmitry Timoshkov -
Nikolay Sivov