[PATCH 1/3 resend] setupapi/tests: Change return values from coinst functions.

[Jeff Smith]

On Fri, Jul 24, 2020 at 11:10 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
> On 7/24/20 11:08 AM, Jeff Smith wrote:
> > On Fri, Jul 24, 2020 at 9:53 AM Zebediah Figura <z.figura12 at gmail.com> wrote:
> >>
> >> On 7/24/20 9:34 AM, Jeff Smith wrote:
> >>> As co_error and class_error both return 0xdeadbeef, use of 0xdeadbeef as
> >>> a canary value as well can lead to ambiguous tests results.
> >>>
> >>> As 0xdeadbeef is a typical canary value in wine, change the return
> >>> values for co_error and class_error, avoiding the potential ambiguity.
> >>
> >> Is there a reason why we can't just use a different canary value in 2/3?
> >
> > Technically, that would work as well, and that is what I did in the original
> > version of this patchset.  There was an objection raised though.
> > While I'm not strongly attached to one way or the other, I could see the
> > point of using deadbeef for the canary, and returning a different value
> > from co_error/class_error, as done in this patchset.
>
> I think that objection was rather specious.

Maybe.

> If nothing else, I don't
> think E_FAIL is a very good canary value.

Going to go out on a limb and say that the value returned by
co_error/class_error
(which is different from the canary) isn't that important as long as
it is unlikely to
be ambiguous as to where it came from.  So I just chose about the most generic
COM error that I could come up with.

As much as I want the tests to be the best they can be, TBH it's
really the fix (patch 3)
that I care about, so I'll quit splitting hairs.  You think forgetting
patch 1 and using a
different canary for patch 2 is the way to go?

> >>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> >>> ---
> >>> Patches 2 and 3 will work without patch 1.  This patch just makes tests
> >>> marginally more precise if an error does get caught.
> >>>
> >>>  dlls/setupapi/tests/coinst.c  |  4 ++--
> >>>  dlls/setupapi/tests/devinst.c | 10 +++++-----
> >>>  2 files changed, 7 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/dlls/setupapi/tests/coinst.c b/dlls/setupapi/tests/coinst.c
> >>> index 5e8d890cfa5..604bf74d1eb 100644
> >>> --- a/dlls/setupapi/tests/coinst.c
> >>> +++ b/dlls/setupapi/tests/coinst.c
> >>> @@ -47,7 +47,7 @@ DWORD WINAPI class_default(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *
> >>>
> >>>  DWORD WINAPI class_error(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *device)
> >>>  {
> >>> -    return 0xdeadbeef;
> >>> +    return E_FAIL;
> >>>  }
> >>>
> >>>  DWORD WINAPI co_success(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *device,
> >>> @@ -67,5 +67,5 @@ DWORD WINAPI CoDeviceInstall(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA
> >>>  DWORD WINAPI co_error(DI_FUNCTION function, HDEVINFO set, SP_DEVINFO_DATA *device,
> >>>          COINSTALLER_CONTEXT_DATA *context)
> >>>  {
> >>> -    return 0xdeadbeef;
> >>> +    return E_FAIL;
> >>>  }
> >>> diff --git a/dlls/setupapi/tests/devinst.c b/dlls/setupapi/tests/devinst.c
> >>> index d4c82dea1f6..80abc44029c 100644
> >>> --- a/dlls/setupapi/tests/devinst.c
> >>> +++ b/dlls/setupapi/tests/devinst.c
> >>> @@ -2727,17 +2727,17 @@ static void test_class_installer(void)
> >>>
> >>>      ret = SetupDiCallClassInstaller(DIF_ALLOW_INSTALL, set, &device);
> >>>      ok(!ret, "Expected failure.\n");
> >>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
> >>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
> >>>
> >>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
> >>>      ret = SetupDiCallClassInstaller(DIF_REGISTERDEVICE, set, &device);
> >>>      ok(!ret, "Expected failure.\n");
> >>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
> >>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
> >>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
> >>>
> >>>      ret = SetupDiCallClassInstaller(DIF_REMOVE, set, &device);
> >>>      ok(!ret, "Expected failure.\n");
> >>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
> >>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
> >>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
> >>>
> >>>      SetupDiDestroyDeviceInfoList(set);
> >>> @@ -2875,12 +2875,12 @@ static void test_class_coinstaller(void)
> >>>
> >>>      ret = SetupDiCallClassInstaller(DIF_ALLOW_INSTALL, set, &device);
> >>>      ok(!ret, "Expected failure.\n");
> >>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
> >>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
> >>>
> >>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
> >>>      ret = SetupDiCallClassInstaller(DIF_REGISTERDEVICE, set, &device);
> >>>      ok(!ret, "Expected failure.\n");
> >>> -    ok(GetLastError() == 0xdeadbeef, "Got unexpected error %#x.\n", GetLastError());
> >>> +    ok(GetLastError() == E_FAIL, "Got unexpected error %#x.\n", GetLastError());
> >>>      ok(!device_is_registered(set, &device), "Expected device not to be registered.\n");
> >>>
> >>>      SetupDiDestroyDeviceInfoList(set);
> >>>
> >>
> >>
> >
>