Re: [1/3] taskschd/tests: Add some tests for task registration info.

[Nikolay Sivov]

On 4/11/2014 11:26, Dmitry Timoshkov wrote:
> - hr = IRegistrationInfo_get_Description(reginfo, &bstr);
>
> +todo_wine
> - ok(hr == S_OK, "get_Description error %#x\n", hr);
>
> +if (hr == S_OK)
> +{
> - ok(bstr && !lstrcmpW(bstr, Task1), "expected Task1, got %s\n", wine_dbgstr_w(bstr));

It's redundant to check for 'bstr' being not NULL, lstrcmpW test will 
fail in this case too. For other tests when it returns NULL bstr it 
would be cleaner to set pointer to some non-zero value to test that each 
method actually resets it to NULL.

[1/3] taskschd/tests: Add some tests for task registration info.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> On 4/11/2014 11:26, Dmitry Timoshkov wrote:
> > - hr = IRegistrationInfo_get_Description(reginfo, &bstr);
> >
> > +todo_wine
> > - ok(hr == S_OK, "get_Description error %#x\n", hr);
> >
> > +if (hr == S_OK)
> > +{
> > - ok(bstr && !lstrcmpW(bstr, Task1), "expected Task1, got %s\n", wine_dbgstr_w(bstr));
>
> It's redundant to check for 'bstr' being not NULL, lstrcmpW test will 
> fail in this case too.

I had an impression that when __WINESRC__ is defined lstrcmpW() gets replaced
by an inline implementation which doesn't have a SEH wrapper and crashes, but
it looks like that's not the case. Thanks.
> For other tests when it returns NULL bstr it 
> would be cleaner to set pointer to some non-zero value to test that each 
> method actually resets it to NULL.

That's a matter of taste.

[1/3] taskschd/tests: Add some tests for task registration info.

[Nikolay Sivov]

On 4/11/2014 11:56, Dmitry Timoshkov wrote:
> > For other tests when it returns NULL bstr it
> > would be cleaner to set pointer to some non-zero value to test that each
> > method actually resets it to NULL.
>
> That's a matter of taste.

Not really. You don't know if it's changed or not after the call if you 
don't know what initial value was.
For series of calls like these:

---

+    hr = IRegistrationInfo_get_Documentation(reginfo, &bstr);
+todo_wine
+    ok(hr == S_OK, "get_Documentation error %#x\n", hr);
+if (hr == S_OK)
+    ok(!bstr, "expected NULL, got %s\n", wine_dbgstr_w(bstr));
+    hr = IRegistrationInfo_get_URI(reginfo, &bstr);
+todo_wine
+    ok(hr == S_OK, "get_URI error %#x\n", hr);
+if (hr == S_OK)
+    ok(!bstr, "expected NULL, got %s\n", wine_dbgstr_w(bstr));
---

there's no way to tell did get_URI set it to NULL or not.

[1/3] taskschd/tests: Add some tests for task registration info.

[Dmitry Timoshkov]

Nikolay Sivov  wrote:
> > > For other tests when it returns NULL bstr it
> > > would be cleaner to set pointer to some non-zero value to test that each
> > > method actually resets it to NULL.
> >
> > That's a matter of taste.
>
> Not really. You don't know if it's changed or not after the call if you 
> don't know what initial value was.
> For series of calls like these:
> - hr = IRegistrationInfo_get_Documentation(reginfo, &bstr);
>
> +todo_wine
> - ok(hr == S_OK, "get_Documentation error %#x\n", hr);
>
> +if (hr == S_OK)
> - ok(!bstr, "expected NULL, got %s\n", wine_dbgstr_w(bstr));
> - hr = IRegistrationInfo_get_URI(reginfo, &bstr);
>
> +todo_wine
> - ok(hr == S_OK, "get_URI error %#x\n", hr);
>
> +if (hr == S_OK)
> - ok(!bstr, "expected NULL, got %s\n", wine_dbgstr_w(bstr));
>
> there's no way to tell did get_URI set it to NULL or not.

In the case of error I agree, but if the method returned S_OK that
means that it really returned something.