[PATCH v4 8/8] oleacc: Add tests for default edit accessible object.

[Connor McAdams]

On Mon, Sep 27, 2021 at 04:10:39PM +0200, Piotr Caban wrote:
> Hi Connor,
> 
> On 9/24/21 10:53 PM, Connor McAdams wrote:
> > +typedef struct
> > +{
> > +    INT child_id;
> > +
> > +    const WCHAR *expected_name;
> > +    HRESULT expected_name_hr;
> > +
> > +    const WCHAR *expected_value;
> > +    HRESULT expected_value_hr;
> > +
> > +    const WCHAR *expected_description;
> > +    HRESULT expected_description_hr;
> > +
> > +    const WCHAR *expected_help;
> > +    HRESULT expected_help_hr;
> > +
> > +    const WCHAR *expected_kbd_shortcut;
> > +    HRESULT expected_kbd_shortcut_hr;
> > +
> > +    const WCHAR *expected_default_action;
> > +    HRESULT expected_default_action_hr;
> Do we really need all these *_hr fields? Looking on the tests the functions
> are always returning non-NULL pointer+S_OK and NULL+S_FALSE.
> 
> > +    LONG left_offset, top_offset, width_offset, height_offset;
> I bet you're adding it for future use but currently all of these fields are
> always 0. It's better to add them when they are actually used. I'm not sure
> if hard-coding exact values will work here.
> 

Yeah, I added these for future simple element tests, but they may not
work. I can remove them for now and test this later when we've got a
case of this.


> > +    /*
> > +     * Tests beyond this point are only applicable to full accessible objects
> > +     * and not simple elements.
> > +     */
> > +    if (vals->child_id != CHILDID_SELF)
> > +        return;
> The code is not used at this point. It's better to add this check while
> adding tests with different child_id.
> 
> > +static void test_default_edit_accessible_object(void)
> > +{
> > +    HWND hwnd, label0, label1, btn0, btn1;
> > +    IAccessible *acc;
> > +    HWND edit[4];
> > +    HRESULT hr;
> > +    VARIANT v;
> > +    BSTR str;
> > +
> > +    /* Create a window that looks like this:
> > +     * +----------------------------------------+
> > +     * |   ___________________________________  |
> > +     * |  |___________________________________| |
> > +     * |                (edit[0])               |
> > +     * |          ____________________________  |
> > +     * | Label0: |____________________________| |
> > +     * |(label0)       (edit[1])  (password)    |
> > +     * |          ______   _________________    |
> > +     * | Label1: |button| |multi-line edit  |   |
> > +     * |(label1)  (btn0)  |_________________|   |
> > +     * |                     (edit[2])          |
> > +     * |   __________________________________   |
> > +     * |  |                                  |  |
> > +     * |  |    edit with embedded button     |  |
> > +     * |  |__________________________________|  |
> > +     * |    (edit[3])             (btn1)        |
> > +     * +----------------------------------------+
> > +     */
> We're generally trying to avoid adding comments when code is
> self-explanatory. As far as I understand the positions are not important
> anyway so maybe it would be even better to show that in tests.
> 
> Thanks,
> Piotr

In general, do you think this style of test is too complicated? I did it
in hopes of making future tests for different control objects
cleaner/more readable, but if really long test functions are preferable,
I can do it that way. My main issue with doing long function body tests
is that after awhile I find it hard to keep track of what is being
tested.

Thanks for the review!