Re: [PATCH 1/2] comctl32: Added getter-setter tests for the tab control (second attempt)

[Vitaliy Margolen]

The_Hagop wrote:
> @@ -1,5 +1,6 @@
>  /* Unit test suite for tab control.
>   *
> + * Copyright 2007 Hagop Hagopian
>   * Copyright 2003 Vitaliy Margolen
>   *
>   * This library is free software; you can redistribute it and/or
> These are sorted in  historical order.
> @@ -52,6 +59,8 @@
>
> static HFONT hFont = 0;
>
> +static HWND hTab;
> +
>  static HWND
>  create_tabcontrol (DWORD style, DWORD mask)
>  {
> You using exactly the same name as parameter to all of your functions.
> This creates lots of confusion. Please remove this global variable and
> pass it as a function parameter instead.
> +    /* Creating a tooltip window*/
> +    hwndTT = CreateWindowEx(WS_EX_TOPMOST,
> +        TOOLTIPS_CLASS,
> +        NULL,
> +        WS_POPUP | TTS_NOPREFIX | TTS_ALWAYSTIP,
> +        CW_USEDEFAULT,
> +        CW_USEDEFAULT,
> +        CW_USEDEFAULT,
> +        CW_USEDEFAULT,
> +        hTab,
> +        NULL,
> +        0,
> +        NULL
> +        );
> +
> +    SetWindowPos(hwndTT,
> +        HWND_TOPMOST,
> +        0,
> +        0,
> +        0,
> +        0,
> +        SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE);
> This looks just plain bad. Please put all parameters one after another.
> There are absolutely no need to have each on it's own line.
> +    ti.rect.left = rect.left;
> +    ti.rect.top = rect.top;
> +    ti.rect.right = rect.right;
> +    ti.rect.bottom = rect.bottom;
> You can assign structures to structures in C:
> ti.rect = rect;
> +    todo_wine{
> +        /* Testing CurFocus with negative value */
> +        SendMessage(hTab, TCM_SETCURFOCUS, -1, 0);
> +        focusIndex = SendMessage(hTab, TCM_GETCURFOCUS, 0, 0);
> +        expect(-1, focusIndex);
> +    }
> Here and all other places - the only part of the test that's failing is
> "expect(,)". Add "todo_wine" at front of it only.

Also it would be interesting to test larger negative numbers, like -10.
This is because number of places use -1 to indicate that nothing is
selected. So in a sense you might be getting a correct number.
> +    expect((int) extendedStyle, (int) prevExtendedStyle);
> +    todo_wine{
> +        extendedStyle = SendMessage(hTab, TCM_GETEXTENDEDSTYLE, 0, 0);
> +        expect(TCS_EX_FLATSEPARATORS, (int) extendedStyle); 
> +    }
> Why are you casting DWORD to int?
> +static void test_getters_setters(INT nTabs)
> +{
> +    RECT rTab;
> +    INT nTabsRetrieved;
> +    INT rowCount;
> +
> +    hTab = createFilledTabControl(TCS_FIXEDWIDTH, TCIF_TEXT|TCIF_IMAGE, nTabs);
> +    ok(hTab != NULL, "Failed to create tab control\n");
> +
> +    SendMessage(hTab, TCM_SETMINTABWIDTH, 0, -1);
> +
> +    /* Testing GetItemCount */
> +    nTabsRetrieved = SendMessage(hTab, TCM_GETITEMCOUNT, 0, 0);
> +    expect(nTabs, nTabsRetrieved);
> +
> +    /* Testing GetRowCount */
> +    rowCount = SendMessage(hTab, TCM_GETROWCOUNT, 0, 0);
> +    expect(1, rowCount);
> +
> +    /* Testing GetItemRect */
> +    SendMessage(hTab, TCM_GETITEMRECT, 0 , (LPARAM) &rTab );
> +    CheckSize(hTab, TAB_DEFAULT_WIDTH, -1 , "Default Width");
> +
> +    test_getset_curFocus(hTab, nTabs);
> +    test_getset_curSel(hTab, nTabs);
> +
> +    test_getset_extendedStyle(hTab);
> +    test_getset_unicodeFormat(hTab);
> +    test_getset_item(hTab);
> +    test_getset_tooltip(hTab);
> +
> +    DestroyWindow(hTab);
> +}
> All your small functions should go inside this function. There is no
> need to create 100 small functions that do 1-3 tests.
> +
> +    test_getters_setters(5);
>  }

This part part of your patch appears to be malformed.

Vitlaiy

[PATCH 1/2] comctl32: Added getter-setter tests for the tab control (second attempt)

[Lei Zhang]

On 3/3/07, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
> The_Hagop wrote:
> > +static void test_getters_setters(INT nTabs)
> > +{
> > +    RECT rTab;
> > +    INT nTabsRetrieved;
> > +    INT rowCount;
> > +
> > +    hTab = createFilledTabControl(TCS_FIXEDWIDTH, TCIF_TEXT|TCIF_IMAGE, nTabs);
> > +    ok(hTab != NULL, "Failed to create tab control\n");
> > +
> > +    SendMessage(hTab, TCM_SETMINTABWIDTH, 0, -1);
> > +
> > +    /* Testing GetItemCount */
> > +    nTabsRetrieved = SendMessage(hTab, TCM_GETITEMCOUNT, 0, 0);
> > +    expect(nTabs, nTabsRetrieved);
> > +
> > +    /* Testing GetRowCount */
> > +    rowCount = SendMessage(hTab, TCM_GETROWCOUNT, 0, 0);
> > +    expect(1, rowCount);
> > +
> > +    /* Testing GetItemRect */
> > +    SendMessage(hTab, TCM_GETITEMRECT, 0 , (LPARAM) &rTab );
> > +    CheckSize(hTab, TAB_DEFAULT_WIDTH, -1 , "Default Width");
> > +
> > +    test_getset_curFocus(hTab, nTabs);
> > +    test_getset_curSel(hTab, nTabs);
> > +
> > +    test_getset_extendedStyle(hTab);
> > +    test_getset_unicodeFormat(hTab);
> > +    test_getset_item(hTab);
> > +    test_getset_tooltip(hTab);
> > +
> > +    DestroyWindow(hTab);
> > +}
> > All your small functions should go inside this function. There is no
> > need to create 100 small functions that do 1-3 tests.

I suggested breaking up the test into smaller functions to help
improve readability. If he rolls all the code into this function,
after applying part 2 of this patch, it would have created a 200 line
function that's harder to understand and maintain.

- Lei

[PATCH 1/2] comctl32: Added getter-setter tests for the tab control (second attempt)

[Vitaliy Margolen]

Lei Zhang wrote:
> On 3/3/07, Vitaliy Margolen <wine-devel(a)kievinfo.com> wrote:
> > The_Hagop wrote:
> > > +static void test_getters_setters(INT nTabs)
> > > +{
> > > +}
> > > All your small functions should go inside this function. There is no
> > > need to create 100 small functions that do 1-3 tests.
>
> I suggested breaking up the test into smaller functions to help
> improve readability. If he rolls all the code into this function,
> after applying part 2 of this patch, it would have created a 200 line
> function that's harder to understand and maintain.

I don't see nothing wrong with 200 lines function that does exactly the
same things over and over again. But creating 10 functions that do 5
checks each is pointless. That really makes it hard to read the test.
Also don't forget that all the tests run on the same control. So if you
change state of the control in one place, it will affect all the rest.
If you doing that, then they all should either create their own control
to test, or be all in one place.

Vitaliy