Testing redisplay code in batch

Testing redisplay code in batch

[Stefan Monnier]

I have a test for the Bug#43519 which I'd like to add to our test suite,
but it requires the redisplay to do its job, and in batch mode it seems
that Mr. Redisplay gets quite sloppy.

Does someone have experience with batch-tests of redisplay code?

So far I've only use the "let-bind execute-kbd-macro to t" trick in
order to convince minibuffer functions to actually use a minibuffer
and process events from `unread-command-events`, but here I need to go
further because I the "dummy windows" used in batch mode aren't
good enough.

So I'm ideally looking for a way to convince Emacs's internals to
perform redisplay as if there were "real" windows.  I don't need to
"see/display" the result, OTOH because I only need to test things like
`window-start` and `window-end` after calling `redisplay`.

If that can't be done without major surgery to the C code, has someone
already setup some way to run interactive tests in some virtual
interactive session (I guess using something like a detached `screen` or
`tmux`)?


        Stefan

Re: Testing redisplay code in batch

[Phil Sainty]

On 2020-09-23 15:56, Stefan Monnier wrote:
> Does someone have experience with batch-tests of redisplay code?

No, but FWIW the subject came up here:

https://lists.gnu.org/archive/html/emacs-devel/2019-10/msg00971.html

Re: Testing redisplay code in batch

[Alan Mackenzie]

Hello, Stefan.

On Tue, Sep 22, 2020 at 23:56:33 -0400, Stefan Monnier wrote:
> I have a test for the Bug#43519 which I'd like to add to our test suite,
> but it requires the redisplay to do its job, and in batch mode it seems
> that Mr. Redisplay gets quite sloppy.

> Does someone have experience with batch-tests of redisplay code?

> So far I've only use the "let-bind execute-kbd-macro to t" trick in
> order to convince minibuffer functions to actually use a minibuffer
> and process events from `unread-command-events`, but here I need to go
> further because I the "dummy windows" used in batch mode aren't
> good enough.

> So I'm ideally looking for a way to convince Emacs's internals to
> perform redisplay as if there were "real" windows.  I don't need to
> "see/display" the result, OTOH because I only need to test things like
> `window-start` and `window-end` after calling `redisplay`.

> If that can't be done without major surgery to the C code, has someone
> already setup some way to run interactive tests in some virtual
> interactive session (I guess using something like a detached `screen` or
> `tmux`)?

Have a look at .../tests/000tests.el in standalone CC Mode.  In
particular, look at this function (which has been in place for ~18
years):

(defun cc-test-force-font-lock-buffer ()
  ;; Try to forcibly font lock the current buffer, even in batch mode.
  ;; We're doing really dirty things to trick font-lock into action in
  ;; batch mode in the different emacsen.
  (let ((orig-noninteractive-function
         (and (fboundp 'noninteractive)
              (symbol-function 'noninteractive)))
        (orig-noninteractive-variable
         (and (boundp 'noninteractive)
              (symbol-value 'noninteractive)))
        ;; font-lock in XEmacs 19 looks at a variable named `noninteractive'.
        (noninteractive nil))
    (unwind-protect
        (progn
          (when orig-noninteractive-function
            ;; XEmacs (at least 21.4) calls `noninteractive' to check
            ;; for batch mode, so we let it lie.
            (fset 'noninteractive (lambda () nil)))
          (font-lock-mode 1)
          (unless (or (get-text-property (point-min) 'face)
                      (next-single-property-change (point-min) 'face))
            ;; Some emacsen have already fontified the buffer above,
            ;; but others need some more coercion..
            (let (;; Avoid getting some lazy fontification package that
                  ;; might decide that nothing should be done.
                  (font-lock-fontify-buffer-function
                   'font-lock-default-fontify-buffer))
              (font-lock-fontify-buffer))))
      (when orig-noninteractive-function
        (fset 'noninteractive orig-noninteractive-function)))))

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Testing redisplay code in batch

[Stefan Monnier]

> Have a look at .../tests/000tests.el in standalone CC Mode.  In
> particular, look at this function (which has been in place for ~18
> years):
>
> (defun cc-test-force-font-lock-buffer ()

Thanks.  But that only forces font-lock, not redisplay.
BTW, you could update your function to use `font-lock-ensure`
(in Emacs≥25).


        Stefan

Re: Testing redisplay code in batch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Tue, 22 Sep 2020 23:56:33 -0400
> 
> So I'm ideally looking for a way to convince Emacs's internals to
> perform redisplay as if there were "real" windows.  I don't need to
> "see/display" the result, OTOH because I only need to test things like
> `window-start` and `window-end` after calling `redisplay`.

I think it is best to define the capabilities required from such a
test harness.  What you say about seems to hint that you only intend
to look at stuff exposed to Lisp.  If that is indeed the case, it
could be easier to provide, but many aspects of the display engine
cannot be tested this way, since only a small fraction of that is
exposed to Lisp.

Re: Testing redisplay code in batch

[Stefan Monnier]

>> So I'm ideally looking for a way to convince Emacs's internals to
>> perform redisplay as if there were "real" windows.  I don't need to
>> "see/display" the result, OTOH because I only need to test things like
>> `window-start` and `window-end` after calling `redisplay`.
>
> I think it is best to define the capabilities required from such a
> test harness.  What you say about seems to hint that you only intend
> to look at stuff exposed to Lisp.  If that is indeed the case, it
> could be easier to provide, but many aspects of the display engine
> cannot be tested this way, since only a small fraction of that is
> exposed to Lisp.

Indeed.  In the past I was thinking (abstractly) of much more demanding
circumstances, but for this specific test, all I need is to know the
`window-start`, so the corresponding data is already exposed to Lisp.
I don't know if that makes it easy enough to implement it without
significant work.


        Stefan

Re: Testing redisplay code in batch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 23 Sep 2020 14:37:50 -0400
> 
> Indeed.  In the past I was thinking (abstractly) of much more demanding
> circumstances, but for this specific test, all I need is to know the
> `window-start`, so the corresponding data is already exposed to Lisp.
> I don't know if that makes it easy enough to implement it without
> significant work.

What about running a second instance of Emacs as a subprocess,
starting it without -batch?  On a Posix system, that would use a PTY,
which supposedly will look to the sub-Emacs as a text-mode display.
Then at least the features that don't require GUI capabilities could
be tested.  Does that make sense?

Re: Testing redisplay code in batch

[Stefan Monnier]

>> Does someone have experience with batch-tests of redisplay code?
> No, but FWIW the subject came up here:
> https://lists.gnu.org/archive/html/emacs-devel/2019-10/msg00971.html

Thanks for digging it up.

Based on the hint from Eli in it, the simple patch below was enough to
make my test "work" in batch mode!

This means: with this patch, the xdisp-tests.el succeeds or not in batch
mode depending on whether the resize_mini_window patch has been applied!

Yay!

WDYT?
Should we add a `internal--force-redisplay-in-initial-terminal` variable
(with a suitable docstring hinting at the fact that setting this to non-nil
 means you'll probably be using code paths that no-one has used before)?


        Stefan


diff --git a/src/dispnew.c b/src/dispnew.c
index d318e26308..4d585bd3e0 100644
--- a/src/dispnew.c
+++ b/src/dispnew.c
@@ -3236,9 +3236,14 @@ update_frame (struct frame *f, bool force_p, bool inhibit_hairy_id_p)
       build_frame_matrix (f);
 
       /* Update the display.  */
-      update_begin (f);
-      paused_p = update_frame_1 (f, force_p, inhibit_hairy_id_p, 1, false);
-      update_end (f);
+      if (FRAME_INITIAL_P (f))
+        paused_p = false;       /* No actual display to update!  */
+      else
+        {
+          update_begin (f);
+          paused_p = update_frame_1 (f, force_p, inhibit_hairy_id_p, 1, false);
+          update_end (f);
+        }
 
       if (FRAME_TERMCAP_P (f) || FRAME_MSDOS_P (f))
         {
diff --git a/src/frame.c b/src/frame.c
index 3f93450437..bf5f6ef910 100644
--- a/src/frame.c
+++ b/src/frame.c
@@ -931,18 +931,18 @@ make_frame (bool mini_p)
 
   wset_frame (rw, frame);
 
-  /* 10 is arbitrary,
+  /* 80/40 is arbitrary,
      just so that there is "something there."
      Correct size will be set up later with adjust_frame_size.  */
 
-  SET_FRAME_COLS (f, 10);
-  SET_FRAME_LINES (f, 10);
+  SET_FRAME_COLS (f, 80);
+  SET_FRAME_LINES (f, 40);
   SET_FRAME_WIDTH (f, FRAME_COLS (f) * FRAME_COLUMN_WIDTH (f));
   SET_FRAME_HEIGHT (f, FRAME_LINES (f) * FRAME_LINE_HEIGHT (f));
 
-  rw->total_cols = 10;
+  rw->total_cols = FRAME_COLS (f);
   rw->pixel_width = rw->total_cols * FRAME_COLUMN_WIDTH (f);
-  rw->total_lines = mini_p ? 9 : 10;
+  rw->total_lines = FRAME_LINES (f) - (mini_p ? 1 : 0);
   rw->pixel_height = rw->total_lines * FRAME_LINE_HEIGHT (f);
 
   if (mini_p)
@@ -1101,7 +1101,7 @@ make_initial_frame (void)
 
   terminal = init_initial_terminal ();
 
-  f = make_frame (1);
+  f = make_frame (true);
   XSETFRAME (frame, f);
 
   Vframe_list = Fcons (frame, Vframe_list);
@@ -1136,6 +1136,7 @@ make_initial_frame (void)
     init_frame_faces (f);
 
   last_nonminibuf_frame = f;
+  echo_area_window = f->minibuffer_window;
 
   f->can_set_window_size = true;
   f->after_make_frame = true;
diff --git a/src/xdisp.c b/src/xdisp.c
index 8fadff972f..2505fedbe8 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -11818,7 +11818,7 @@ resize_mini_window (struct window *w, bool exact_p)
 	     don't force w->start to be at the beginning of a screen
 	     line, important parts of the stuff in the mini-window,
 	     such as user prompt, will be hidden from view.  */
-	  move_it_by_lines (&it, 0);
+	  move_it_by_lines (&it, 0); /* bug#43519 */
 	  start = it.current.pos;
 	}
       else
@@ -15414,8 +15414,8 @@ redisplay_internal (void)
   /* No redisplay if running in batch mode or frame is not yet fully
      initialized, or redisplay is explicitly turned off by setting
      Vinhibit_redisplay.  */
-  if (FRAME_INITIAL_P (SELECTED_FRAME ())
-      || !NILP (Vinhibit_redisplay))
+  if (/* FRAME_INITIAL_P (SELECTED_FRAME ())
+       * || */ !NILP (Vinhibit_redisplay))
     return;
 
   /* Don't examine these until after testing Vinhibit_redisplay.
diff --git a/test/src/xdisp-tests.el b/test/src/xdisp-tests.el
index 3d0d0f5830..af46c72398 100644
--- a/test/src/xdisp-tests.el
+++ b/test/src/xdisp-tests.el
@@ -35,17 +35,18 @@ xdisp-tests--minibuffer-resizing
             (let ((ol (make-overlay (point) (point)))
                   (max-mini-window-height 1)
                   (text "askdjfhaklsjdfhlkasjdfhklasdhflkasdhflkajsdhflkashdfkljahsdlfkjahsdlfkjhasldkfhalskdjfhalskdfhlaksdhfklasdhflkasdhflkasdhflkajsdhklajsdgh"))
-             ;; (save-excursion (insert text))
-             ;; (sit-for 2)
-             ;; (delete-region (point) (point-max))
-             (put-text-property 0 1 'cursor t text)
-             (overlay-put ol 'after-string text)
-             (redisplay 'force)
-             (throw 'result
-                    ;; Make sure we do the see "hello" text.
-                    (prog1 (equal (window-start) (point-min))
-                      ;; (list (window-start) (window-end) (window-width))
-                      (delete-overlay ol)))))
+              ;; (save-excursion (insert text))
+              ;; (sit-for 2)
+              ;; (delete-region (point) (point-max))
+              (put-text-property 0 1 'cursor t text)
+              (overlay-put ol 'after-string text)
+              (let ((executing-kbd-macro nil)) ;Don't skip redisplay
+                (redisplay 'force))
+              (throw 'result
+                     ;; Make sure we do the see "hello" text.
+                     (prog1 (equal (window-start) (point-min))
+                       ;; (list (window-start) (window-end) (window-width))
+                       (delete-overlay ol)))))
         (let ((executing-kbd-macro t)) ;Force real minibuffer in `read-string'.
           (read-string "toto: ")))))))
 

Re: Testing redisplay code in batch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Wed, 23 Sep 2020 19:16:01 -0400
> Cc: emacs-devel@gnu.org
> 
> This means: with this patch, the xdisp-tests.el succeeds or not in batch
> mode depending on whether the resize_mini_window patch has been applied!
> 
> Yay!
> 
> WDYT?

Thanks.  I have a couple of minor comments:

> +      if (FRAME_INITIAL_P (f))
> +        paused_p = false;       /* No actual display to update!  */

I'd appreciate a comment here saying why setting paused_p to false is
the consequence of "no display to update".

> -  /* 10 is arbitrary,
> +  /* 80/40 is arbitrary,

Why not 80x25?  Those are the standard dimensions of old TTYs, so 25
sounds more natural to me than 40.

> @@ -1136,6 +1136,7 @@ make_initial_frame (void)
>      init_frame_faces (f);
>  
>    last_nonminibuf_frame = f;
> +  echo_area_window = f->minibuffer_window;

Why is this bit needed?

> -	  move_it_by_lines (&it, 0);
> +	  move_it_by_lines (&it, 0); /* bug#43519 */

This is unrelated (and unneeded, IMO).

> @@ -15414,8 +15414,8 @@ redisplay_internal (void)
>    /* No redisplay if running in batch mode or frame is not yet fully
>       initialized, or redisplay is explicitly turned off by setting
>       Vinhibit_redisplay.  */
> -  if (FRAME_INITIAL_P (SELECTED_FRAME ())
> -      || !NILP (Vinhibit_redisplay))
> +  if (/* FRAME_INITIAL_P (SELECTED_FRAME ())
> +       * || */ !NILP (Vinhibit_redisplay))
>      return;

This should be done cleaner, and should also update the commentary.

How about a small NEWS item about this?

Re: Testing redisplay code in batch

[Stefan Monnier]

> Thanks.  I have a couple of minor comments:

Based on your comments, I assume you consider that this feature is
acceptable for `master` once it's clean.

>> +      if (FRAME_INITIAL_P (f))
>> +        paused_p = false;       /* No actual display to update!  */
>
> I'd appreciate a comment here saying why setting paused_p to false is
> the consequence of "no display to update".

It's not.  The other branch sets `paused_p` so we need to set it
as well.  The comment explains why we don't do anything else *beside*
setting `paused_p`.
Not sure how best to explain that in the comment.  I changed it to:

        /* No actual display to update so the "update" is a nop and
           obviously isn't interrupted by pending input.  */
        paused_p = false;

>> -  /* 10 is arbitrary,
>> +  /* 80/40 is arbitrary,
> Why not 80x25?

80x25 it is, then ;-)

>> @@ -1136,6 +1136,7 @@ make_initial_frame (void)
>>      init_frame_faces (f);
>>  
>>    last_nonminibuf_frame = f;
>> +  echo_area_window = f->minibuffer_window;
>
> Why is this bit needed?

Ah, good question.
Without it I got an assertion failure in xdisp.c:18241

  if (MINI_WINDOW_P (w))
    {
      if (w == XWINDOW (echo_area_window)
	  && !NILP (echo_area_buffer[0]))

because XWINDOW complains that `echo_area_window` is not a window.

Maybe the better fix is to change `init_xdisp` so it also sets
`echo_area_window` when `noninteractive`?

>> -	  move_it_by_lines (&it, 0);
>> +	  move_it_by_lines (&it, 0); /* bug#43519 */
>
> This is unrelated (and unneeded, IMO).

Indeed, it's just a temporary hack I added so I can quickly find that
spot in the code (via `C-x v =`) when I need to (un)comment it as I test
my patch ;-)

>> @@ -15414,8 +15414,8 @@ redisplay_internal (void)
>>    /* No redisplay if running in batch mode or frame is not yet fully
>>       initialized, or redisplay is explicitly turned off by setting
>>       Vinhibit_redisplay.  */
>> -  if (FRAME_INITIAL_P (SELECTED_FRAME ())
>> -      || !NILP (Vinhibit_redisplay))
>> +  if (/* FRAME_INITIAL_P (SELECTED_FRAME ())
>> +       * || */ !NILP (Vinhibit_redisplay))
>>      return;
>
> This should be done cleaner, and should also update the commentary.

Do you think it's otherwise acceptable as-is?  I was thinking of only
allowing redisplay in `FRAME_INITIAL_P` under the control of some
special config var.

> How about a small NEWS item about this?

I'll see to it,


        Stefan

Re: Testing redisplay code in batch

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: psainty@orcon.net.nz,  emacs-devel@gnu.org
> Date: Thu, 24 Sep 2020 14:31:58 -0400
> 
> Based on your comments, I assume you consider that this feature is
> acceptable for `master` once it's clean.

Sure, why not?  It looks useful.

>         /* No actual display to update so the "update" is a nop and
>            obviously isn't interrupted by pending input.  */
>         paused_p = false;

OK.

> Without it I got an assertion failure in xdisp.c:18241
> 
>   if (MINI_WINDOW_P (w))
>     {
>       if (w == XWINDOW (echo_area_window)
> 	  && !NILP (echo_area_buffer[0]))
> 
> because XWINDOW complains that `echo_area_window` is not a window.
> 
> Maybe the better fix is to change `init_xdisp` so it also sets
> `echo_area_window` when `noninteractive`?

Yes, I think so.

> Do you think it's otherwise acceptable as-is?  I was thinking of only
> allowing redisplay in `FRAME_INITIAL_P` under the control of some
> special config var.

A command-line option, perhaps?  I agree that the "normal" -batch
should probably continue working as it does now.

And yes, I think it's okay to go in.  I bet it may not support too
many display features, but we can always extend it as we go.

> > How about a small NEWS item about this?
> 
> I'll see to it,

Thanks.