[PATCH] winetest: Add GUI to display results of individual tests

[Zebediah Figura]

Sorry for the late review; I'm a little busy at the moment.

On 12/11/2018 07:52 PM, Isira Seneviratne wrote:
> From 5902ce10274900a04a740aa9a11ab91070d6e7b0 Mon Sep 17 00:00:00 2001
> From: Isira Seneviratne 
> Date: Wed, 5 Dec 2018 19:32:31 +0530
> Subject: [PATCH] winetest: Add GUI to display results of individual tests
>
> This patch adds an interface that displays the result of each test
> as soon as it completes, with the status value if the test failed.
> The dialog appears when the tests start running.
> Signed-off-by: Isira Seneviratne
>
> programs/winetest/main.c      | 78 +++++++++++++++++++++++++++++++++++
>  programs/winetest/resource.h  |  4 ++
>  programs/winetest/winetest.rc | 10 +++++
>  3 files changed, 92 insertions(+)
>
> diff --git a/programs/winetest/main.c b/programs/winetest/main.c
> index 56044095f3..9a5a069c26 100644
> --- a/programs/winetest/main.c
> +++ b/programs/winetest/main.c
> @@ -4,6 +4,7 @@
> - Copyright 2003, 2004 Jakob Eriksson   (for Solid Form Sweden AB)
> - Copyright 2003 Dimitrie O. Paun
> - Copyright 2003 Ferenc Wagner
> -
> - Copyright 2018 Isira Seneviratne
> -
> - This library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
>
> @@ -34,6 +35,7 @@
>  #include <windows.h>
>  #include <winternl.h>
>  #include <mshtml.h>
> +#include <commctrl.h>
>
> #include "winetest.h"
>  #include "resource.h"
> @@ -85,6 +87,11 @@ static void (WINAPI *pReleaseActCtx)(HANDLE);
>  /* To store the current PATH setting (related to .NET only provided

dlls) */
> static char *curpath;
>
> +/* To store the test results separately */
> +static HWND hResDlg;
> +static HWND hListView;

Please try to avoid Hungarian notation in new code.
> +static int i = 0;

'i' shouldn't be global.
> -
>
> /* check if test is being filtered out */
>  static BOOL test_filtered_out( LPCSTR module, LPCSTR testname )
>  {
> @@ -789,13 +796,45 @@ run_test (struct wine_test* test, const char*

subtest, HANDLE out_file, const ch
>  else
>  {
>      int status;
> -
>    char result[20];
>    DWORD pid, start = GetTickCount();
> -
>    LVITEMA lvItem;

Similarly here.
> -
>    char *cmd = strmake (NULL, "%s %s", test->exename, subtest);
>    report (R_STEP, "Running: %s:%s", test->name, subtest);
>    xprintf ("%s:%s start %s -\n", test->name, subtest, file);
>    status = run_ex (cmd, out_file, tempdir, 120000, &pid);
>    heap_free (cmd);
>    xprintf ("%s:%s:%04x done (%d) in %ds\n", test->name, 

subtest, pid, status, (GetTickCount()-start)/1000);
> -
> -
>    /* Stores the results separately */
> -
>    if (status)
> -
>        strcpy(result, "Success");
> -
>    else
> -
>        sprintf(result, "Failure (%d)", status);
> -

This is duplicated below.
> -
>    /* Adds a single row to the list view control (test name, 

subtest name and result). */
> -
>    lvItem.mask = LVIF_TEXT;
> -
>    lvItem.state = 0;
> -
>    lvItem.stateMask = 0;
> -
>    lvItem.iSubItem = 0;
> -
>    lvItem.iItem = i;
> -
>    lvItem.pszText = test->name;
> -
>    ListView_InsertItemA(hListView, &lvItem);
> -
> -
>    lvItem.iSubItem = 1;
> -
>    lvItem.pszText = (char *) subtest;
> -
>    ListView_SetItemA(hListView, &lvItem);
> -
> -
>    lvItem.iSubItem = 2;
> -
>    if (status == 0)
> -
>        strcpy(result, "Success");
> -
>    else
> -
>        sprintf(result, "Failure (%d)", status);

'status' here is the return value of MsgWaitForMultipleObjects(), which 
isn't very interesting to report. I'd suggest instead giving a count of 
the number of failures, similar to what test.winehq.org shows (e.g. 
[1].) Note that to do this you'll need to parse failures from the test 
output, similar to how TestLauncher does it.

[1] 
http://test.winehq.org/data/4397d9497608a3df45a5c519a37f5dcde5cc2301/index_L...
> -
>    lvItem.pszText = result;
> -
>    ListView_SetItemA(hListView, &lvItem);
> -
>    i++;
> -
>    if (status) failures++;
> }
>    if (failures) report (R_STATUS, "Running tests - %u failures",

failures);
> @@ -996,6 +1035,24 @@ extract_test_proc (HMODULE hModule, LPCSTR

lpszType, LPSTR lpszName, LONG_PTR lP
>  return TRUE;
>
> }
>
> +INT_PTR CALLBACK
> +ResultDlgProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
> +{
> - switch(msg)
> - {
> -
>    case WM_INITDIALOG:
> -
>        return TRUE;
> -
> -
>    case WM_CLOSE:
> -
>        EndDialog(hwnd, 0);
> -
>        break;
> -
> -
>    default:
> -
>        return FALSE;
> - }
> - return TRUE;
>
> +}
> -
>
> static char *
>  run_tests (char *logname, char *outdir)
>  {
> @@ -1006,6 +1063,9 @@ run_tests (char *logname, char *outdir)
>      char tmppath[MAX_PATH], tempdir[MAX_PATH+4];
>      DWORD needed;
>      HMODULE kernel32;
> -
>
> /* Variables needed to display the results of the tests */
> -
>
> LVCOLUMNA lvCol;
> -
>
> HINSTANCE hInstance = GetModuleHandleA(NULL);
>
> /* Get the current PATH only once */
> needed = GetEnvironmentVariableA("PATH", NULL, 0);
>
> @@ -1124,6 +1184,24 @@ run_tests (char *logname, char *outdir)
>      if (nr_native_dlls)
>          report( R_WARNING, "Some dlls are configured as native, you

won't be able to submit results." );
> - /* Initializes the result dialog window and the list view

control it contains */
> - hResDlg = CreateDialogParamA(hInstance,

MAKEINTRESOURCEA(IDD_RES_DLG), 0, ResultDlgProc, 0);
> -
> - hListView = GetDlgItem(hResDlg, IDD_RES_LIST);
> -
> - lvCol.mask = LVCF_FMT | LVCF_WIDTH | LVCF_TEXT | LVCF_SUBITEM;
> - lvCol.cx = 100;
> - lvCol.fmt = LVCFMT_LEFT;
> -
> - lvCol.pszText = (char *) "Test";
> - ListView_InsertColumnA(hListView, 0, &lvCol);
> -
> - lvCol.pszText = (char *) "Subtest";
> - ListView_InsertColumnA(hListView, 1, &lvCol);
> -
> - lvCol.pszText = (char *) "Result";
> - ListView_InsertColumnA(hListView, 2, &lvCol);
> - report (R_STATUS, "Running tests");
>    report (R_PROGRESS, 1, nr_of_tests);
>    for (i = 0; i < nr_of_files; i++) {
>
> diff --git a/programs/winetest/resource.h b/programs/winetest/resource.h
> index be71274f58..46525afad5 100644
> --- a/programs/winetest/resource.h
> +++ b/programs/winetest/resource.h
> @@ -2,6 +2,7 @@
> - Resource definitions
> -
> - Copyright 2004 Ferenc Wagner
> -
> - Copyright 2018 Isira Seneviratne
> -
> - This library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
>
> @@ -25,6 +26,9 @@
>  #define IDD_TAG    102
>  #define IDD_EMAIL  103
>
> +#define IDD_RES_DLG 110
> +#define IDD_RES_LIST 111
> -
>
> #define IDC_STATIC -1
>
> #define IDC_ST0 1000
> diff --git a/programs/winetest/winetest.rc

b/programs/winetest/winetest.rc
> index a3898b6ce0..673ee57843 100644
> --- a/programs/winetest/winetest.rc
> +++ b/programs/winetest/winetest.rc
> @@ -2,6 +2,7 @@
> - Winetest resources
> -
> - Copyright 2004 Ferenc Wagner
> -
> - Copyright 2018 Isira Seneviratne
> -
> - This library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
>
> @@ -88,6 +89,15 @@ BEGIN
>      DEFPUSHBUTTON "Close", IDCANCEL, 55, 40, 40, 14
>  END
>
> +IDD_RES_DLG DIALOG DISCARDABLE 160, 0, 240, 230
> +STYLE WS_VISIBLE | WS_POPUP | WS_CAPTION | WS_SYSMENU
> +CAPTION "Winetest Results"
> +FONT 8, "MS Shell Dlg"
> +BEGIN
> - CONTROL "", IDD_RES_LIST, "SysListView32", WS_CHILD | WS_VISIBLE

| WS_BORDER | LVS_REPORT, 10,
> - 10, 220, 210
>
> +END
> -

Again, I think this would make more sense as part of the existing dialog 
rather than a new one. I'm open to arguments against this, but I'd like 
to hear them first.
> /* @makedep: winetest.ico */
>  IDI_WINE ICON "winetest.ico"
>
> --
> 2.20.0