[PATCH 0/5] MR3492: clean up user32:msg tests
This is mostly an attempt to consolidate all the duplicated logic for matching the expected message sequence with the actual message sequence. Other improvements: * It seems that winevent_hook_todo wasn't previously reporting cases where Wine correctly sends the event. * More clarity about which type of message was received when there's a mismatch. * If a message doesn't match, and does not exist in the expected sequence, only the actual message will be skipped. Otherwise, only the expected message will be skipped. This isn't perfect, but I think it will help make the output from failures more readable. * Fixed dump_sequence not printing anything for WM_NCCALCSIZE with wParam == FALSE. Unfortunately, this is going to change the test output, so it will probably break the patterns for testbot detecting known failures. Sorry, Francois. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492
From: Esme Povirk <esme(a)codeweavers.com> --- dlls/user32/tests/msg.c | 82 +++++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 7d1f796e53f..dd4c3c8b668 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -2726,6 +2726,8 @@ static void ok_sequence_(const struct message *expected_list, const char *contex actual = sequence; + winetest_push_context("%s: %u", context, count); + while (expected->message && actual->message) { if (expected->message == actual->message && @@ -2739,16 +2741,16 @@ static void ok_sequence_(const struct message *expected_list, const char *contex failcount ++; dump++; ok_( file, line) (FALSE, - "%s: %u: in msg 0x%04x expecting wParam 0x%Ix got 0x%Ix\n", - context, count, expected->message, expected->wParam, actual->wParam); + "in msg 0x%04x expecting wParam 0x%Ix got 0x%Ix\n", + expected->message, expected->wParam, actual->wParam); } if (is_wine) goto done; } else { ok_( file, line)( ((expected->wParam ^ actual->wParam) & ~expected->wp_mask) == 0, - "%s: %u: in msg 0x%04x expecting wParam 0x%Ix got 0x%Ix\n", - context, count, expected->message, expected->wParam, actual->wParam); + "in msg 0x%04x expecting wParam 0x%Ix got 0x%Ix\n", + expected->message, expected->wParam, actual->wParam); if ((expected->wParam ^ actual->wParam) & ~expected->wp_mask) dump++; } @@ -2761,16 +2763,16 @@ static void ok_sequence_(const struct message *expected_list, const char *contex failcount ++; dump++; ok_( file, line) (FALSE, - "%s: %u: in msg 0x%04x expecting lParam 0x%Ix got 0x%Ix\n", - context, count, expected->message, expected->lParam, actual->lParam); + "in msg 0x%04x expecting lParam 0x%Ix got 0x%Ix\n", + expected->message, expected->lParam, actual->lParam); } if (is_wine) goto done; } else { ok_( file, line)(((expected->lParam ^ actual->lParam) & ~expected->lp_mask) == 0, - "%s: %u: in msg 0x%04x expecting lParam 0x%Ix got 0x%Ix\n", - context, count, expected->message, expected->lParam, actual->lParam); + "in msg 0x%04x expecting lParam 0x%Ix got 0x%Ix\n", + expected->message, expected->lParam, actual->lParam); if ((expected->lParam ^ actual->lParam) & ~expected->lp_mask) dump++; } } @@ -2780,6 +2782,10 @@ static void ok_sequence_(const struct message *expected_list, const char *contex /* don't match optional messages if their defwinproc or parent status differs */ expected++; count++; + + winetest_pop_context(); + winetest_push_context("%s: %u", context, count); + continue; } if ((expected->flags & defwinproc) != (actual->flags & defwinproc) && todo) @@ -2788,47 +2794,47 @@ static void ok_sequence_(const struct message *expected_list, const char *contex failcount ++; dump++; ok_( file, line) (FALSE, - "%s: %u: the msg 0x%04x should %shave been sent by DefWindowProc\n", - context, count, expected->message, (expected->flags & defwinproc) ? "" : "NOT "); + "the msg 0x%04x should %shave been sent by DefWindowProc\n", + expected->message, (expected->flags & defwinproc) ? "" : "NOT "); } if (is_wine) goto done; } else { ok_( file, line) ((expected->flags & defwinproc) == (actual->flags & defwinproc), - "%s: %u: the msg 0x%04x should %shave been sent by DefWindowProc\n", - context, count, expected->message, (expected->flags & defwinproc) ? "" : "NOT "); + "the msg 0x%04x should %shave been sent by DefWindowProc\n", + expected->message, (expected->flags & defwinproc) ? "" : "NOT "); if ((expected->flags & defwinproc) != (actual->flags & defwinproc)) dump++; } ok_( file, line) ((expected->flags & beginpaint) == (actual->flags & beginpaint), - "%s: %u: the msg 0x%04x should %shave been sent by BeginPaint\n", - context, count, expected->message, (expected->flags & beginpaint) ? "" : "NOT "); + "the msg 0x%04x should %shave been sent by BeginPaint\n", + expected->message, (expected->flags & beginpaint) ? "" : "NOT "); if ((expected->flags & beginpaint) != (actual->flags & beginpaint)) dump++; ok_( file, line) ((expected->flags & (sent|posted)) == (actual->flags & (sent|posted)), - "%s: %u: the msg 0x%04x should have been %s\n", - context, count, expected->message, (expected->flags & posted) ? "posted" : "sent"); + "the msg 0x%04x should have been %s\n", + expected->message, (expected->flags & posted) ? "posted" : "sent"); if ((expected->flags & (sent|posted)) != (actual->flags & (sent|posted))) dump++; ok_( file, line) ((expected->flags & parent) == (actual->flags & parent), - "%s: %u: the msg 0x%04x was expected in %s\n", - context, count, expected->message, (expected->flags & parent) ? "parent" : "child"); + "the msg 0x%04x was expected in %s\n", + expected->message, (expected->flags & parent) ? "parent" : "child"); if ((expected->flags & parent) != (actual->flags & parent)) dump++; ok_( file, line) ((expected->flags & hook) == (actual->flags & hook), - "%s: %u: the msg 0x%04x should have been sent by a hook\n", - context, count, expected->message); + "the msg 0x%04x should have been sent by a hook\n", + expected->message); if ((expected->flags & hook) != (actual->flags & hook)) dump++; ok_( file, line) ((expected->flags & winevent_hook) == (actual->flags & winevent_hook), - "%s: %u: the msg 0x%04x should have been sent by a winevent hook\n", - context, count, expected->message); + "the msg 0x%04x should have been sent by a winevent hook\n", + expected->message); if ((expected->flags & winevent_hook) != (actual->flags & winevent_hook)) dump++; ok_( file, line) ((expected->flags & kbd_hook) == (actual->flags & kbd_hook), - "%s: %u: the msg 0x%04x should have been sent by a keyboard hook\n", - context, count, expected->message); + "the msg 0x%04x should have been sent by a keyboard hook\n", + expected->message); if ((expected->flags & kbd_hook) != (actual->flags & kbd_hook)) dump++; expected++; @@ -2849,8 +2855,8 @@ static void ok_sequence_(const struct message *expected_list, const char *contex static int reported; if (!reported++) todo_wine { ok_( file, line) (FALSE, - "%s: %u: the msg 0x%04x was expected, but got msg 0x%04x instead\n", - context, count, expected->message, actual->message); + "the msg 0x%04x was expected, but got msg 0x%04x instead\n", + expected->message, actual->message); } } expected++; @@ -2860,20 +2866,23 @@ static void ok_sequence_(const struct message *expected_list, const char *contex failcount++; todo_wine { dump++; - ok_( file, line) (FALSE, "%s: %u: the msg 0x%04x was expected, but got msg 0x%04x instead\n", - context, count, expected->message, actual->message); + ok_( file, line) (FALSE, "the msg 0x%04x was expected, but got msg 0x%04x instead\n", + expected->message, actual->message); } goto done; } else { - ok_( file, line) (FALSE, "%s: %u: the msg 0x%04x was expected, but got msg 0x%04x instead\n", - context, count, expected->message, actual->message); + ok_( file, line) (FALSE, "the msg 0x%04x was expected, but got msg 0x%04x instead\n", + expected->message, actual->message); dump++; expected++; actual++; } count++; + + winetest_pop_context(); + winetest_push_context("%s: %u", context, count); } /* skip all optional trailing messages, check for winevent todo's. */ @@ -2885,8 +2894,8 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if ((expected->flags & winevent_hook_todo) && hEvent_hook) { todo_wine { - ok_( file, line) (FALSE, "%s: %u: the msg sequence is not complete: expected 0x%04x - actual 0x%04x\n", - context, count, expected->message, actual->message); + ok_( file, line) (FALSE, "the msg sequence is not complete: expected 0x%04x - actual 0x%04x\n", + expected->message, actual->message); } goto done; } @@ -2899,8 +2908,8 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if (expected->message || actual->message) { failcount++; dump++; - ok_( file, line) (FALSE, "%s: %u: the msg sequence is not complete: expected %04x - actual %04x\n", - context, count, expected->message, actual->message); + ok_( file, line) (FALSE, "the msg sequence is not complete: expected %04x - actual %04x\n", + expected->message, actual->message); } } if (is_wine) goto done; @@ -2910,8 +2919,8 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if (expected->message || actual->message) { dump++; - ok_( file, line) (FALSE, "%s: %u: the msg sequence is not complete: expected %04x - actual %04x\n", - context, count, expected->message, actual->message); + ok_( file, line) (FALSE, "the msg sequence is not complete: expected %04x - actual %04x\n", + expected->message, actual->message); } } if (todo && !failcount && !strcmp(winetest_platform, "wine")) /* succeeded yet marked todo */ @@ -2921,6 +2930,7 @@ static void ok_sequence_(const struct message *expected_list, const char *contex } done: + winetest_pop_context(); if (dump && (!is_wine || winetest_debug > 1)) dump_sequence(expected_list, context, file, line); flush_sequence(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3492
From: Esme Povirk <esme(a)codeweavers.com> --- dlls/user32/tests/msg.c | 54 ++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index dd4c3c8b668..129e718c15d 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -186,7 +186,7 @@ static const struct message WmCreateOverlappedSeq[] = { { 0x0094, sent|defwinproc|optional }, { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, /* Not sent on win10. */ { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { 0 } }; /* SetWindowPos(SWP_SHOWWINDOW|SWP_NOSIZE|SWP_NOMOVE) @@ -735,7 +735,7 @@ static const struct message WmCreateMaxPopupSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { HCBT_MINMAX, hook|lparam, 0, SW_MAXIMIZE }, @@ -779,7 +779,7 @@ static const struct message WmCreateInvisibleMaxPopupSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { HCBT_MINMAX, hook|lparam, 0, SW_MAXIMIZE }, @@ -863,7 +863,7 @@ static const struct message WmCreatePopupSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_SHOWWINDOW, sent|wparam, 1 }, @@ -1132,7 +1132,7 @@ static const struct message WmShowPopupExtremeLocationSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_SHOWWINDOW, sent|wparam, 1 }, @@ -1251,7 +1251,7 @@ static const struct message WmFirstDrawSetWindowPosSeq1[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_WINDOWPOSCHANGING, sent }, @@ -1282,7 +1282,7 @@ static const struct message WmFirstDrawSetWindowPosSeq2[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_WINDOWPOSCHANGING, sent }, @@ -1311,7 +1311,7 @@ static const struct message WmFirstDrawSetWindowPosSeq3[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, /* These happen only on Wine: */ @@ -1330,7 +1330,7 @@ static const struct message WmFirstDrawSetWindowPosSeq4[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_WINDOWPOSCHANGING, sent }, @@ -1359,7 +1359,7 @@ static const struct message WmFirstDrawSetWindowPosSeq5[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_WINDOWPOSCHANGING, sent }, @@ -1415,7 +1415,7 @@ static const struct message WmCreateMaximizedChildSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { HCBT_MINMAX, hook|lparam, 0, SW_MAXIMIZE }, @@ -1436,7 +1436,7 @@ static const struct message WmCreateVisibleChildSeq[] = { { WM_NCCALCSIZE, sent|wparam, 0 }, { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, /* Not sent on Win10. */ { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_PARENTNOTIFY, sent|parent|wparam, WM_CREATE }, @@ -2947,7 +2947,7 @@ static const struct message WmCreateMDIframeSeq[] = { { WM_NCCALCSIZE, sent|wparam, 0 }, { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, /* Not sent on Win8+. */ { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_NOTIFYFORMAT, sent|optional }, { WM_QUERYUISTATE, sent|optional }, { WM_WINDOWPOSCHANGING, sent|optional }, @@ -3004,7 +3004,7 @@ static const struct message WmCreateMDIclientSeq[] = { { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, { WM_CREATE, sent }, { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_PARENTNOTIFY, sent|wparam, WM_CREATE }, /* in MDI frame */ @@ -3051,7 +3051,7 @@ static const struct message WmCreateMDIchildVisibleSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, /* Win2k sends wparam set to @@ -3119,7 +3119,7 @@ static const struct message WmCreateMDIchildInvisibleParentSeq[] = { { WM_NCCALCSIZE, sent|wparam, 0 }, { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_PARENTNOTIFY, sent /*|wparam, WM_CREATE*/ }, /* in MDI client */ @@ -3221,7 +3221,7 @@ static const struct message WmCreateMDIchildInvisibleSeq[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, /* Win2k sends wparam set to @@ -3498,7 +3498,7 @@ static const struct message WmCreateMDIchildInvisibleMaxSeq4[] = { { WM_NCCALCSIZE, sent|wparam, 0 }, { EVENT_OBJECT_REORDER, winevent_hook|wparam|lparam|optional, 0, 0 }, /* Not sent on Win8+. */ { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_FRAMECHANGED|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE, 0, SWP_NOZORDER }, /* MDI frame */ { WM_NCCALCSIZE, sent|wparam|optional, 1 }, /* MDI frame */ @@ -13984,7 +13984,7 @@ static INT_PTR CALLBACK wm_quit_dlg_proc(HWND hwnd, UINT message, WPARAM wp, LPA static const struct message WmQuitDialogSeq[] = { { HCBT_CREATEWND, hook }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SETFONT, sent }, { WM_INITDIALOG, sent }, { WM_CHANGEUISTATE, sent|optional }, @@ -15177,7 +15177,7 @@ static const struct message WmCreateDialogParamSeq_0[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_SETFONT, sent }, @@ -15191,7 +15191,7 @@ static const struct message WmCreateDialogParamSeq_1[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_SETFONT, sent }, @@ -15221,7 +15221,7 @@ static const struct message WmCreateDialogParamSeq_2[] = { { WM_NCCREATE, sent }, { WM_NCCALCSIZE, sent|wparam, 0 }, { WM_CREATE, sent }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { WM_SIZE, sent|wparam, SIZE_RESTORED }, { WM_MOVE, sent }, { WM_CHANGEUISTATE, sent|optional }, @@ -16642,7 +16642,7 @@ static const struct message wm_popup_menu_1[] = { WM_MENUSELECT, sent|wparam, MAKEWPARAM(1,MF_HILITE|MF_POPUP) }, { WM_INITMENUPOPUP, sent|lparam, 0, 1 }, { HCBT_CREATEWND, hook|optional }, /* Win9x doesn't create a window */ - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, { EVENT_OBJECT_SHOW, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, @@ -16680,7 +16680,7 @@ static const struct message wm_popup_menu_2[] = { WM_MENUSELECT, sent|wparam|optional, MAKEWPARAM(0,MF_HILITE|MF_POPUP) }, /* Win9x */ { WM_INITMENUPOPUP, sent|lparam|optional, 0, 0 }, /* Win9x */ { HCBT_CREATEWND, hook }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, { EVENT_OBJECT_SHOW, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, @@ -16693,7 +16693,7 @@ static const struct message wm_popup_menu_2[] = { HCBT_KEYSKIPPED, hook|wparam|lparam|optional, VK_RIGHT, 0x10000001 }, { WM_INITMENUPOPUP, sent|lparam|optional, 0, 0 }, /* Win9x doesn't send it */ { HCBT_CREATEWND, hook|optional }, /* Win9x doesn't send it */ - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, { EVENT_OBJECT_SHOW, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, @@ -16735,7 +16735,7 @@ static const struct message wm_popup_menu_3[] = { WM_MENUSELECT, sent|wparam|optional, MAKEWPARAM(0,MF_HILITE|MF_POPUP) }, /* Win9x */ { WM_INITMENUPOPUP, sent|lparam|optional, 0, 0 }, /* Win9x */ { HCBT_CREATEWND, hook }, - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, { EVENT_OBJECT_SHOW, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, @@ -16748,7 +16748,7 @@ static const struct message wm_popup_menu_3[] = { HCBT_KEYSKIPPED, hook|wparam|lparam|optional, VK_RIGHT, 0x10000001 }, { WM_INITMENUPOPUP, sent|lparam|optional, 0, 0 }, /* Win9x doesn't send it */ { HCBT_CREATEWND, hook|optional }, /* Win9x doesn't send it */ - { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, + { EVENT_OBJECT_CREATE, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, { EVENT_OBJECT_SHOW, winevent_hook|wparam|lparam, 0, 0 }, { EVENT_OBJECT_LOCATIONCHANGE, winevent_hook|wparam|lparam|winevent_hook_todo, 0, 0 }, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3492
From: Esme Povirk <esme(a)codeweavers.com> --- dlls/user32/tests/msg.c | 192 +++++++++++++++++----------------------- 1 file changed, 83 insertions(+), 109 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 129e718c15d..51046a99cf0 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -2627,6 +2627,71 @@ static void flush_sequence(void) LeaveCriticalSection( &sequence_cs ); } +static const char* message_type_name(int flags) { + if (flags & hook) return "hook"; + if (flags & kbd_hook) return "kbd_hook"; + if (flags & winevent_hook) return "winevent_hook"; + return "msg"; +} + +static BOOL can_skip_message(const struct message *expected) +{ + if (expected->flags & optional) return TRUE; + + if ((expected->flags & winevent_hook) && !hEvent_hook) return TRUE; + if ((expected->flags & hook) && !hCBT_hook) return TRUE; + + if ((expected->flags & winevent_hook_todo) && !strcmp(winetest_platform, "wine")) return TRUE; + + return FALSE; +} + +static BOOL messages_equal(const struct message *expected, const struct recvd_message *actual, + BOOL expect_equal, const char* file, int line) +{ + int todo = (expected->flags & winevent_hook_todo) != 0; + const int message_type_flags = hook|winevent_hook|kbd_hook; + static int todo_reported; + + if (!todo && can_skip_message(expected)) + expect_equal = FALSE; + + if (!expected->message || !actual->message) { + if (expect_equal && (!todo || !todo_reported++)) + todo_wine_if(todo) + ok_( file, line) (FALSE, "the msg sequence is not complete: expected %s %04x - actual %s %04x\n", + message_type_name(expected->flags), expected->message, message_type_name(actual->flags), actual->message); + return FALSE; + } + + if (expected->message != actual->message || + (expected->flags & message_type_flags) != (actual->flags & message_type_flags)) + { + if (expect_equal && (!todo || !todo_reported++)) + todo_wine_if(todo) + ok_( file, line) (FALSE, "the %s 0x%04x was expected, but got %s 0x%04x instead\n", + message_type_name(expected->flags), expected->message, message_type_name(actual->flags), actual->message); + return FALSE; + } + + if (expected->flags & optional) + { + /* If a message can be sent in 2 different ways at the same time, we may need to treat + * them as unequal so that the optional message can be properly skipped. */ + if ((expected->flags & defwinproc) != (actual->flags & defwinproc)) { + /* don't match messages if their defwinproc status differs */ + return FALSE; + } + } + + if (expect_equal) + todo_wine_if(todo) + ok_( file, line) (TRUE, "got %s 0x%04x as expected\n", + message_type_name(expected->flags), expected->message); + + return TRUE; +} + static void dump_sequence(const struct message *expected, const char *context, const char *file, int line) { const struct recvd_message *actual = sequence; @@ -2637,46 +2702,15 @@ static void dump_sequence(const struct message *expected, const char *context, c { if (actual->output[0]) { - if (expected->flags & hook) - { - trace_(file, line)( " %u: expected: hook %04x - actual: %s\n", - count, expected->message, actual->output ); - } - else if (expected->flags & winevent_hook) - { - trace_(file, line)( " %u: expected: winevent %04x - actual: %s\n", - count, expected->message, actual->output ); - } - else if (expected->flags & kbd_hook) - { - trace_(file, line)( " %u: expected: kbd %04x - actual: %s\n", - count, expected->message, actual->output ); - } - else - { - trace_(file, line)( " %u: expected: msg %04x - actual: %s\n", - count, expected->message, actual->output ); - } + trace_(file, line)( " %u: expected: %s %04x - actual: %s\n", + count, message_type_name(expected->flags), expected->message, actual->output ); } - if (expected->message == actual->message) + if (!messages_equal(expected, actual, FALSE, file, line) && + can_skip_message(expected)) { - if ((expected->flags & defwinproc) != (actual->flags & defwinproc) && - (expected->flags & optional)) - { - /* don't match messages if their defwinproc status differs */ - expected++; - } - else - { - expected++; - actual++; - } - } - /* silently drop winevent messages if there is no support for them */ - else if ((expected->flags & optional) || ((expected->flags & winevent_hook) && !hEvent_hook) || - ((expected->flags & winevent_hook_todo) && !strcmp(winetest_platform, "wine"))) - expected++; + expected++; + } else { expected++; @@ -2686,8 +2720,7 @@ static void dump_sequence(const struct message *expected, const char *context, c } /* optional trailing messages */ - while (expected->message && ((expected->flags & optional) || - ((expected->flags & winevent_hook) && !hEvent_hook))) + while (can_skip_message(expected)) { trace_(file, line)( " %u: expected: msg %04x - actual: nothing\n", count, expected->message ); expected++; @@ -2730,8 +2763,7 @@ static void ok_sequence_(const struct message *expected_list, const char *contex while (expected->message && actual->message) { - if (expected->message == actual->message && - !((expected->flags ^ actual->flags) & (hook|winevent_hook|kbd_hook))) + if (messages_equal(expected, actual, !todo, file, line)) { if (expected->flags & wparam) { @@ -2776,18 +2808,6 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if ((expected->lParam ^ actual->lParam) & ~expected->lp_mask) dump++; } } - if ((expected->flags & optional) && - ((expected->flags ^ actual->flags) & (defwinproc|parent))) - { - /* don't match optional messages if their defwinproc or parent status differs */ - expected++; - count++; - - winetest_pop_context(); - winetest_push_context("%s: %u", context, count); - - continue; - } if ((expected->flags & defwinproc) != (actual->flags & defwinproc) && todo) { todo_wine { @@ -2822,59 +2842,25 @@ static void ok_sequence_(const struct message *expected_list, const char *contex expected->message, (expected->flags & parent) ? "parent" : "child"); if ((expected->flags & parent) != (actual->flags & parent)) dump++; - ok_( file, line) ((expected->flags & hook) == (actual->flags & hook), - "the msg 0x%04x should have been sent by a hook\n", - expected->message); - if ((expected->flags & hook) != (actual->flags & hook)) dump++; - - ok_( file, line) ((expected->flags & winevent_hook) == (actual->flags & winevent_hook), - "the msg 0x%04x should have been sent by a winevent hook\n", - expected->message); - if ((expected->flags & winevent_hook) != (actual->flags & winevent_hook)) dump++; - - ok_( file, line) ((expected->flags & kbd_hook) == (actual->flags & kbd_hook), - "the msg 0x%04x should have been sent by a keyboard hook\n", - expected->message); - if ((expected->flags & kbd_hook) != (actual->flags & kbd_hook)) dump++; - expected++; actual++; } /* - * silently drop hook messages if there is no support for them, mark - * winevent todo's. + * silently drop hook messages if there is no support for them */ - else if ((expected->flags & optional) || - ((expected->flags & hook) && !hCBT_hook) || - ((expected->flags & winevent_hook) && !hEvent_hook) || - ((expected->flags & kbd_hook) && !hKBD_hook) || - ((expected->flags & winevent_hook_todo) && is_wine)) + else if (can_skip_message(expected)) { - if ((expected->flags & winevent_hook_todo) && hEvent_hook) - { - static int reported; - if (!reported++) todo_wine { - ok_( file, line) (FALSE, - "the msg 0x%04x was expected, but got msg 0x%04x instead\n", - expected->message, actual->message); - } - } expected++; } else if (todo) { + todo_wine messages_equal(expected, actual, TRUE, file, line); failcount++; - todo_wine { - dump++; - ok_( file, line) (FALSE, "the msg 0x%04x was expected, but got msg 0x%04x instead\n", - expected->message, actual->message); - } + dump++; goto done; } else { - ok_( file, line) (FALSE, "the msg 0x%04x was expected, but got msg 0x%04x instead\n", - expected->message, actual->message); dump++; expected++; actual++; @@ -2885,20 +2871,10 @@ static void ok_sequence_(const struct message *expected_list, const char *contex winetest_push_context("%s: %u", context, count); } - /* skip all optional trailing messages, check for winevent todo's. */ - while (expected->message && ((expected->flags & optional) || - ((expected->flags & hook) && !hCBT_hook) || - ((expected->flags & winevent_hook) && !hEvent_hook) || - ((expected->flags & winevent_hook_todo) && is_wine))) + /* skip all optional trailing messages */ + while (can_skip_message(expected)) { - if ((expected->flags & winevent_hook_todo) && hEvent_hook) - { - todo_wine { - ok_( file, line) (FALSE, "the msg sequence is not complete: expected 0x%04x - actual 0x%04x\n", - expected->message, actual->message); - } - goto done; - } + messages_equal(expected, actual, TRUE, file, line); /* check for message todo's */ expected++; } @@ -2908,8 +2884,7 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if (expected->message || actual->message) { failcount++; dump++; - ok_( file, line) (FALSE, "the msg sequence is not complete: expected %04x - actual %04x\n", - expected->message, actual->message); + messages_equal(expected, actual, TRUE, file, line); } } if (is_wine) goto done; @@ -2919,14 +2894,13 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if (expected->message || actual->message) { dump++; - ok_( file, line) (FALSE, "the msg sequence is not complete: expected %04x - actual %04x\n", - expected->message, actual->message); + messages_equal(expected, actual, TRUE, file, line); } } if (todo && !failcount && !strcmp(winetest_platform, "wine")) /* succeeded yet marked todo */ todo_wine { dump++; - ok_( file, line)( TRUE, "%s: marked \"todo_wine\" but succeeds\n", context); + ok_( file, line)( TRUE, "marked \"todo_wine\" but succeeds\n"); } done: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3492
From: Esme Povirk <esme(a)codeweavers.com> This may not be very clever, but it should make the output much more readable when sequences fail. --- dlls/user32/tests/msg.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 51046a99cf0..495e1ca0f1e 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -2692,6 +2692,17 @@ static BOOL messages_equal(const struct message *expected, const struct recvd_me return TRUE; } +static BOOL sequence_contains_message(const struct message *expected, const struct recvd_message *actual) +{ + while (expected->message) + { + if (messages_equal(expected, actual, FALSE, __FILE__, __LINE__)) + return TRUE; + expected++; + } + return FALSE; +} + static void dump_sequence(const struct message *expected, const char *context, const char *file, int line) { const struct recvd_message *actual = sequence; @@ -2706,17 +2717,21 @@ static void dump_sequence(const struct message *expected, const char *context, c count, message_type_name(expected->flags), expected->message, actual->output ); } - if (!messages_equal(expected, actual, FALSE, file, line) && - can_skip_message(expected)) + if (messages_equal(expected, actual, FALSE, file, line)) + { + expected++; + actual++; + count++; + } + else if (can_skip_message(expected) || sequence_contains_message(expected, actual)) { expected++; + count++; } else { - expected++; actual++; } - count++; } /* optional trailing messages */ @@ -2843,6 +2858,7 @@ static void ok_sequence_(const struct message *expected_list, const char *contex if ((expected->flags & parent) != (actual->flags & parent)) dump++; expected++; + count++; actual++; } /* @@ -2851,6 +2867,7 @@ static void ok_sequence_(const struct message *expected_list, const char *contex else if (can_skip_message(expected)) { expected++; + count++; } else if (todo) { @@ -2859,13 +2876,17 @@ static void ok_sequence_(const struct message *expected_list, const char *contex dump++; goto done; } - else + else if (sequence_contains_message(expected, actual)) { dump++; expected++; + count++; + } + else + { + dump++; actual++; } - count++; winetest_pop_context(); winetest_push_context("%s: %u", context, count); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3492
From: Esme Povirk <esme(a)codeweavers.com> It's confusing to look at a dumped sequence and see empty lines. --- dlls/user32/tests/msg.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 495e1ca0f1e..f45f2131710 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -2505,6 +2505,11 @@ static void add_message_(int line, const struct recvd_message *msg) } else { + RECT *r = (RECT*)msg->lParam; + + sprintf(seq->output, "%s: %p WM_NCCALCSIZE: (%d,%d)-(%d,%d)", + msg->descr, msg->hwnd, + (int)r->left, (int)r->top, (int)r->right, (int)r->bottom); seq->lParam = 0; } break; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3492
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135580 Your paranoid android. === w10pro64 (32 bit report) === user32: msg.c:18181: Test failed: SetParent() visible WS_POPUP: 27: the msg sequence is not complete: expected msg 0000 - actual msg 0088 === debian11 (32 bit zh:CN report) === user32: msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got hook 0x0005 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x030f instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x001c instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x0086 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x0006 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got hook 0x0009 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 should NOT have been sent by DefWindowProc msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected in child msg.c:6908: Test failed: SetFocus(hwnd) on a button: 5: the msg 0x0138 was expected, but got msg 0x0008 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 5: the msg 0x0138 was expected, but got winevent_hook 0x8005 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 5: the msg 0x0138 was expected, but got msg 0x0007 instead
On Wed Aug 2 20:15:28 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=135580 Your paranoid android. === w10pro64 (32 bit report) === user32: msg.c:18181: Test failed: SetParent() visible WS_POPUP: 27: the msg sequence is not complete: expected msg 0000 - actual msg 0088 === debian11 (32 bit zh:CN report) === user32: msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got hook 0x0005 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x030f instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x001c instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x0086 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got msg 0x0006 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected, but got hook 0x0009 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 should NOT have been sent by DefWindowProc msg.c:6908: Test failed: SetFocus(hwnd) on a button: 4: the msg 0x0007 was expected in child msg.c:6908: Test failed: SetFocus(hwnd) on a button: 5: the msg 0x0138 was expected, but got msg 0x0008 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 5: the msg 0x0138 was expected, but got winevent_hook 0x8005 instead msg.c:6908: Test failed: SetFocus(hwnd) on a button: 5: the msg 0x0138 was expected, but got msg 0x0007 instead ``` The w10pro64 failure doesn't seem to be on the failures list. 0088 is WM_SYNCPAINT. I don't remember this message, but looking at MSDN it sounds like the sort of thing that other random processes could trigger? Anyway, I don't think I could have caused that.
The failure on debian looks like bug 55066. We can now see that the 0x0005 here isn't WM_SIZE at all but HCBT_ACTIVATE. So it seems the window is being activated (and therefore the focus isn't where we expect it? I'm not entirely clear on what the expectations are for this test). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41124
Rémi Bernon (@rbernon) commented about dlls/user32/tests/msg.c:
LeaveCriticalSection( &sequence_cs ); }
-static void dump_sequence(const struct message *expected, const char *context, const char *file, int line) -{ - const struct recvd_message *actual = sequence; - unsigned int count = 0; +static const char* message_type_name(int flags) {
I think it'd be nice to fix the style at the same time, to match the general user32 style. ```suggestion:-0+0 static const char* message_type_name( int flags ) { ``` Similar changes pretty much everywhere below. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41321
Rémi Bernon (@rbernon) commented about dlls/user32/tests/msg.c:
+ if ((expected->flags & hook) && !hCBT_hook) return TRUE; + + if ((expected->flags & winevent_hook_todo) && !strcmp(winetest_platform, "wine")) return TRUE; + + return FALSE; } - else if (expected->flags & winevent_hook) + +static BOOL messages_equal(const struct message *expected, const struct recvd_message *actual, + BOOL expect_equal, const char* file, int line) { - trace_(file, line)( " %u: expected: winevent %04x - actual: %s\n", - count, expected->message, actual->output ); + int todo = (expected->flags & winevent_hook_todo) != 0; + const int message_type_flags = hook|winevent_hook|kbd_hook; + static int todo_reported; I'm not sure we need a throttle as there's already one builtin in todo_wine reports?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41322
Rémi Bernon (@rbernon) commented about dlls/user32/tests/msg.c:
if (expected->message || actual->message) { dump++; - ok_( file, line) (FALSE, "the msg sequence is not complete: expected %04x - actual %04x\n", - expected->message, actual->message); + messages_equal(expected, actual, TRUE, file, line); } } if (todo && !failcount && !strcmp(winetest_platform, "wine")) /* succeeded yet marked todo */ todo_wine { dump++; - ok_( file, line)( TRUE, "%s: marked \"todo_wine\" but succeeds\n", context); + ok_( file, line)( TRUE, "marked \"todo_wine\" but succeeds\n");
Should be in the previous change probably. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41323
Rémi Bernon (@rbernon) commented about dlls/user32/tests/msg.c:
} else { + RECT *r = (RECT*)msg->lParam; + + sprintf(seq->output, "%s: %p WM_NCCALCSIZE: (%d,%d)-(%d,%d)", + msg->descr, msg->hwnd, + (int)r->left, (int)r->top, (int)r->right, (int)r->bottom);
```suggestion:-2+0 RECT *rect = (RECT *)msg->lParam; sprintf( seq->output, "%s: %p WM_NCCALCSIZE: %s", msg->descr, msg->hwnd, wine_dbgstr_rect( rect ) ); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41324
Rémi Bernon (@rbernon) commented about dlls/user32/tests/msg.c:
if ((expected->lParam ^ actual->lParam) & ~expected->lp_mask) dump++; } } - if ((expected->flags & optional) && - ((expected->flags ^ actual->flags) & (defwinproc|parent))) - {
We were previously reaching this case when the messages or flags differ, without printing an error. With this change, I think we'd be getting a test failure from messages_equal before checking the optional / defwinproc flag? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41325
Fwiw I've never been really satisfied with how the dumped sequences look like, it is often hard to tell the actual sequence vs the expected sequence from looking at the diff-like output dump_sequence prints. I think maybe the code should not be so smart and instead print the full expected / actual sequences (or maybe only the actual sequence if it's too long) and let us decide how to match it. This is just a thought, I'm not completely sure about it either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41326
If a message doesn't match, and does not exist in the expected sequence, only the actual message will be skipped. Otherwise, only the expected message will be skipped. This isn't perfect, but I think it will help make the output from failures more readable.
It might sound a bit complicated, but I think it would be better to actually compute the longest common subsequence (like diff) than add more heuristics. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41385
On Fri Aug 4 14:58:57 2023 +0000, Jinoh Kang wrote:
If a message doesn't match, and does not exist in the expected sequence, only the actual message will be skipped. Otherwise, only the expected message will be skipped. This isn't perfect, but I think it will help make the output from failures more readable. It might sound a bit complicated, but I think it would be better to actually compute the longest common subsequence (like diff) than add more heuristics. Fwiw I had done something like this at some point (https://www.winehq.org/pipermail/wine-devel/2022-January/206255.html), reporting the smallest mismatch.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41386
On Fri Aug 4 10:03:22 2023 +0000, Rémi Bernon wrote:
I'm not sure we need a throttle as there's already one builtin in todo_wine reports? I did this to match the existing behavior. I'd prefer to report all the todo's, but apparently that makes the test output too heavy and breaks things.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41401
On Fri Aug 4 10:03:24 2023 +0000, Rémi Bernon wrote:
We were previously reaching this case when the messages or flags differ, without printing an error. With this change, I think we'd be getting a test failure from messages_equal before checking the optional / defwinproc flag? messages_equal should return FALSE without reporting a test failure in this case. In fact, messages_equal should never report an error for a test with the `optional` flag.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41402
I would suggest looking at the testbot output of debian11 here: https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41119 This isn't perfect (the child WM_SETFOCUS was incorrectly matched with a parent WM_SETFOCUS), but it still makes it very easy to see the list of "extra" messages in the sequence. This is a vast improvement over the current system which is practically guaranteed to desync after a single mismatch, and it would save me a lot of time analyzing test failures. (I will also point out: I had no way of knowing it would fail in this way, and it still worked even though I didn't get to choose the example.) I don't think a minimal diff is worth the complexity it would add to the code, because in most cases the actual sequence should only differ by the addition or removal of a single contiguous block of messages. Anyway, I'm not willing to do that work. If a heuristic is not acceptable, then we should stop checking after a single mismatch and just print the actual sequence (without a side-by-side comparison, I don't see the value in printing the expected sequence). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41403
On Mon Aug 7 16:17:10 2023 +0000, Esme Povirk wrote:
I would suggest looking at the testbot output of debian11 here: https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41119 This isn't perfect (the child WM_SETFOCUS was incorrectly matched with a parent WM_SETFOCUS), but it still makes it very easy to see the list of "extra" messages in the sequence. This is a vast improvement over the current system which is practically guaranteed to desync after a single mismatch, and it would save me a lot of time analyzing test failures. (I will also point out: I had no way of knowing it would fail in this way, and it still worked even though I didn't get to choose the example.) I don't think a minimal diff is worth the complexity it would add to the code, because in most cases the actual sequence should only differ by the addition or removal of a single contiguous block of messages. Anyway, I'm not willing to do that work. If a heuristic is not acceptable, then we should stop checking after a single mismatch and just print the actual sequence (without a side-by-side comparison, I don't see the value in printing the expected sequence). I'm not completely convinced that it's much better than what we have. The output doesn't make it obvious that the message 0007 has actually been received for instance, you have to deduce it from the increased message number (and here from the mismatched flags). It also doesn't make it easy to get the full sequence that was received, if you want to do a better message matching yourself.
I think it might just be easier to print the received sequence whenever there's a mismatch. Reducing the verbosity at the same time to avoid too much output might also be a good thing (for instance do we really need "`SetFocus(hwnd) on a button:`" on each line?). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41506
On Mon Aug 7 16:17:10 2023 +0000, Rémi Bernon wrote:
I'm not completely convinced that it's much better than what we have. The output doesn't make it obvious that the message 0007 has actually been received for instance, you have to deduce it from the increased message number (and here from the mismatched flags). It also doesn't make it easy to get the full sequence that was received, if you want to do a better message matching yourself. I think it might just be easier to print the received sequence whenever there's a mismatch. Reducing the verbosity at the same time to avoid too much output might also be a good thing (for instance do we really need "`SetFocus(hwnd) on a button:`" on each line?). If the message 0007 was not received, it would tell you the message was not received.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41508
On Mon Aug 7 16:42:55 2023 +0000, Esme Povirk wrote:
If the message 0007 was not received, it would tell you the message was not received. I could see an argument, separately, for making `dump_sequence` print only the actual messages, since the expected messages are in the test itself, and doing so for any non-todo Wine tests. But given that I plan on spending time with these tests, I don't want to manually compare sequences if I don't have to.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41511
On Mon Aug 7 17:03:28 2023 +0000, Esme Povirk wrote:
I could see an argument, separately, for making `dump_sequence` print only the actual messages, since the expected messages are in the test itself, and doing so for any non-todo Wine tests. But given that I plan on spending time with these tests, I don't want to manually compare sequences if I don't have to. If all you get is a dumped message sequence, you have to manually compare everything (at least starting from the first expected message that failed, and often starting with the optional messages before that). This includes looking up different types of message constants for every message. And, not being a computer, it's easy to make mistakes or lose your place.
Maybe for some reason I don't understand, it's easier for you, but *for me* this makes understanding what happened much easier. Assuming it doesn't actively make things more difficult for you (and I don't see why it would - it's not mutually exclusive with the other changes you want), or significantly increase the code complexity (again, I don't think it does), isn't that enough for it to be worth having? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41514
On Mon Aug 7 17:49:57 2023 +0000, Esme Povirk wrote:
If all you get is a dumped message sequence, you have to manually compare everything (at least starting from the first expected message that failed, and often starting with the optional messages before that). This includes looking up different types of message constants for every message. And, not being a computer, it's easy to make mistakes or lose your place. Maybe for some reason I don't understand, it's easier for you, but *for me* this makes understanding what happened much easier. Assuming it doesn't actively make things more difficult for you (and I don't see why it would - it's not mutually exclusive with the other changes you want), or significantly increase the code complexity (again, I don't think it does), isn't that enough for it to be worth having? Here's another alternative I'd be OK with: If any mismatch is found, calculate the common prefix and common suffix. Dump the lengths of those, along with the actual messages in between, and don't try to compare anything in the middle. For only the common messages (prefix/suffix or just the full thing), do a more detailed comparison. This would be a step towards LCS, but it wouldn't have nearly the complexity.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41517
On Mon Aug 7 19:03:50 2023 +0000, Esme Povirk wrote:
Here's another alternative I'd be OK with: If any mismatch is found, calculate the common prefix and common suffix. Dump the lengths of those, along with the actual messages in between, and don't try to compare anything in the middle. For only the common messages (prefix/suffix or just the full thing), do a more detailed comparison. This would be a step towards LCS, but it wouldn't have nearly the complexity. My idea was that with full sequences printed out, and because you won't get all the possible sequences at once, you could maybe gather them across the various runs, check and compare the failing sequences to figure some common patterns. Having only partial diff, it makes it more difficult.
I don't have a strong opinion there, I think these messages tests have a deeper flaw anyway, as for instance there's way too many optional messages. Some sequences are pretty much entirely optional with only a few percent of actually expected messages, I don't see how this can verify anything useful. Imo the data-driven tests here are showing their limitation (or the data description we use is too limited). I'm not really working on that area so I'll leave this for @julliard to decide. I think your proposal is fine, and I was only trying to explore a different approach. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_41608
On Fri Aug 11 19:10:06 2023 +0000, Rémi Bernon wrote:
I think it'd be nice to fix the style at the same time, to match the general user32 style. ```suggestion:-0+0 static const char* message_type_name( int flags ) { ``` Similar changes pretty much everywhere below. Looking at the rest of the file I barely see that style anywhere, so I'd be making it less consistent.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_42029
On Fri Aug 4 14:58:57 2023 +0000, Rémi Bernon wrote:
Fwiw I had done something like this at some point (https://www.winehq.org/pipermail/wine-devel/2022-January/206255.html), reporting the smallest mismatch. Resolving this thread because there's another with the same topic.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3492#note_42030
participants (5)
-
Esme Povirk -
Esme Povirk (＠madewokherd)
-
Jinoh Kang (＠iamahuman)
-
Marvin -
Rémi Bernon