Re: [PATCH v2] kernel32/tests: Add more tests for GetConsoleFontInfo

[Sebastian Lackner]

On 29.04.2016 13:29, Hugh McMaster wrote:
> Signed-off-by: Hugh McMaster
>
> dlls/kernel32/tests/console.c | 76 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 71 insertions(+), 5 deletions(-)
>
> diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
> index 3f627be..756eaba 100644
> --- a/dlls/kernel32/tests/console.c
> +++ b/dlls/kernel32/tests/console.c
> @@ -2789,7 +2789,7 @@ static void test_GetConsoleFontInfo(HANDLE std_output)
>      CONSOLE_FONT_INFO *cfi;
>      BOOL ret;
>      CONSOLE_SCREEN_BUFFER_INFO csbi;
> - COORD orig_font, tmp_font;
> -
>
> COORD orig_sb_size, tmp_sb_size, orig_font, tmp_font;
>
> hmod = GetModuleHandleA("kernel32.dll");
> pGetConsoleFontInfo = (void *)GetProcAddress(hmod, "GetConsoleFontInfo");
>
> @@ -2811,20 +2811,35 @@ static void test_GetConsoleFontInfo(HANDLE std_output)
>      cfi = HeapAlloc(GetProcessHeap(), 0, memsize);
>      memset(cfi, 0, memsize);
> - GetConsoleScreenBufferInfo(std_output, &csbi);
> - orig_sb_size = csbi.dwSize;
> - tmp_sb_size.X = csbi.dwSize.X + 3;
> - tmp_sb_size.Y = csbi.dwSize.Y + 5;
> - SetConsoleScreenBufferSize(std_output, tmp_sb_size);
> - SetLastError(0xdeadbeef);
>    ret = pGetConsoleFontInfo(NULL, FALSE, 0, cfi);
>    ok(!ret, "got %d, expected zero\n", ret);
>    todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, "got %u, expected 6\n", GetLastError());
> - ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> - ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> - ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);

Those tests do not add much value. Without initializing cfi before each call,
its impossible to tell if GetConsoleFontInfo() nulled the struct or if it wasn't
touched at all. However, I'm also not sure if we have to care about the struct
content in case of a failure.
>  SetLastError(0xdeadbeef);
>  ret = pGetConsoleFontInfo(GetStdHandle(STD_INPUT_HANDLE), FALSE, 0, cfi);
>  ok(!ret, "got %d, expected zero\n", ret);
>  todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, "got %u, expected 6\n", GetLastError());
> -
>
> ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> -
>
> ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> -
>
> ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
>
> SetLastError(0xdeadbeef);
> ret = pGetConsoleFontInfo(std_output, FALSE, 0, cfi);
> ok(!ret, "got %d, expected zero\n", ret);
> todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> -
>
> ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> -
>
> ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> -
>
> ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
>
> GetConsoleScreenBufferInfo(std_output, &csbi);
> win_width = csbi.srWindow.Right - csbi.srWindow.Left + 1;
>
> @@ -2834,22 +2849,73 @@ static void test_GetConsoleFontInfo(HANDLE std_output)
>      index = cfi[0].nFont;
>      orig_font = GetConsoleFontSize(std_output, index);
> - memset(cfi, 0, memsize);
> SetLastError(0xdeadbeef);
> ret = pGetConsoleFontInfo(std_output, FALSE, num_fonts, cfi);
> todo_wine ok(ret, "got %d, expected non-zero\n", ret);
> todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> - ok(cfi[index].dwFontSize.X == win_width, "got %d, expected %d\n", cfi[index].dwFontSize.X, win_width);
> - ok(cfi[index].dwFontSize.Y == win_height, "got %d, expected %d\n", cfi[index].dwFontSize.Y, win_height);
> -
>
> todo_wine ok(cfi[index].dwFontSize.X == win_width, "got %d, expected %d\n",
> -
>             cfi[index].dwFontSize.X, win_width);
> -
>
> todo_wine ok(cfi[index].dwFontSize.Y == win_height, "got %d, expected %d\n",
> -
>             cfi[index].dwFontSize.Y, win_height);
>
> for (i = 0; i < num_fonts; i++)
>    {
>        tmp_font = GetConsoleFontSize(std_output, cfi[i].nFont);
>        tmp_w = (double)orig_font.X / tmp_font.X * win_width;
>        tmp_h = (double)orig_font.Y / tmp_font.Y * win_height;
> -
>    ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> -
>    ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> -
>    todo_wine ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> -
>    todo_wine ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);

As mentioned in one of the previous mails, I still think it would be useful to have a test like
ok(cfi[i].nFont == i, "...\n"); to show that the elements are returned in sorted order, and that
the font index matches the array index. Otherwise accessing by index (as done in the test above)
would not work. Did you defer it to a later patch or was there anything unclear?
> - }
> -
> - memset(cfi, 0, memsize);
> - SetLastError(0xdeadbeef);
> - ret = pGetConsoleFontInfo(NULL, TRUE, 0, cfi);
> - ok(!ret, "got %d, expected zero\n", ret);
> - todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, "got %u, expected 6\n", GetLastError());
> - ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> - ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> - ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
> -
> - SetLastError(0xdeadbeef);
> - ret = pGetConsoleFontInfo(GetStdHandle(STD_INPUT_HANDLE), TRUE, 0, cfi);
> - ok(!ret, "got %d, expected zero\n", ret);
> - todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, "got %u, expected 6\n", GetLastError());
> - ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> - ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> - ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
> -
> - SetLastError(0xdeadbeef);
> - ret = pGetConsoleFontInfo(std_output, TRUE, 0, cfi);
> - ok(!ret, "got %d, expected zero\n", ret);
> - todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> - ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> - ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> - ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
> -
> - memset(cfi, 0, memsize);
> - SetLastError(0xdeadbeef);
> - ret = pGetConsoleFontInfo(std_output, TRUE, num_fonts, cfi);
> - todo_wine ok(ret, "got %d, expected non-zero\n", ret);
> - todo_wine ok(GetLastError() == 0xdeadbeef, "got %u, expected 0xdeadbeef\n", GetLastError());
> -
> - todo_wine ok(cfi[index].dwFontSize.X == csbi.dwMaximumWindowSize.X, "got %d, expected %d\n",
> -
>   cfi[index].dwFontSize.X, csbi.dwMaximumWindowSize.X);
> - todo_wine ok(cfi[index].dwFontSize.Y == csbi.dwMaximumWindowSize.Y, "got %d, expected %d\n",
> -
>   cfi[index].dwFontSize.Y, csbi.dwMaximumWindowSize.Y);
> -
> - for (i = 0; i < num_fonts; i++)
> - {
> -
>    tmp_font = GetConsoleFontSize(std_output, cfi[i].nFont);
> -
>    tmp_w = (double)orig_font.X / tmp_font.X * csbi.dwMaximumWindowSize.X;
> -
>    tmp_h = (double)orig_font.Y / tmp_font.Y * csbi.dwMaximumWindowSize.Y;
> -
>    todo_wine ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> -
>    todo_wine ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> }
> -
> - HeapFree(GetProcessHeap(), 0, cfi);
> - SetConsoleScreenBufferSize(std_output, orig_sb_size);
>
> }
>
> START_TEST(console)

[PATCH v2] kernel32/tests: Add more tests for GetConsoleFontInfo

[Hugh McMaster]

On Friday, 29 April 2016 11:07 PM, Sebastian Lackner wrote:

On 29.04.2016 13:29, Hugh McMaster wrote:
> >  SetLastError(0xdeadbeef);
> >  ret = pGetConsoleFontInfo(NULL, FALSE, 0, cfi);
> >  ok(!ret, "got %d, expected zero\n", ret);
> >  todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, "got %u, expected 6\n", GetLastError());
> > - ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> > - ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> > - ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
>
> Those tests do not add much value. Without initializing cfi before each call,
> its impossible to tell if GetConsoleFontInfo() nulled the struct or if it wasn't
> touched at all. However, I'm also not sure if we have to care about the struct
> content in case of a failure.

The values in the struct weren't altered from zero after the first failed call. But I take your point.
I'll try memset-ing another value and checking the returned values.
>  for (i = 0; i < num_fonts; i++)
>  {
>      tmp_font = GetConsoleFontSize(std_output, cfi[i].nFont);
>      tmp_w = (double)orig_font.X / tmp_font.X * win_width;
>      tmp_h = (double)orig_font.Y / tmp_font.Y * win_height;
> -
>    ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> -
>    ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> -
>    todo_wine ok(cfi[i].dwFontSize.X == tmp_w, "got %d, expected %d\n", cfi[i].dwFontSize.X, tmp_w);
> -
>    todo_wine ok(cfi[i].dwFontSize.Y == tmp_h, "got %d, expected %d\n", cfi[i].dwFontSize.Y, tmp_h);
> As mentioned in one of the previous mails, I still think it would be useful to have a test like
> ok(cfi[i].nFont == i, "...\n"); to show that the elements are returned in sorted order, and that
> the font index matches the array index. Otherwise accessing by index (as done in the test above)
> would not work. Did you defer it to a later patch or was there anything unclear?

This was on my 'to do' list. Anyway, I've never seen behaviour to suggest the elements aren't
sorted, but I'll add the test to each loop to make certain.

[PATCH v2] kernel32/tests: Add more tests for GetConsoleFontInfo

[Hugh McMaster]

On Friday, 29 April 2016 11:40 PM, Hugh McMaster wrote:
> On Friday, 29 April 2016 11:07 PM, Sebastian Lackner wrote:
>
> On 29.04.2016 13:29, Hugh McMaster wrote:
> > >  SetLastError(0xdeadbeef);
> > >  ret = pGetConsoleFontInfo(NULL, FALSE, 0, cfi);
> > >  ok(!ret, "got %d, expected zero\n", ret);
> > >  todo_wine ok(GetLastError() == ERROR_INVALID_HANDLE, "got %u, expected 6\n", GetLastError());
> > > - ok(!cfi[0].nFont, "got %d, expected 0\n", cfi[0].nFont);
> > > - ok(!cfi[0].dwFontSize.X, "got %d, expected 0\n", cfi[0].dwFontSize.X);
> > > - ok(!cfi[0].dwFontSize.Y, "got %d, expected 0\n", cfi[0].dwFontSize.Y);
> >
> > Those tests do not add much value. Without initializing cfi before each call,
> > its impossible to tell if GetConsoleFontInfo() nulled the struct or if it wasn't
> > touched at all. However, I'm also not sure if we have to care about the struct
> > content in case of a failure.
>
> The values in the struct weren't altered from zero after the first failed call. But I take your point.
> I'll try memset-ing another value and checking the returned values.

I tried doing a memset with values 1, 5 and 10. Each test returned junk values.
Only a memset with zero did not return a junk value (as in the original patch).

So it definitely looks as though GetConsoleFontInfo does not null the struct on error or before
performing an operation. In this case, I'll remove those !cfi[0].element tests.