Hi David,
The patch looks much better now, but needs a bit more. BTW, this would
be better tested on protocol level (like we do in protocol.c) than
moniker binding, but this will do too.
On 1/3/11 3:47 AM, David Hedberg wrote:
> Try 2: Better tests, among other things.
> ---
> dlls/urlmon/http.c | 173 +++++++++++++++++++++++++++++++++++++-
> dlls/urlmon/tests/url.c | 211 +++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 373 insertions(+), 11 deletions(-)
>
> @@ -2546,11 +2670,19 @@ static void test_BindToStorage(int protocol, DWORD flags, DWORD t)
> SET_EXPECT(QueryInterface_IHttpNegotiate);
> SET_EXPECT(QueryInterface_IWindowForBindingUI);
> SET_EXPECT(QueryService_IWindowForBindingUI);
> + SET_EXPECT(GetWindow_IWindowForBindingUI);
> SET_EXPECT(BeginningTransaction);
> SET_EXPECT(QueryInterface_IHttpNegotiate2);
> SET_EXPECT(GetRootSecurityId);
> SET_EXPECT(OnProgress_FINDINGRESOURCE);
> SET_EXPECT(OnProgress_CONNECTING);
> + if(flags& BINDTEST_INVALID_CN) {
> + SET_EXPECT(QueryInterface_IHttpSecurity);
> + SET_EXPECT(QueryService_IHttpSecurity);
> + SET_EXPECT(OnSecurityProblem);
> + if(SUCCEEDED(onsecurityproblem_hres))
> + SET_EXPECT(GetWindow_IHttpSecurity);
> + }
> }
> if(!no_callback) {
> if(test_protocol == HTTP_TEST || test_protocol == HTTPS_TEST || test_protocol == FTP_TEST
> @@ -2600,6 +2732,47 @@ static void test_BindToStorage(int protocol, DWORD flags, DWORD t)
> ok(hres == INET_E_DATA_NOT_AVAILABLE,
> "IMoniker_BindToStorage failed: %08x, expected INET_E_DATA_NOT_AVAILABLE\n", hres);
> ok(unk == NULL, "istr should be NULL\n");
> + }else if(flags& BINDTEST_INVALID_CN) {
> + if(invalid_cn_accepted) {
> + todo_wine CHECK_NOT_CALLED(QueryInterface_IHttpSecurity);
> + todo_wine CHECK_NOT_CALLED(QueryService_IHttpSecurity);
> + todo_wine CHECK_NOT_CALLED(OnSecurityProblem);
> + }else {
> + CHECK_CALLED(QueryInterface_IHttpSecurity);
> + CHECK_CALLED(QueryService_IHttpSecurity);
> + CHECK_CALLED(OnSecurityProblem);
> + if(onsecurityproblem_hres != S_OK || security_problem == ERROR_INTERNET_SEC_CERT_ERRORS) {
> + CHECK_NOT_CALLED(QueryInterface_IInternetBindInfo);
> + CHECK_NOT_CALLED(QueryService_IInternetBindInfo);
> + CHECK_CALLED(QueryInterface_IHttpNegotiate);
> + CHECK_CALLED(BeginningTransaction);
> + CHECK_CALLED(QueryInterface_IHttpNegotiate2);
> + CHECK_CALLED(GetRootSecurityId);
> + CLEAR_CALLED(GetWindow_IWindowForBindingUI);
> + CLEAR_CALLED(QueryInterface_IWindowForBindingUI);
> + CLEAR_CALLED(QueryService_IWindowForBindingUI);
> + if(onsecurityproblem_hres == S_FALSE) {
> + if(security_problem == ERROR_INTERNET_SEC_CERT_ERRORS)
> + CLEAR_CALLED(OnProgress_FINDINGRESOURCE);
> + else
> + CHECK_CALLED(OnProgress_FINDINGRESOURCE);
> + CHECK_CALLED(GetWindow_IHttpSecurity);
> + }else {
> + todo_wine CHECK_NOT_CALLED(OnProgress_FINDINGRESOURCE);
> + CHECK_NOT_CALLED(GetWindow_IHttpSecurity);
> + }
> + }
> + }
> + if(binding_hres != S_OK) {
> + ok(hres == binding_hres, "Got %08x\n", hres);
> + ok(unk == NULL, "Got %p\n", unk);
> + }else if(invalid_cn_accepted) {
> + todo_wine ok(hres == S_OK, "IMoniker_BindToStorage failed: %08x\n", hres);
> + todo_wine ok(unk != NULL, "unk == NULL\n");
> + }else {
> + ok(hres == S_OK, "IMoniker_BindToStorage failed: %08x\n", hres);
> + ok(unk != NULL, "unk == NULL\n");
> + }
This is making tests even harder to maintain than they are now. I guess
you're duplicating checking called function here because of returning on
failure a few lines later. It's the return that shouldn't be there. You
can change it to
if(FAILED(hres) && !(flags & BINDTEST_INVALID_CN))
return;
if you don't want to deal with existing problems in error handling.
Jacek