supercollider icon indicating copy to clipboard operation
supercollider copied to clipboard

Add additional waiting time in `TestNodeProxy_Server:test_synthDef_isReleased_afterFree`

Open capital-G opened this issue 1 year ago • 3 comments

Purpose and Motivation

This is rather unstable test and it is unclear to me why it occasionally fails (see https://github.com/supercollider/supercollider/actions/runs/10649343960/job/29543125921#step:7:3469). I now added a waiting time in case the server needs some time for garbage collection or else? If this still fails, there should be a waiting time added in the beginning of the test.

I am unsure if this PR actually fixes the underlying issue, but at least it provides some additional information about the state of the test through printing.

If everything goes well, this fixes #6438

Generally these tests could also run on the same server, instead of booting and killing it for every test - see #6440 (max 50 seconds to gain here).

Types of changes

  • Bug fix

To-do list

  • [x] Code is tested
  • [x] All tests are passing
  • [x] Updated documentation
  • [x] This PR is ready for review

capital-G avatar Sep 03 '24 18:09 capital-G

Has someone a clue why this seems to break now another test? :/

 	[1/2]	TestServer_clientID_booted:test_remoteLogin (1.84s)
		 * after first login, remote client should have its requested clientID.
		 ! after first login, remote client should be known to be fully running.

Fails on

  • this PR: https://github.com/supercollider/supercollider/actions/runs/10688273732/job/29628490240#step:7:6071
  • CI from my fork: https://github.com/capital-G/supercollider/actions/runs/10687772986/job/29627798778#step:7:6071

capital-G avatar Sep 03 '24 18:09 capital-G

I see, the problem behind this seems to be that there is no done OSC message sent when a SynthDef is freed from the server. So there is nothing to wait for in s.sync, and the sync is immediately:



OSCFunc({ |...args| "--done message: --".postln; args.postln }, "/done");
Ndef(\x, { Silent.ar });

(
fork {
	Ndef(\x).source = nil;
	s.sync;
	"done".postln;
}
)

telephon avatar Sep 04 '24 07:09 telephon

So the proposed waiting time in between both states to compare seems like a valid idea here? If this is the case we should add it as a comment for our future self's and others.

But I am still trying to understand why https://github.com/supercollider/supercollider/blob/31bf288b777f70a64e6267f04dbe9e29da4fd86a/testsuite/classlibrary/TestServer_clientID_booted.sc#L131-L162 seems to break now because of this test :/

One thing that seems strange on this clientID test: doesn't self.assert "panic" on a false value (because continuing in a "false/unintended" state could be dangerous), therefore the killing of the PID is never performed when this test fails which is not good. IIUC sclang doesn't have a finally block on try catch that could be normally used here.

Should we discuss this new failing test here or should this be another PR/Issue?

capital-G avatar Sep 04 '24 07:09 capital-G

@capital-G is this PR still valid?

dyfer avatar May 27 '25 22:05 dyfer

Haven't seen this in a while, so probably not.

capital-G avatar May 28 '25 00:05 capital-G