Bug#990244: tests/streams: Failing test on pipe stdout file descriptor
control: forwarded -1 https://sourceforge.net/p/clisp/bugs/747/
X-Debbugs-CC: Ariel D'Alessandro <ariel.dalessandro@collabora.com>
On Thu, Dec 09, 2021 at 11:20:52PM -0300, Ariel D'Alessandro wrote:
> Did a quick check by running the lisp tests using ptywrap and the error
> is no longer happening. Of course, now the pty is used instead of the
> (root owned) pipe, so permissions are fine to re-open the file.
>
> So, moving forward. This is the failing test simplified:
>
> (streamp (setq s (make-stream :error))) T
> (let ((*reopen-open-file* nil)) ; stdout can be a file, it will be detected!
> (with-open-file (copy s :direction :output) (streamp copy))) T
>
> My question here would be: is this test expected to always succeed?
You'd have to ask the original author of the test that. In a perfect
world every test would be accompanied by a detailed description of
what exactly it tests for, what conditions it expects and what
constitutes a pass or failure.
> AFAIU, we can't assume to have the permissions to re-open the file
> related to the file descriptor, which is what the code is explicitly
> trying to do.
In a test suite you usually can since it is probably not supposed to
be run by a privileged user in such a way that it fails. Build
servers and CI pipelines, which are almost always involved in exposing
such bugs, really ought to put in more effort to make their
environment more similar to the one the developer develops and tests
the code in instead of silently expecting everyone else to put in the
additional effort to placate them. Fooling a build/test suite into
thinking it is hooked to a terminal with a tool like ptywrap is really
not that big of an ask and it should be a default feature in all such
products. Alternatively the privileged invoker could just as easily
call chown(2) on the pipe after opening it and solve the issue this
way.
> Please correct me if I'm wrong as I'm not used to clisp syntax, just
> going through the source code :-)
>
> From what I see, it responsibility of the clisp test code to check if it
> has the permissions to re-open the file. Otherwise, it's not guaranteed
> to succeed. For example, having this fd pointing to a (root owned) pipe
> is a valid situation and should be properly handled by this test.
That's debatable. Yes, it could be argued that the test should do a
better job at disarming itself by testing if it can reopen the file
with e.g. access(2). OTOH it could also be argued that it is the
invoker's burden to ensure conditions that allow the test to succeed.
Writing test suites is annoying enough as is. No need to make it more
annoying by also expecting accommodation for build servers and CI
pipelines if they themselves could do the accommodation much more
easily.
Regards.
Reply to: