Re: user32: Improve the LoadImage() test.

[Dmitry Timoshkov]

"Andrew Riedi"  wrote:
> - icon_entry->xHotspot = 1;
> - icon_entry->yHotspot = 1;
> - icon_entry->xHotspot = 1; /* Color planes for .ico. */
> - icon_entry->yHotspot = ICON_BPP; /* BPP for .ico. */

...
>      ok(icon_info.xHotspot == 1, "xHotspot is %u.\n", icon_info.xHotspot);
> -
>    ok(icon_info.yHotspot == 1, "yHotspot is %u.\n", icon_info.yHotspot);
> -
>    ok(icon_info.yHotspot == 32, "yHotspot is %u.\n", icon_info.yHotspot);

It should be ICON_BPP instead of 32 for consistency.
> - /* Test loading an icon as an icon. */
> - SetLastError(0xdeadbeef);
> - handle = LoadImageA(NULL, "icon.ico", IMAGE_ICON, 0, 0, LR_LOADFROMFILE);
> - ok(handle != NULL, "LoadImage() failed.\n");
> - error = GetLastError();
> - ok(error == 0 ||
> -
>    broken(error == 0xdeadbeef) || /* Win9x */
> -
>    broken(error == ERROR_BAD_PATHNAME), /* Win98, WinMe */
> -
>    "Last error: %u\n", error);

There is no point in testing last error value if the API didn't fail.
> - /* Test the icon information. */
> - SetLastError(0xdeadbeef);
> - ret = GetIconInfo(handle, &icon_info);
> - ok(ret, "GetIconInfo() failed.\n");
> - error = GetLastError();
> - ok(error == 0xdeadbeef, "Last error: %u\n", error);

Same here.
> - if (ret)
> - {
> -
>    ok(icon_info.fIcon == TRUE, "fIcon == FALSE.\n");
> -
>    ok(icon_info.xHotspot == 16, "xHotspot is %u.\n", icon_info.xHotspot);
> -
>    ok(icon_info.yHotspot == 16, "yHotspot is %u.\n", icon_info.yHotspot);
> -
>    ok(icon_info.hbmColor != NULL, "No hbmColor!\n");
> -
>    ok(icon_info.hbmMask != NULL, "No hbmMask!\n");
> - }

'if (ret)' is useless if you don't expect a failure.
> -
> - /* Clean up. */
> - SetLastError(0xdeadbeef);
> - ret = DestroyIcon(handle);
> - ok(ret, "DestroyIcon() failed.\n");
> - error = GetLastError();
> - ok(error == 0xdeadbeef, "Last error: %u\n", error);

Same here.

user32: Improve the LoadImage() test.

[Andrew Riedi]

On Wed, Nov 26, 2008 at 10:14 PM, Dmitry Timoshkov
 wrote:
> "Andrew Riedi"  wrote:
> > - icon_entry->xHotspot = 1;
> > - icon_entry->yHotspot = 1;
> > - icon_entry->xHotspot = 1; /* Color planes for .ico. */
> > - icon_entry->yHotspot = ICON_BPP; /* BPP for .ico. */
>
> ...
> >     ok(icon_info.xHotspot == 1, "xHotspot is %u.\n",
> >
> > icon_info.xHotspot);
> > -
> >    ok(icon_info.yHotspot == 1, "yHotspot is %u.\n",
> >
> > icon_info.yHotspot);
> > -
> >    ok(icon_info.yHotspot == 32, "yHotspot is %u.\n",
> >
> > icon_info.yHotspot);
>
> It should be ICON_BPP instead of 32 for consistency.
> > - /* Test loading an icon as an icon. */
> > - SetLastError(0xdeadbeef);
> > - handle = LoadImageA(NULL, "icon.ico", IMAGE_ICON, 0, 0,
> >
> > LR_LOADFROMFILE);
> > - ok(handle != NULL, "LoadImage() failed.\n");
> > - error = GetLastError();
> > - ok(error == 0 ||
> > -
> >    broken(error == 0xdeadbeef) || /* Win9x */
> > -
> >    broken(error == ERROR_BAD_PATHNAME), /* Win98, WinMe */
> > -
> >    "Last error: %u\n", error);
>
> There is no point in testing last error value if the API didn't fail.
> > - /* Test the icon information. */
> > - SetLastError(0xdeadbeef);
> > - ret = GetIconInfo(handle, &icon_info);
> > - ok(ret, "GetIconInfo() failed.\n");
> > - error = GetLastError();
> > - ok(error == 0xdeadbeef, "Last error: %u\n", error);
>
> Same here.
> > - if (ret)
> > - {
> > -
> >    ok(icon_info.fIcon == TRUE, "fIcon == FALSE.\n");
> > -
> >    ok(icon_info.xHotspot == 16, "xHotspot is %u.\n",
> >
> > icon_info.xHotspot);
> > -
> >    ok(icon_info.yHotspot == 16, "yHotspot is %u.\n",
> >
> > icon_info.yHotspot);
> > -
> >    ok(icon_info.hbmColor != NULL, "No hbmColor!\n");
> > -
> >    ok(icon_info.hbmMask != NULL, "No hbmMask!\n");
> > - }
>
> 'if (ret)' is useless if you don't expect a failure.
> > -
> > - /* Clean up. */
> > - SetLastError(0xdeadbeef);
> > - ret = DestroyIcon(handle);
> > - ok(ret, "DestroyIcon() failed.\n");
> > - error = GetLastError();
> > - ok(error == 0xdeadbeef, "Last error: %u\n", error);
>
> Same here.
>
> --
> Dmitry.

All great tips.  I will rewrite this and clean up some of the existing
code in that file.  As always, thanks for looking over my work,
Dmitry!

user32: Improve the LoadImage() test.

[Rob Shearman]

2008/11/27 Dmitry Timoshkov :
> "Andrew Riedi"  wrote:
> > - /* Test loading an icon as an icon. */
> > - SetLastError(0xdeadbeef);
> > - handle = LoadImageA(NULL, "icon.ico", IMAGE_ICON, 0, 0, LR_LOADFROMFILE);
> > - ok(handle != NULL, "LoadImage() failed.\n");
> > - error = GetLastError();
> > - ok(error == 0 ||
> > -
> >    broken(error == 0xdeadbeef) || /* Win9x */
> > -
> >    broken(error == ERROR_BAD_PATHNAME), /* Win98, WinMe */
> > -
> >    "Last error: %u\n", error);
>
> There is no point in testing last error value if the API didn't fail.

There is in this case - if the test is correct it shows that we expect
the last error to be set to 0 on success on, not left at its previous
value (0xdeadbeef).