Re: master 8dcb19f 4/4: Add a unit test testing interaction between threads and processes.

[Eli Zaretskii]

> From: Philipp <p.stephani2@gmail.com>
> Date: Sun, 21 Mar 2021 13:00:36 +0100
> Cc: Philipp Stephani <phst@google.com>,
>  emacs-devel@gnu.org
> 
> >>> See below: I don't see how you ensure in this test that each thread
> >>> waits for just one process.
> >> 
> >> Shouldn't providing a process argument to accept-process-output ensure 
> >> this, 
> > 
> > No.  All it ensures is that accept-process-output will not return
> > until it heard from that process.  It doesn't say what would happen
> > with output from other subprocesses that might be running.
> 
> Yeah, but why should that matter?  If multiple processes are started in 
> parallel, surely people shouldn't assume that there's some deterministic 
> order of output arrival across processes.

You've changed the subject: the original question was whether each
thread waits for one process.

> >> the test should pass whether the process objects have thread affinity or 
> >> not (and that's the case for me).
> > 
> > But the test has no control of the affinity, so it just tests some
> > case, not the other(s).  We have no idea what happens in those cases
> > it doesn't test.
> 
> Sure, that's the nature of unit tests: they only test a single scenario.

Sure, but that assumes you have other unit tests that test the other
scenarios.  By contrast, here we have a single test, and you are
saying that it's enough to test this part of the Emacs functionality,
because you've made significant changes in the implementation, and
used just this one test to verify the changes didn't break anything.

> I certainly won't stop anyone from adding further tests testing different 
> scenarios.

I'm wondering why _you_ considered this to be enough.

> > If you aren't interested in implementation details, there's no useful
> > answer to such questions.  For example, maybe whoever implemented
> > set-process-thread had some strange ideas or simply made a design
> > mistake?
> 
> Or we can check the manual?
> 
> "Because threads were a relatively late addition to Emacs Lisp, and due
> to the way dynamic binding was sometimes used in conjunction with
> ‘accept-process-output’, by default a process is locked to the thread
> that created it."

That's highly inaccurate, as I described up-thread.  At least if the
"locked" part is understood in the way you want to interpret it.

> > When the process exits, some thread which waits on its descriptor, or
> > on the self-pipe descriptor, will return from the 'pselect' call, and
> > will attempt to take the global lock.  If it cannot take the global
> > lock, it will block, in which case neither the process output will be
> > delivered nor the sentinel will run, until the thread becomes
> > unblocked.  So, as you see, even your simple expectations aren't
> > really guaranteed.  And your test still succeeds, regardless.  What
> > does that tell you about the usefulness of the test?
> 
> Nothing.  You seem to have very different ideas about the purpose of unit 
> tests than I, so I don't think we should belabor this point any further.

Well, let me say then that I think your unit test as written is
useless, and any results one gets from them can be considered valid,
even if the test hangs.  Because the test creates a situation that is
entirely uninteresting for any practical use of Emacs, and thus the
test is basically tuned to a single platform where it was originally
written.

That's not how tests for our test suite should be designed and
implemented.

> > No, a thread can also exit if it signals an error.
> 
> Of course, like any function.  I was just being a bit sloppy.  The unit test 
> verifies that the threads don't signal errors.

No, because it doesn't verify that none of the threads signaled.