dyfer
commented
3 years ago @jamshark70 do you have any ideas about these failures?
Open dyfer opened 3 years ago
dyfer
commented
3 years ago @jamshark70 do you have any ideas about these failures?
jamshark70
commented
3 years ago do you have any ideas about these failures?
5 second question, 45 minutes' research :laughing:
test_newFromTempoClock_reschedulesOldClockQueue -- There's some odd problem with the Pbind in this test sending messages -- somehow one of the messages is still on TempoClock, after that clock has been stopped. I can't figure out why. In any case, there's no need for the stream player to be generating OSC messages at all, so: if I change the one line to streamplayer = Pbind(\type, \rest, \dur, 0.02).play(tempoClock);, then I get no failures.
test_newLinkClock_shouldNotChangeLinkTempo -- consider this:
(
var i = 0;
f = { |tempo = 1|
var new;
var count;
i = i + 1;
count = i;
new = LinkClock(tempo);
SimpleController(new)
.put(\tempo, {
new.tempo.debug("clock % tempo".format(count))
});
new
};
)
a = f.(2.5); // no tempo post
b = f.(1); // quickly posts tempo
a.stop; b.stop;If I hit a =, wait at least a second (or 2), then hit b =, b's posted tempo is always the expected 2.5. But if I hit them fairly quickly, even half a second between them, then b posts tempo 1 fairly often.
The likely explanation is that Ableton Link doesn't guarantee sync within a certain amount of time. The usual use case is that you open a music app, and the app will create one Link peer, and this peer is expected to be running for several minutes or hours. So it isn't important to have absolutely instantaneous handshaking: in most cases, you will set up the clock, and then start playing at least two or three seconds later.
But here, we crash into the requirements of our test suite, where we don't want individual tests to take too long. (Also, I think in this scenario, stress testing is largely useless because creating a Link is not something that you ever really stress, within one app.)
My opinion is that it is impossible to test LinkClock in pretty much any capacity according to the usual timing recommendations. This test is designed for a 1.0-second cycle and even that is not long enough. If I change the two wait statements to 1.0.wait instead of 0.5, then the failures disappear. But code reviews in the past were rather stern about 2000-ms tests.
test_LinkClock_sync_meter_aligns_barlines -- The test is supposed to create one reference clock -- then, a test clock should sync to it -- then we check that it synced, and delete the test clock, and try another test clock. So at any moment, there should be only one reference clock and one test clock.
"ERROR: LinkClock peers disagree on barline positions" means that at the moment of trying to sync a test clock, there exist at least two reference clocks. (It's impossible to have a barline conflict with only one.) The test code is pretty clearly doing testClock.stop; before looping back to create the next one, so this should never happen. So -- maybe it takes a short time to dispose of the previous test clock? Or...? I dunno... Maybe try adding a short wait after stopping the test clock.
FWIW I can't reproduce the failure with this test, on multiple runs of your 20-cycle loop.
dyfer
commented
3 years ago Thank you @jamshark70 for looking into this!
I'll try to implement more relaxed timing (with a note that the test(s) are unusually long for the reasons you've provided) to see if we can get these tests back in. I'd like to do that partially because I wanted to have something to test https://github.com/supercollider/supercollider/pull/5416 with :)
dyfer
commented
2 years ago Update:
I thought that maybe the LinkClock tests that fail when run inside the test suite are the ones that use the Semaphore object. I'm speculating that there might be an interaction between routines and Ultimately, I don't know what causes these issues.
Here's an example output of one of the errors:try {} statements of our test runner (or maybe some other aspect of that script?) and the Semaphore behavior.
BTW I have tried suggested fixes and while they did seem to work for running multiple tests in a row locally, they did not prevent the errors from occurring in the CI.
I tried to turn some of the tests back on in #5702 but ultimately failed, since the test suite kept freezing when running these tests, e.g. https://github.com/supercollider/supercollider/runs/4875282243?check_suite_focus=true
jamshark70
commented
2 years ago https://github.com/supercollider/supercollider/issues/232
Try within a loop is Loops within try not reliable, and it's not going to be reliable until that bug is fixed. Unfortunately nobody knows how.
What is odd about this specific error is that prNumPeersChanged is called only from the backend -- never as a method invocation from the language.
void OnNumPeersChanged(std::size_t numPeers) {
gLangMutex.lock();
g->canCallOS = false;
++g->sp;
SetObject(g->sp, mTempoClockObj);
++g->sp;
SetInt(g->sp, numPeers);
runInterpreter(g, getsym("prNumPeersChanged"), 2);
g->canCallOS = false;
gLangMutex.unlock();
}We lock the interpreter, push the LinkClock object, push an integer, and call the method -- but the interpreter applies the message to a totally unrelated object. I can't explain that other than something is mucking around with the stack between the backend calling runInterpreter and the interpreter actually dispatching the message... but this should not happen because the interpreter is locked.
Environment
Steps to reproduce
Setup:
testsuite/classlibraryto your sclang pathRun tests:
test_newFromTempoClock_reschedulesOldClockQueue- some iterations error out:Result:
test_newLinkClock_shouldNotChangeLinkTempo- some iterations fail:Result:
test_LinkClock_sync_meter_aligns_barlines- some iterations error out or fail:Result:
Expected vs. actual behavior
Some tests in TestLinkClock either fail occasionally or throw errors. On my system this can only be observed when running multiple iterations of each test.