[PATCH 2/2] winetest: Implement GUI to display the results of individual tests

[Zebediah Figura]

On 12/04/2018 06:01 AM, Isira Seneviratne wrote:
> From 77b23896edc01515b3da81f475893dd309283cc3 Mon Sep 17 00:00:00 2001
> From: Isira Seneviratne 
> Date: Tue, 4 Dec 2018 17:13:47 +0530
> Subject: [PATCH 2/2] winetest: Implement GUI to display the results of
>  individual tests
>
> This is an interface to allow testers to view the results of individual
> tests, and it is displayed either after all tests have finished or the
> test running is aborted.
> Signed-off-by: Isira Seneviratne

Thanks for returning to this project; it'd be a welcome addition to 
winetest.
> programs/winetest/Makefile.in  |   1 +
>  programs/winetest/main.c       |  21 ++-
>  programs/winetest/resource.h   |   8 +
>  programs/winetest/results_ui.c | 268 +++++++++++++++++++++++++++++++++
>  programs/winetest/winetest.h   |   5 +
>  5 files changed, 300 insertions(+), 3 deletions(-)
>  create mode 100644 programs/winetest/results_ui.c
>
> diff --git a/programs/winetest/Makefile.in b/programs/winetest/Makefile.in
> index 2719a24ec2..f72ee1904d 100644
> --- a/programs/winetest/Makefile.in
> +++ b/programs/winetest/Makefile.in
> @@ -6,6 +6,7 @@ DELAYIMPORTS = ole32
>  C_SRCS = \
>    gui.c \
>    main.c \
> - results_ui.c \
> send.c \
> util.c
>
> diff --git a/programs/winetest/main.c b/programs/winetest/main.c
> index 56044095f3..6ba4c2ef08 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
>
> @@ -57,6 +58,8 @@ char *tag = NULL;
>  char *description = NULL;
>  char *url = NULL;
>  char *email = NULL;
> +char *submiturl = NULL;
> +char *logname = NULL;
>  BOOL aborting = FALSE;
>  static struct wine_test *wine_tests;
>  static int nr_of_files, nr_of_tests, nr_of_skips;
> @@ -85,6 +88,10 @@ static void (WINAPI *pReleaseActCtx)(HANDLE);
>  /* To store the current PATH setting (related to .NET only provided dlls) */
>  static char *curpath;
>
> +/* To store the result data without unnecessary additional information */
> +char result_file_path[100] = "winetest_results.txt";

This should be a temporary file, created with GetTempPath() / 
GetTempFileName().
> +static FILE *result_file;

And this should use Win32 APIs, I think, rather than POSIX. Yes, fopen() 
et al. are provided by msvcrt, but I believe we try to avoid usage of 
msvcrt in wine.

That said, do we really need to be using a file here? It's only used 
locally, for the duration that winetest is running. There's no reason we 
can't just store an array in memory instead.
> -
>
> /* check if test is being filtered out */
>  static BOOL test_filtered_out( LPCSTR module, LPCSTR testname )
>  {
> @@ -796,6 +803,7 @@ run_test (struct wine_test* test, const char* subtest, HANDLE out_file, const ch
>          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);
> -
>    fprintf (result_file, "%s %s %d\n", test->name, subtest, status);
>    if (status) failures++;
> }
>    if (failures) report (R_STATUS, "Running tests - %u failures", failures);
>
> @@ -996,7 +1004,7 @@ extract_test_proc (HMODULE hModule, LPCSTR lpszType, LPSTR lpszName, LONG_PTR lP
>      return TRUE;
>  }
>
> -static char *
> +char *
>  run_tests (char *logname, char *outdir)
>  {
>      int i;
> @@ -1061,6 +1069,9 @@ run_tests (char *logname, char *outdir)
>      else
>          strcpy( tempdir, outdir);
> -
>
> /* Stores the result data in a separate file for easy retrieval */
> -
>
> result_file = fopen(result_file_path, "w");
> -
>
> report (R_DIR, tempdir);
>
> xprintf ("Version 4\n");
>
> @@ -1157,6 +1168,10 @@ run_tests (char *logname, char *outdir)
>      heap_free(wine_tests);
>      heap_free(curpath);
> - /* Closes the result file and displays the final results in a separate window */
> - fclose(result_file);
> - displayResults(result_file_path, submiturl, logname, outdir);

This should be configurable. In particular I guess we only want to 
display it if the GUI is used, though perhaps others may differ.

I'd also propose that perhaps we should make this results UI an 
expansion of the existing GUI, rather than adding a new window for it.
> - return logname;
>
> }
>
> @@ -1239,9 +1254,9 @@ usage (void)
>  int main( int argc, char *argv[] )
>  {
>      BOOL (WINAPI *pIsWow64Process)(HANDLE hProcess, PBOOL Wow64Process);
> - char *logname = NULL, *outdir = NULL;
> - char *outdir = NULL;
> const char *extract = NULL;
> - const char *cp, *submit = NULL, *submiturl = NULL;
> - const char *cp, *submit = NULL;
> int reset_env = 1;
> int poweroff = 0;
> int interactive = 1;
>
> diff --git a/programs/winetest/resource.h b/programs/winetest/resource.h
> index be71274f58..7206322c4e 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_WIN         110
> +#define IDD_RESULT_CONTAIN  111
> -
>
> #define IDC_STATIC -1
>
> #define IDC_ST0 1000
> @@ -44,4 +48,8 @@
>  #define IDC_EDIT  4000
>  #define IDC_ABOUT 4001
>
> +#define IDC_RERUN_TESTS     5000
> +#define IDC_UPLOAD_RESULTS  5001
> +#define IDC_EXIT            5002
> -
>
> #define IDS_BUILD_ID 1
> diff --git a/programs/winetest/results_ui.c b/programs/winetest/results_ui.c
> new file mode 100644
> index 0000000000..fe68053d6c
> --- /dev/null
> +++ b/programs/winetest/results_ui.c
> @@ -0,0 +1,268 @@
> +/*
> -
> - Result GUI
> -
> -
> -
> - 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
> -
> - License as published by the Free Software Foundation; either
> -
> - version 2.1 of the License, or (at your option) any later version.
> -
> -
> -
> - This library is distributed in the hope that it will be useful,
> -
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> -
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> -
> - Lesser General Public License for more details.
> -
> -
> -
> - You should have received a copy of the GNU Lesser General Public
> -
> - License along with this library; if not, write to the Free Software
> -
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> - */
>
> +#include <string.h>
> +#include <windows.h>
> +#include <wchar.h>
> +#include "resource.h"
> +#include "winetest.h"
> -
>
> +static const char window_class_name[] = "WinetestMgmtUI";
> +static const char child_window_class_name[] = "WinetestChildContainer";
> -
>
> +HWND hMain;
> +HWND hContainer;
> -
>
> +/* Variables for reading the results */
> +char *result_file_name;
> +char *submit_url;
> +char formattedpath[200];
> -
>
> +/* Variables for rerunning the tests */
> +char *log_name;
> +char *directory;

Why are these duplicated? In fact, why are they global at all?
> -
>
> +LRESULT CALLBACK
> +WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam)
> +{
> - switch (msg)
> - {
> -
>    case WM_COMMAND:
> -
>        {
> -
>            switch (LOWORD(wParam))
> -
>            {
> -
>                case IDC_EXIT:
> -
>                    DestroyWindow(hMain);
> -
>                    PostQuitMessage(0);
> -
>                    break;
> -
> -
>                case IDC_UPLOAD_RESULTS:
> -
>                {
> -
>                    int status = send_file (submit_url, log_name);
> -
>                    if(status)
> -
>                    {
> -
>                        MessageBoxA(hMain, "Submitted results.", "Success",
> -
>                            MB_ICONINFORMATION | MB_OK);
> -
>                    }
> -
>                    else
> -
>                    {
> -
>                        MessageBoxA(hMain, "Unable to submit results.",
> -
>                            "Error", MB_ICONERROR | MB_OK);
> -
>                    }

From a UI perspective (and maybe others would disagree with me), I'm 
not sure we want this, especially in the success case; I think we should 
report success without requiring user interaction.
> -
>                }
> -
>                break;
> -
> -
>                case IDC_RERUN_TESTS:
> -
>                {
> -
>                    DestroyWindow(hMain);
> -
> -
>                    /*
> -
>                     * TODO: Rerun all available tests properly
> -
>                     * (calling the function below does not properly refresh
> -
>                     * the main Winetest UI.)
> -
>                     */
> -
>                    run_tests(log_name, directory);
> -
>                }
> -
>                break;

I'm curious what the idea behind this feature is.

Regardless of its utility, though, I wouldn't include it in this patch. 
Once you have a proper implementation, you can add it in a separate patch.
> -
>            }
> -
>        }
> -
>        break;
> -
>    case WM_CLOSE:
> -
>        PostQuitMessage(0);
> -
>        break;
> -
>    case WM_VSCROLL:
> -
>        {
> -
>            /* TODO: Handle vertical scroll events. */
> -
>        }
> -
>        break;
> -
>    default:
> -
>        return DefWindowProcA(hwnd, msg, wParam, lParam);
> - }
> - return 0;
>
> +}
> -
>
> +/* This method displays the results window. */
> +int
> +displayResults(char *resultfile, char *submiturl, char *logname, char *outdir)
> +{
> - WNDCLASSEXA wnd;
> - MSG msg;
> - wchar_t numLinesStr[10];
> - char str[100];
> - int i, numLines = 0;
> -
> - /* This variable is needed to make the font consistent with the other windows. */
> - HFONT font;
> -
> - HINSTANCE instance;
> -
> - /* Required to open the result file. */
> - FILE *result_file;
> -
> - instance = GetModuleHandleA(NULL);
> -
> - result_file_name = resultfile;
> - submit_url = submiturl;
> - log_name = logname;
> - directory = outdir;
> -
> - /* Gets the number of lines present in the results.txt file. */
> - result_file = fopen(result_file_name, "r");
> - if(result_file == NULL)
> - {
> -
>    MessageBoxA(NULL, "Unable to read the log file.", "Error",
> -
> 	MB_ICONEXCLAMATION | MB_OK);
> -
> return 0;
> - }
> - else
> - {
> -
>    while (fgets(str, 100, result_file) != NULL)
> -
>        numLines++;
> -
>    rewind(result_file);
> - }
> - swprintf(numLinesStr, 10, L"%d", numLines);
> -
> - /* Initializes and registers the parent window class */
> - wnd.cbSize = sizeof(WNDCLASSEXA);
>
> ...
> - CreateWindowExA(0, "STATIC", "Result", WS_CHILD | WS_VISIBLE | WS_BORDER,
> -
>    300, 0, 100, 20, hContainer, NULL, instance, NULL);
> -

This is an awful lot of code for a bunch of controls that don't need to 
be dynamic. I believe this can be handled much more simply by using 
control resources and dialog APIs.
> - /*
> -
> * Dynamically creates the controls required to display the results and allow
> -
> * viewing of more details.
> -
> */
> - for (i = 0; i < numLines; i++)
> - {
> -
>    int vert_pos = 20*(i+1), status_code = -1;
> -
>    char test_str[20], subtest_str[20], status[30];
> -
>    fscanf(result_file, "%s %s %d", test_str, subtest_str, &status_code);
> -
> -
>    CreateWindowExA(0, "STATIC", test_str, WS_CHILD | WS_VISIBLE | WS_BORDER,
> -
>        0, vert_pos, 100, 20, hContainer, NULL, instance, NULL);
> -
> -
>    CreateWindowExA(0, "STATIC", subtest_str, WS_CHILD | WS_VISIBLE | WS_BORDER,
> -
>        100, vert_pos, 200, 20, hContainer, NULL, instance, NULL);
> -
> -
>    if (status_code == 0)
> -
>        sprintf(status, "Success");
> -
>    else
> -
>        sprintf(status, "Failure (code %d)", status_code);
> -
> -
>    CreateWindowExA(0, "STATIC", status, WS_CHILD | WS_VISIBLE | WS_BORDER,
> -
>        300, vert_pos, 100, 20, hContainer, NULL, instance, NULL);
> - }

I'm pretty sure we've been over this before; you don't want to create 
1800 controls. I'm pretty sure we've suggested using a listview; is 
there a reason you haven't taken that suggestion?
> -
> - /* Removes the file. */
> - if (remove(result_file_name) != 0)
> - {
> -
>    MessageBoxA(NULL, "Unable to remove winetest_results.txt", "Error",
> -
>        MB_ICONERROR | MB_OK);
> - }
> -
> - /* Sets the window font to MS Sans Serif with size 8 */
> - font = CreateFontA(8, 0, 0, 0, FF_DONTCARE, FALSE, TRUE, FALSE, DEFAULT_CHARSET,
> -
>    OUT_OUTLINE_PRECIS, CLIP_DEFAULT_PRECIS, CLEARTYPE_QUALITY, VARIABLE_PITCH,
> -
>    "MS Sans Serif");
> - SendMessageA(hMain, WM_SETFONT, (WPARAM) font, MAKELPARAM(TRUE, 1));
> -
> - ShowWindow(hMain, 1);
> - UpdateWindow(hMain);
> -
> - while (GetMessageA(&msg, NULL, 0, 0) > 0)
> - {
> -
> TranslateMessage(&msg);
> -
> DispatchMessageA(&msg);
> - }
> - return msg.wParam;
>
> +}
> diff --git a/programs/winetest/winetest.h b/programs/winetest/winetest.h
> index 30e2d051e6..733c001cfb 100644
> --- a/programs/winetest/winetest.h
> +++ b/programs/winetest/winetest.h
> @@ -78,4 +78,9 @@ int guiAskTag (void);
>  int guiAskEmail (void);
>  int report (enum report_type t, ...);
>
> +int displayResults (char *resultfile, char *submiturl, char *logname, char *outdir);
> -
>
> +/* Test function (needed to rerun tests from the results GUI */
> +char* run_tests(char *logname, char *outdir);
> -
> #endif /* __WINETESTS_H */
>
> 2.19.2