GNU bug report logs - #62578
30.0.50; [PATCH] Add regression tests for synchronous processes in Eshell

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 62578 in the body.
You can then email your comments to 62578 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Add regression tests for synchronous processes in
 Eshell
Date: Fri, 31 Mar 2023 21:41:09 -0700

[Message part 1 (text/plain, inline)]

As far as I understand it, Eshell only uses synchronous processes on 
MS-DOS (where 'make-process' doesn't exist). Since I don't build Emacs 
for MS-DOS, and I expect not many others do either, I'm worried that 
this support will regress. To avoid that, here are some regression tests.

It also fixes a small bug with how Eshell updated its internal markers 
when using synchronous processes. This mostly affects the tests, but 
could also cause unexpected behavior when using some of Eshell's 
buffer-navigation commands.

[0001-Add-tests-for-synchronous-processes-in-Eshell.patch (text/plain, attachment)]

Message #8 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50;
 [PATCH] Add regression tests for synchronous processes in Eshell
Date: Sat, 01 Apr 2023 09:07:00 +0300

> Date: Fri, 31 Mar 2023 21:41:09 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> As far as I understand it, Eshell only uses synchronous processes on 
> MS-DOS (where 'make-process' doesn't exist). Since I don't build Emacs 
> for MS-DOS, and I expect not many others do either, I'm worried that 
> this support will regress. To avoid that, here are some regression tests.

Thanks.

> +(ert-deftest esh-proc-test/synchronous-proc/simple/interactive ()
> +  "Test that synchronous processes work in an interactive shell."
> +  (skip-unless (executable-find "echo"))

This will always skip on MS-DOS, since "echo" is not available as an
external program OOTB, only if GNU Coreutils are installed.  And even
if Coreutils _are_ installed, a command that invokes "echo" will most
probably invoke the shell builtin instead.

> +  (skip-unless (and (executable-find "echo")
> +                    (executable-find "rev")))

Likewise here: "rev" is not expected to be available on MS-DOS.

I think you need to rethink these tests in a way that uses different
commands.  My suggestion is to use the Emacs executable, since that is
always available when running these tests.

Message #11 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sat, 1 Apr 2023 00:16:38 -0700

On 3/31/2023 11:07 PM, Eli Zaretskii wrote:
>> Date: Fri, 31 Mar 2023 21:41:09 -0700
>> From: Jim Porter <jporterbugs <at> gmail.com>
>>
>> +(ert-deftest esh-proc-test/synchronous-proc/simple/interactive ()
>> +  "Test that synchronous processes work in an interactive shell."
>> +  (skip-unless (executable-find "echo"))
> 
> This will always skip on MS-DOS, since "echo" is not available as an
> external program OOTB, only if GNU Coreutils are installed.  And even
> if Coreutils _are_ installed, a command that invokes "echo" will most
> probably invoke the shell builtin instead.

My main goal here is to test synchronous subprocesses on platforms 
*other than* MS-DOS, since these new tests aren't really necessary on 
MS-DOS itself: there are plenty of existing Eshell tests that create 
processes, and they'd *all* use synchronous subprocesses on MS-DOS.

As for calling a shell builtin, Eshell should translate "*echo" to 
"/path/to/echo" (or "C:/path/to/echo.exe") before calling it, so it 
shouldn't use a shell builtin here. You're right though that the tests 
probably require GNU Coreutils to be installed (this is also true of 
quite a few existing Eshell tests though).

>> +  (skip-unless (and (executable-find "echo")
>> +                    (executable-find "rev")))
> 
> Likewise here: "rev" is not expected to be available on MS-DOS.

To make this work in more places, I could switch "rev" to something 
that's actually in GNU Coreutils. Maybe "wc".

> I think you need to rethink these tests in a way that uses different
> commands.  My suggestion is to use the Emacs executable, since that is
> always available when running these tests.

Since these tests are meant to check the "synchronous subprocess" code 
in Eshell on non-MS-DOS platforms, I'd say it's ok. However, I can 
change my patch if you prefer. I could either:

1) Add a comment to the tests explaining that they're just meant to 
simulate some of MS-DOS's limitations on non-MS-DOS systems, or

2) Rework these tests so they work the same on both MS-DOS and other 
systems.

Personally, I lean softly towards (1), partly because the Eshell test 
suite probably breaks in quite a few other places on MS-DOS anyway. 
However, it shouldn't be too hard to do (2) instead.

What do you think?

Message #14 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sat, 01 Apr 2023 10:22:22 +0300

> Date: Sat, 1 Apr 2023 00:16:38 -0700
> Cc: 62578 <at> debbugs.gnu.org
> From: Jim Porter <jporterbugs <at> gmail.com>
> 
> Since these tests are meant to check the "synchronous subprocess" code 
> in Eshell on non-MS-DOS platforms, I'd say it's ok. However, I can 
> change my patch if you prefer. I could either:
> 
> 1) Add a comment to the tests explaining that they're just meant to 
> simulate some of MS-DOS's limitations on non-MS-DOS systems, or
> 
> 2) Rework these tests so they work the same on both MS-DOS and other 
> systems.
> 
> Personally, I lean softly towards (1), partly because the Eshell test 
> suite probably breaks in quite a few other places on MS-DOS anyway. 
> However, it shouldn't be too hard to do (2) instead.
> 
> What do you think?

I don't understand why not use Emacs instead of all those external
commands.  That solves all the problems nicely and portably, and still
allows you to do anything you want.

But it's your call, eventually.

Message #17 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sat, 1 Apr 2023 00:40:16 -0700

On 4/1/2023 12:22 AM, Eli Zaretskii wrote:
> I don't understand why not use Emacs instead of all those external
> commands.  That solves all the problems nicely and portably, and still
> allows you to do anything you want.

Mostly just so that the tests are easier to understand, but for these 
newly-added ones, I think they should be easy enough to understand no 
matter what. I'll try using Emacs.

My only concern there is making Emacs do something reasonable when 
piping data into it, but I can just try a few things out, and if it 
really doesn't work, the existing tests in my patch are hopefully still 
better than nothing.

Message #20 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sat, 1 Apr 2023 17:58:59 -0700

[Message part 1 (text/plain, inline)]

On 4/1/2023 12:40 AM, Jim Porter wrote:
> My only concern there is making Emacs do something reasonable when 
> piping data into it, but I can just try a few things out, and if it 
> really doesn't work, the existing tests in my patch are hopefully still 
> better than nothing.

Ok, that wasn't too hard. How does this look?

[0001-Add-tests-for-synchronous-processes-in-Eshell.patch (text/plain, attachment)]

Message #23 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sun, 02 Apr 2023 08:29:27 +0300

> Date: Sat, 1 Apr 2023 17:58:59 -0700
> From: Jim Porter <jporterbugs <at> gmail.com>
> Cc: 62578 <at> debbugs.gnu.org
> 
> On 4/1/2023 12:40 AM, Jim Porter wrote:
> > My only concern there is making Emacs do something reasonable when 
> > piping data into it, but I can just try a few things out, and if it 
> > really doesn't work, the existing tests in my patch are hopefully still 
> > better than nothing.
> 
> Ok, that wasn't too hard. How does this look?

LGTM, thanks.

One minor nit: 'message' in a batch session prints to stderr, not to
stdout.

Message #26 received at 62578 <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62578 <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sat, 1 Apr 2023 22:51:30 -0700

[Message part 1 (text/plain, inline)]

On 4/1/2023 10:29 PM, Eli Zaretskii wrote:
> One minor nit: 'message' in a batch session prints to stderr, not to
> stdout.

!! Thanks for catching that. That actually means there's a small bug (or 
a missing feature) in Eshell: it doesn't support redirecting stdout and 
stderr to separate places when using synchronous subprocesses.

I've added a note about this to esh-proc.el (fixing it would probably be 
tricky), and changed 'message' in the tests to be 'princ'. See attached; 
I'll probably merge this tomorrow unless you catch any remaining issues.

[0001-Add-tests-for-synchronous-processes-in-Eshell.patch (text/plain, attachment)]

Message #31 received at 62578-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Porter <jporterbugs <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 62578-done <at> debbugs.gnu.org
Subject: Re: bug#62578: 30.0.50; [PATCH] Add regression tests for synchronous
 processes in Eshell
Date: Sun, 2 Apr 2023 14:25:35 -0700

On 4/1/2023 10:51 PM, Jim Porter wrote:
> I'll probably merge this tomorrow unless you catch any remaining issues.

Merged as 00144fa287e. Closing this now.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.