Re: master updated (5af5ed6c62 -> f07505d1ec)

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

After this patch series, I'm getting:

4 unexpected results:
   FAILED  esh-proc-test/exit-status/with-stderr-pipe
   FAILED  esh-proc-test/output/stderr-to-buffer
   FAILED  esh-proc-test/output/stdout-and-stderr-to-buffer
   FAILED  esh-proc-test/output/stdout-to-buffer

This is on Debian/bookworm.

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> After this patch series, I'm getting:
>
> 4 unexpected results:
>    FAILED  esh-proc-test/exit-status/with-stderr-pipe
>    FAILED  esh-proc-test/output/stderr-to-buffer
>    FAILED  esh-proc-test/output/stdout-and-stderr-to-buffer
>    FAILED  esh-proc-test/output/stdout-to-buffer
>
> This is on Debian/bookworm.

(To clarify -- I didn't check that this patch series was the actual
culprit, but it seemed like the most likely change.)

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> After this patch series, I'm getting:
>
> 4 unexpected results:
>    FAILED  esh-proc-test/exit-status/with-stderr-pipe
>    FAILED  esh-proc-test/output/stderr-to-buffer
>    FAILED  esh-proc-test/output/stdout-and-stderr-to-buffer
>    FAILED  esh-proc-test/output/stdout-to-buffer
>
> This is on Debian/bookworm.

And...  those went away after a "make bootstrap".  Sorry for the noise.

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Jim Porter]

On 9/4/2022 5:17 PM, Lars Ingebrigtsen wrote:
> After this patch series, I'm getting:
> 
> 4 unexpected results:
>     FAILED  esh-proc-test/exit-status/with-stderr-pipe
>     FAILED  esh-proc-test/output/stderr-to-buffer
>     FAILED  esh-proc-test/output/stdout-and-stderr-to-buffer
>     FAILED  esh-proc-test/output/stdout-to-buffer
> 
> This is on Debian/bookworm.

If the errors say anything about 'eshell-with-temp-buffer', you might 
just need to run "rm test/lisp/eshell/*.elc" to force a full recompile 
of the Eshell test suite.

Several of the tests use a common eshell-tests-helpers.el file, which I 
updated with a new macro (the aforementioned 'eshell-with-temp-buffer'). 
Unfortunately, the tests aren't smart enough to recompile 
eshell-tests-helpers.el first in cases like this. If anyone knows of a 
good way to do this, a patch (or even just a pointer in the right 
direction) would be great.

I probably should have sent a warning about this to emacs-devel, but I 
forgot that I touched eshell-tests-helpers.el in that patch series.

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

Speaking of eshell tests -- this one seems to be failing on EMBA in
nativecomp:

https://emba.gnu.org/emacs/emacs/-/jobs/53337

1 unexpected results:
   FAILED  eshell-test/subcommand-reset-in-pipeline  ((should (eshell-command-result--equal command (eshell-test-command-result command) result)) :form (eshell-command-result--equal "*cat $<echo $eshell-in-pipeline-p | echo> | *cat" "first\n" "first") :value nil :explanation (nonequal-result (command "*cat $<echo $eshell-in-pipeline-p | echo> | *cat") (result "first\n") (expected "first")))

I'm unable to reproduce the failure myself.

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

On 9/6/2022 2:33 AM, Lars Ingebrigtsen wrote:
> Speaking of eshell tests -- this one seems to be failing on EMBA in
> nativecomp:
> 
> https://emba.gnu.org/emacs/emacs/-/jobs/53337
> 
> 1 unexpected results:
>     FAILED  eshell-test/subcommand-reset-in-pipeline  ((should (eshell-command-result--equal command (eshell-test-command-result command) result)) :form (eshell-command-result--equal "*cat $<echo $eshell-in-pipeline-p | echo> | *cat" "first\n" "first") :value nil :explanation (nonequal-result (command "*cat $<echo $eshell-in-pipeline-p | echo> | *cat") (result "first\n") (expected "first")))
> 
> I'm unable to reproduce the failure myself.

Interesting; the test is outputting "first\n", but we don't expect the 
newline. Maybe this is because EMBA uses a different version of "cat"?

I think something like the attached patch would fix things. Then the 
test won't care about whether there's a newline or not.

[-- Attachment #2: 0001-Improve-robustness-of-an-Eshell-regression-test.patch --]
[-- Type: text/plain, Size: 1971 bytes --]

From b23948f4645fa3db3d309e2f1197ff389264b97c Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 6 Sep 2022 11:36:58 -0700
Subject: [PATCH] Improve robustness of an Eshell regression test

* test/lisp/eshell/eshell-tests.el
(eshell-test/subcommand-reset-in-pipeline): Use
'eshell-match-command-output' to check result.

Ref: https://lists.gnu.org/archive/html/emacs-devel/2022-09/msg00314.html
---
 test/lisp/eshell/eshell-tests.el | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index d5112146c2..2dc996e923 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -83,18 +83,19 @@ eshell-test/subcommand-reset-in-pipeline
   (dolist (template '("echo {%s} | *cat"
                       "echo ${%s} | *cat"
                       "*cat $<%s> | *cat"))
-    (eshell-command-result-equal
-     (format template "echo $eshell-in-pipeline-p")
-     nil)
-    (eshell-command-result-equal
-     (format template "echo | echo $eshell-in-pipeline-p")
-     "last")
-    (eshell-command-result-equal
-     (format template "echo $eshell-in-pipeline-p | echo")
-     "first")
-    (eshell-command-result-equal
-     (format template "echo | echo $eshell-in-pipeline-p | echo")
-     "t")))
+    (with-temp-eshell
+     (eshell-match-command-output
+      (format template "echo $eshell-in-pipeline-p")
+      "\\`\\'")
+     (eshell-match-command-output
+      (format template "echo | echo $eshell-in-pipeline-p")
+      "\\`last")
+     (eshell-match-command-output
+      (format template "echo $eshell-in-pipeline-p | echo")
+      "\\`first")
+     (eshell-match-command-output
+      (format template "echo | echo $eshell-in-pipeline-p | echo")
+      "\\`t"))))
 
 (ert-deftest eshell-test/lisp-reset-in-pipeline ()
   "Check that interpolated Lisp forms reset `eshell-in-pipeline-p'."
-- 
2.25.1

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> I think something like the attached patch would fix things. Then the
> test won't care about whether there's a newline or not.

Sounds good to me.  Try pushing it and see what EMBA says...  (By the
way, I see now that the test doesn't fail reliably on EMBA -- it passed
at least once.  So perhaps there's some timing/pipelining issue here?
That is, it's outputting "first" and then "\n" in two separate writes,
but sometimes we get both in the same process output and sometimes in
two bits?)

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Jim Porter]

On 9/6/2022 1:13 PM, Lars Ingebrigtsen wrote:
> Sounds good to me.  Try pushing it and see what EMBA says...  (By the
> way, I see now that the test doesn't fail reliably on EMBA -- it passed
> at least once.  So perhaps there's some timing/pipelining issue here?
> That is, it's outputting "first" and then "\n" in two separate writes,
> but sometimes we get both in the same process output and sometimes in
> two bits?)

Hmm, in that case, I think my patch would be hiding a real bug (though 
possibly just a bug in that test). I'll see if I can force it to fail 
locally, or failing that, we can mark it unstable. I think that would be 
better than working around the bug in the test...

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> Hmm, in that case, I think my patch would be hiding a real bug (though
> possibly just a bug in that test). I'll see if I can force it to fail
> locally, or failing that, we can mark it unstable. I think that would
> be better than working around the bug in the test...

Yup.

In the last run on EMBA, that test didn't fail, but this one did:

Test esh-proc-test/sigpipe-exits-process condition:
    (ert-test-failed
     ((should
       (eq
	(process-list)
	nil))
      :form
      (eq
       (#<process sh-stderr>)
       nil)
      :value nil))
   FAILED  16/16  esh-proc-test/sigpipe-exits-process (1.013610 sec) at lisp/eshell/esh-proc-tests.el:122
Ran 16 tests, 14 results as expected, 1 unexpected, 1 skipped (2022-09-07 10:27:03+0000, 2.594968 sec)
1 unexpected results:
   FAILED  esh-proc-test/sigpipe-exits-process  ((should (eq
    (process-list) nil)) :form (eq (#<process sh-stderr>) nil) :value
    nil)

Also native-comp.

https://emba.gnu.org/emacs/emacs/-/jobs/53405

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On 9/7/2022 4:46 AM, Lars Ingebrigtsen wrote:
> In the last run on EMBA, that test didn't fail, but this one did:
> 
> Test esh-proc-test/sigpipe-exits-process condition:
>      (ert-test-failed
>       ((should
>         (eq
> 	(process-list)
> 	nil))
>        :form
>        (eq
>         (#<process sh-stderr>)
>         nil)
>        :value nil))
>     FAILED  16/16  esh-proc-test/sigpipe-exits-process (1.013610 sec) at lisp/eshell/esh-proc-tests.el:122
> Ran 16 tests, 14 results as expected, 1 unexpected, 1 skipped (2022-09-07 10:27:03+0000, 2.594968 sec)
> 1 unexpected results:
>     FAILED  esh-proc-test/sigpipe-exits-process  ((should (eq
>      (process-list) nil)) :form (eq (#<process sh-stderr>) nil) :value
>      nil)

Luckily, that's an easy one to fix. See attached.

The changes for bug#21605 resulted in that test spawning an extra pipe 
process to monitor stderr, which confused the test. For simplicity, I 
just tweaked the pipeline in that test to avoid spawning the pipe process.

[-- Attachment #2: 0001-Fix-a-race-condition-in-an-Eshell-test.patch --]
[-- Type: text/plain, Size: 1314 bytes --]

From c6bb09c4ac22ffa76f248ccca8285397cc46f12e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 7 Sep 2022 13:58:31 -0700
Subject: [PATCH] Fix a race condition in an Eshell test

* test/lisp/eshell/esh-proc-tests.el
(esh-proc-test/sigpipe-exits-process): Use "|&" when creating the
pipeline to prevent an extra pipe process from being started.
---
 test/lisp/eshell/esh-proc-tests.el | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 52a0d1eeeb..abe363bee0 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -128,8 +128,10 @@ esh-proc-test/sigpipe-exits-process
    (eshell-match-command-output
     ;; The first command is like `yes' but slower.  This is to prevent
     ;; it from taxing Emacs's process filter too much and causing a
-    ;; hang.
-    (concat "sh -c 'while true; do echo y; sleep 1; done' | "
+    ;; hang.  Note that we use "|&" to connect the processes so that
+    ;; Emacs doesn't create an extra pipe process for the first "sh"
+    ;; invocation.
+    (concat "sh -c 'while true; do echo y; sleep 1; done' |& "
             "sh -c 'read NAME; echo ${NAME}'")
     "y\n")
    (eshell-wait-for-subprocess t)
-- 
2.25.1

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> Luckily, that's an easy one to fix. See attached.

Looks good to me.

Re: master updated (5af5ed6c62 -> f07505d1ec)

[Jim Porter]

On 9/8/2022 5:02 AM, Lars Ingebrigtsen wrote:
> Looks good to me.

Thanks. Merged as ef17ba83709794fe0342743397f0e68b90ea1f69. I'll try to 
figure out why the other test is failing intermittently too.