phetsims / utterance-queue

Alerting library powered by aria-live
MIT License
0 stars 2 forks source link

Create more unit tests for priorityProperty #50

Closed jessegreenberg closed 2 years ago

jessegreenberg commented 2 years ago

From https://github.com/phetsims/joist/issues/752, we noted a bunch of cases we want to support with priorityProperty, specifically

// because we just want to test priority
const noCancelOptions = {
  cancelSelf: false,
  cancelOther: false
}
const firstUtterance = new phet.utteranceQueue.Utterance( {
  alert: 'This is a first utterance, it was put in the queue first.',
  announcerOptions: noCancelOptions
} );
const secondUtterance = new phet.utteranceQueue.Utterance( {
  alert: 'This is a second utterance, it was put in the queue second.',
  announcerOptions: noCancelOptions
} );
const thirdUtterance = new phet.utteranceQueue.Utterance( {
  alert: 'This is a third utterance, it was put in the queue third.',
  announcerOptions: noCancelOptions
} );

//-----------------------------------------------------------

// Add all 3 to back
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( secondUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// if we do this, it would interrupt the first one and then we would hear the third one
secondUtterance.priorityProperty.value = 2;

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );

// after these, we should hear thirdUtterance right away
firstUtterance.priorityProperty.value = 0;
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );

// after these, we should hear thirdUtterance right away
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );
firstUtterance.priorityProperty.value = 0;

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// we should hear firstUtterance for 2 seconds, then get interrupted by third utterance
window.setTimeout( () => {
  firstUtterance.priorityProperty.value = 0;
}, 1000 );

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 0;
thirdUtterance.priorityProperty.value = 0;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// we should hear firstUtterance for 2 seconds, then get interrupted by third utterance
window.setTimeout( () => {
  thirdUtterance.priorityProperty.value = 3;
}, 2000 );

//-----------------------------------------------------------

firstUtterance.priorityProperty.value = 10;
phet.scenery.voicingUtteranceQueue.addToBack( firstUtterance );
phet.scenery.voicingUtteranceQueue.addToBack( thirdUtterance );

// we should hear both Utterances in full, the new priority for firstUtterance is higher
// than the second one in the queue
window.setTimeout( () => {
  firstUtterance.priorityProperty.value = 5;
}, 2000 );

//-----------------------------------------------------------

A few of these were added in UtteranceQueueTests, lets make sure that they are all in there.

jessegreenberg commented 2 years ago

These cases were converted to unit tests in the above commit. They usually pass for me in both chrome and Firefox. But I think they occasionally fail because of the way the stepTimer emits in these tests. Opening a new issue for that: https://github.com/phetsims/utterance-queue/issues/51

@zepumph would you like to review or add any more tests?

zepumph commented 2 years ago

Looks really nice. Thanks! If I were writing them, I would have a small preference to get rid of resetQueueAndAnnouncer, and instead make each separate test its own qunit test. That way you can put that logic in beforeEach and all will be good. It seems simpler and would have less code. I didn't want to muck with them if you had opinions though.

Over to you!

jessegreenberg commented 2 years ago

I wondered about that - I avoided it because I thought it was trade for more QUnit boilerplate. But coming back I actually strongly prefer it. These tests take forever to run and separating them out into individual tests will let us isolate just a broken test instead of needing to wait for several at a time. Ill break them up.

jessegreenberg commented 2 years ago

Tests have been broken up into individual tests for each that required a reset of queue and announcer. Anything else @zepumph?