From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: master 8dcb19f 4/4: Add a unit test testing interaction between threads and processes. Date: Sun, 21 Mar 2021 14:23:09 +0200 Message-ID: <83h7l4bsr6.fsf@gnu.org> References: <83k0s34eo1.fsf@gnu.org> <83h7n74cwe.fsf@gnu.org> <838s8i4ak8.fsf@gnu.org> <6C5E64C7-19A6-4D46-AC30-33C29FA5DEBB@gmail.com> <83mtvo7xno.fsf@gnu.org> <25EEFE6F-9532-4A5A-8824-A3F8F41012F3@gmail.com> <83y2ekfp3e.fsf@gnu.org> <50388E17-1D84-4823-94D9-923E792DC3D1@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="5996"; mail-complaints-to="usenet@ciao.gmane.io" Cc: phst@google.com, emacs-devel@gnu.org To: Philipp Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Mar 21 13:23:55 2021 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1lNx7b-0001Ru-FW for ged-emacs-devel@m.gmane-mx.org; Sun, 21 Mar 2021 13:23:55 +0100 Original-Received: from localhost ([::1]:51508 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lNx7a-0002zW-FT for ged-emacs-devel@m.gmane-mx.org; Sun, 21 Mar 2021 08:23:54 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:40616) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lNx6v-0002aa-Aa for emacs-devel@gnu.org; Sun, 21 Mar 2021 08:23:13 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:39118) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lNx6r-0000Ra-HW; Sun, 21 Mar 2021 08:23:12 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:3262 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1lNx6q-0006ps-Qu; Sun, 21 Mar 2021 08:23:09 -0400 In-Reply-To: <50388E17-1D84-4823-94D9-923E792DC3D1@gmail.com> (message from Philipp on Sun, 21 Mar 2021 13:00:36 +0100) X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:266700 Archived-At: > From: Philipp > Date: Sun, 21 Mar 2021 13:00:36 +0100 > Cc: Philipp Stephani , > 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.