From: Mark Jamsek Subject: Re: tog test harness To: Christian Weisgerber Cc: gameoftrees@openbsd.org Date: Sun, 16 Apr 2023 18:19:39 +1000 On 23-04-15 08:12PM, Christian Weisgerber wrote: > Mark Jamsek: > > > The below diff implements the framework for tog automated tests. > > I have some comments on the tog.c changes: > > > +#define TOG_SCREEN_DUMP "SCREENDUMP" > > +#define TOG_SCREEN_DUMP_LEN (sizeof(TOG_SCREEN_DUMP) - 1) > > Why is this worth separate defines, when you use "KEY_ENTER" etc > verbatim in tog_read_script_key()? tbh, I've forgotten why I defined the screendump instruction now. I let this diff sit for about a month while busy with uni, and before that I hadn't touched it since the week after g2k22! I had a bunch of different instructions when I was working on the vi command-modesque implementation of the test harness and may have just left this around ever since. But, yes, it can go. > > +/* > > + * Extract visible substring of line y from the curses screen > > + * and strip trailing whitespace. If vline is set and locale is > > + * UTF-8, overwrite line[vline] with '|' because the ACS_VLINE > > + * character is written out as 'x'. Write the line to file f. > > + */ > > What does the locale have to do with it? If anything, it would > suggest the use of UTF-8 line-drawing characters. The line drawing > characters chosen by ncurses primarily depend on $TERM and the > termcap/terinfo entry. For VT100-compatible terminals, you'd expect > a switch to the Alternate Character Set, and 'x' which is then > rendered as a vertical line. I don't think we want to try to predict > the entire interaction between termcap/terminfo, ncurses, the > terminal, and for all I know, yes, even the locale. > > Maybe overwrite the character unconditionally? The locale is only relevant in this case because we only use the ACS_* characters when in a utf8 locale--otherwise we already draw the | and - characters so I felt like overwriting with the same character was redundant. But I also agree with you insofar as it'll simplify and remove code to just overwrite the character unconditionally. > > /* > > + * XXX Perhaps we should define "xterm" as the terminal > > + * type for standardised testing instead of using $TERM? > > + */ > > Yes, you should use a standardized $TERM. I mean what happens for > TERM=dumb? I'm not sure what a good choice is. "xterm" feels like > a moving target. I was going back and forth on this and ultimately went the other way! But I'm happy to defer to your better judgement on this; "dumb" seems to work so maybe run with that? diff /home/mark/src/got commit - 0f62ede7fdacafe2923dcbb41ba897be89dd44ba path + /home/mark/src/got blob - 9a96a68020526f3cc89eef7dcb2a5da2e100087a file + tog/tog.c --- tog/tog.c +++ tog/tog.c @@ -622,9 +622,7 @@ static int using_mock_io; } tog_io; static int using_mock_io; -#define TOG_SCREEN_DUMP "SCREENDUMP" -#define TOG_SCREEN_DUMP_LEN (sizeof(TOG_SCREEN_DUMP) - 1) -#define TOG_KEY_SCRDUMP SHRT_MIN +#define TOG_KEY_SCRDUMP SHRT_MIN /* * We implement two types of views: parent views and child views. @@ -1450,9 +1448,9 @@ strip_trailing_ws(char *str, int *n) /* * Extract visible substring of line y from the curses screen - * and strip trailing whitespace. If vline is set and locale is - * UTF-8, overwrite line[vline] with '|' because the ACS_VLINE - * character is written out as 'x'. Write the line to file f. + * and strip trailing whitespace. If vline is set, overwrite + * line[vline] with '|' because the ACS_VLINE character is + * written out as 'x'. Write the line to file f. */ static const struct got_error * view_write_line(FILE *f, int y, int vline) @@ -1471,7 +1469,7 @@ view_write_line(FILE *f, int y, int vline) */ strip_trailing_ws(line, &r); - if (vline > 0 && got_locale_is_utf8()) + if (vline > 0) line[vline] = '|'; w = fprintf(f, "%s\n", line); @@ -1521,7 +1519,7 @@ screendump(struct tog_view *view) view->child->begin_y : view->begin_y; for (i = 0; i < view->lines; i++) { - if (hline && got_locale_is_utf8() && i == hline - 1) { + if (hline && i == hline - 1) { int c; /* ACS_HLINE writes out as 'q', overwrite it */ @@ -1655,7 +1653,7 @@ tog_read_script_key(FILE *script, int *ch, int *done) *ch = KEY_DOWN; else if (strncasecmp(line, "KEY_UP", 6) == 0) *ch = KEY_UP; - else if (strncasecmp(line, TOG_SCREEN_DUMP, TOG_SCREEN_DUMP_LEN) == 0) + else if (strncasecmp(line, "SCREENDUMP", 10) == 0) *ch = TOG_KEY_SCRDUMP; else *ch = *line; @@ -4211,8 +4209,7 @@ init_mock_term(const char *test_script_path) goto done; } - /* use local TERM so we test in different environments */ - if (newterm(NULL, tog_io.cout, tog_io.cin) == NULL) + if (newterm("dumb", tog_io.cout, tog_io.cin) == NULL) err = got_error_msg(GOT_ERR_IO, "newterm: failed to initialise curses"); -- Mark Jamsek GPG: F2FF 13DE 6A06 C471 CA80 E6E2 2930 DC66 86EE CF68