FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,7 +48,7 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| speechSynthesis.cancel(); | ||
| promise.reject(); | ||
| }, delay || 1000); | ||
| toSpeak.onend = function () { | ||
| clearTimeout(timeout); | ||
| speechSynthesis.cancel(); | ||
| promise.resolve(); | ||
|
|
@@ -109,16 +109,16 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| args: ["{that}"] | ||
| }, | ||
| pause: { | ||
| funcName: "fluid.textToSpeech.asyncSpeechSynthesisControl", | ||
| args: ["pause", 10] | ||
| }, | ||
| resume: { | ||
| funcName: "fluid.textToSpeech.asyncSpeechSynthesisControl", | ||
| args: ["resume", 10] | ||
| }, | ||
| getVoices: { | ||
| funcName: "fluid.textToSpeech.asyncSpeechSynthesisControl", | ||
| args: ["getVoices", 10] | ||
| }, | ||
| handleStart: { | ||
| funcName: "fluid.textToSpeech.handleStart", | ||
|
|
@@ -142,6 +142,16 @@ var fluid_2_0_0 = fluid_2_0_0 || {}; | |
| } | ||
| }); | ||
|
|
||
| // Issue functions to the speechSynthesis interface | ||
| // after a delay | ||
| // Makes the wrapper behave better when issuing commands | ||
| // in rapid succession | ||
| fluid.textToSpeech.asyncSpeechSynthesisControl = function (control, delay) { | ||
|
Member
There was a problem hiding this comment. I really wanted to approve this and had written a comment declaring that it was probably "good enough" but I think on balance that it probably isn't. There is no guarantee that timeouts scheduled at closely spaced instants in time will be honoured in the order that they are issued - here is some art: http://stackoverflow.com/questions/1776239/are-equal-timeouts-executed-in-order-in-javascript
Member
Author
There was a problem hiding this comment. I agree with your assessment - closely spaced timeouts are notoriously unreliable. I was hoping this might be a sufficient patch for the moment, but a managed queue of pause/resume events won't take too long to implement. |
||
| setTimeout(function () { | ||
| speechSynthesis[control](); | ||
| }, delay); | ||
| }; | ||
|
|
||
| fluid.textToSpeech.speak = function (that, speaking) { | ||
| that.events[speaking ? "onStart" : "onStop"].fire(); | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,113 +31,171 @@ https://github.com/fluid-project/infusion/raw/master/Infusion-LICENSE.txt | |
| }); | ||
|
|
||
| fluid.tests.textToSpeech.cleanUp = function () { | ||
| if(fluid.textToSpeech.isSupported()) { | ||
|
Member
There was a problem hiding this comment. Presumably spacing that fails linting - or at least I hope it does!
Member
Author
There was a problem hiding this comment. What really fails is my failure to install my precommit hooks to make sure i can't commit unlinted code :( |
||
| speechSynthesis.cancel(); | ||
| } | ||
| }; | ||
|
|
||
| fluid.defaults("fluid.tests.textToSpeech.ttsTestEnvironment", { | ||
| gradeNames: "fluid.test.testEnvironment", | ||
| components: { | ||
| tts: { | ||
| type: "fluid.tests.textToSpeech", | ||
| createOnEvent: "{ttsTester}.events.onTestCaseStart" | ||
| }, | ||
| ttsTester: { | ||
| type: "fluid.tests.textToSpeech.ttsTester" | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| fluid.defaults("fluid.tests.textToSpeech.ttsTester", { | ||
| gradeNames: ["fluid.test.testCaseHolder"], | ||
| modules: [ | ||
| { | ||
| name: "Initialization", | ||
| tests: [{ | ||
| expect: 4, | ||
| name: "Test initialization", | ||
| sequence: | ||
| [{ | ||
| func: "fluid.tests.textToSpeech.testInitialization", | ||
| args: ["{tts}"] | ||
| }] | ||
| }], | ||
|
|
||
| }, | ||
| { | ||
| name: "Start and Stop Events", | ||
| tests: [{ | ||
| expect: 10, | ||
| name: "Test Start and Stop Events", | ||
| sequence: | ||
| [{ | ||
| func: "{tts}.queueSpeech", | ||
| args: "Testing start and end events" | ||
| }, { | ||
| listener: "fluid.tests.textToSpeech.testStart", | ||
| args: ["{tts}"], | ||
| event: "{tts}.events.onStart" | ||
| }, { | ||
| listener: "fluid.tests.textToSpeech.testStop", | ||
| args: ["{tts}"], | ||
| event: "{tts}.events.onStop" | ||
| }] | ||
| }] | ||
| }, | ||
| { | ||
| name: "Test Including Pause and Resume Events", | ||
| tests: [{ | ||
| expect: 13, | ||
| name: "Test Including Pause and Resume Events", | ||
| sequence: | ||
| [ | ||
| { | ||
| func: "{tts}.queueSpeech", | ||
| args: "Testing pause and resume events" | ||
| }, | ||
| // This is necessary or {tts}.pause can be called | ||
| // before the speech event has actually started, | ||
| // which messes up the sequencing | ||
| { | ||
| listener: "{tts}.pause", | ||
| event: "{tts}.events.onStart" | ||
| }, | ||
| { | ||
| listener: "fluid.tests.textToSpeech.testPause", | ||
| args: ["{tts}"], | ||
| event: "{tts}.events.onPause" | ||
| }, | ||
| { | ||
| func: "{tts}.resume" | ||
| }, { | ||
| listener: "fluid.tests.textToSpeech.testResume", | ||
| args: ["{tts}"], | ||
| event: "{tts}.events.onResume" | ||
| }, | ||
| // Test on stop to make sure IoC harness doesn't try | ||
| // and destroy too early for the asynchronous onStop | ||
| // event to fire and cause issues | ||
| { | ||
| listener: "fluid.tests.textToSpeech.testStop", | ||
| args: ["{tts}"], | ||
| event: "{tts}.events.onStop" | ||
| } | ||
| ] | ||
| }] | ||
| }] | ||
| }); | ||
|
|
||
| fluid.tests.textToSpeech.testInitialization = function (tts) { | ||
| var that = tts; | ||
| jqUnit.assertTrue("The Text to Speech component should have initialized", that); | ||
| jqUnit.assertFalse("Nothing should be speaking", that.model.speaking); | ||
| jqUnit.assertFalse("Nothing should be pending", that.model.pending); | ||
| jqUnit.assertFalse("Shouldn't be paused", that.model.paused); | ||
| }; | ||
|
|
||
| fluid.tests.textToSpeech.testStart = function (tts) { | ||
| var that = tts; | ||
| jqUnit.assert("The onStart event should have fired"); | ||
| jqUnit.assertTrue("Should be speaking", that.model.speaking); | ||
| jqUnit.assertFalse("Nothing should be pending", that.model.pending); | ||
| jqUnit.assertFalse("Shouldn't be paused", that.model.paused); | ||
| jqUnit.assertDeepEq("The queue should be empty", [], that.queue); | ||
| }; | ||
|
|
||
| fluid.tests.textToSpeech.testStop = function (tts) { | ||
| var that = tts; | ||
| jqUnit.assert("The onStop event should have fired"); | ||
| jqUnit.assertFalse("Should not be speaking", that.model.speaking); | ||
| jqUnit.assertFalse("Nothing should be pending", that.model.pending); | ||
| jqUnit.assertFalse("Shouldn't be paused", that.model.paused); | ||
| jqUnit.assertDeepEq("The queue should be empty", [], that.queue); | ||
| that.cancel(); | ||
| }; | ||
|
|
||
| fluid.tests.textToSpeech.testPause = function (tts) { | ||
| var that = tts; | ||
| jqUnit.assert("The pause event should have fired"); | ||
| jqUnit.assertTrue("Should be speaking", that.model.speaking); | ||
| jqUnit.assertFalse("Nothing should be pending", that.model.pending); | ||
| jqUnit.assertTrue("Should be paused", that.model.paused); | ||
| }; | ||
|
|
||
| fluid.tests.textToSpeech.testResume = function (tts) { | ||
| var that = tts; | ||
| jqUnit.assert("The resume event should have fired"); | ||
| jqUnit.assertTrue("Should be speaking", that.model.speaking); | ||
| jqUnit.assertFalse("Nothing should be pending", that.model.pending); | ||
| jqUnit.assertFalse("Shouldn't be paused", that.model.paused); | ||
| }; | ||
|
|
||
| fluid.tests.textToSpeech.bypassTest = function () { | ||
| jqUnit.test("Tests were skipped - browser does not appear to support TTS", function () { | ||
| jqUnit.assert("TESTS SKIPPED - browser does not support SpeechSynthesis"); | ||
| }); | ||
| }; | ||
|
|
||
| var ttsStatus; | ||
|
|
||
| jqUnit.asyncTest("Confirming if TTS is available", function () { | ||
| jqUnit.expect(1); | ||
| var ttsSupport = fluid.textToSpeech.checkTTSSupport(); | ||
| ttsSupport.then(function (){ | ||
| ttsStatus = "TTS is available"; | ||
| fluid.tests.textToSpeech.ttsTestEnvironment(); | ||
| }, function () { | ||
| ttsStatus = "TTS is unavailable"; | ||
| fluid.tests.textToSpeech.bypassTest(); | ||
| }); | ||
|
|
||
| var ttsTest = function () { | ||
|
Member
There was a problem hiding this comment. I don't see a reason for enclosing these definitions in a function and executing them asynchronously?
Member
Author
There was a problem hiding this comment. This is to ensure the actual asynchronous TTS tests have either started to run or been skipped because TTS is not supported in the all-tests context. If there's not an asynchronous test wrapper around the promise at lines 184-191, the all-tests runner moves on to the next test (it won't wait for the promise to resolve); this seems to have been the cause of https://issues.fluidproject.org/projects/FLUID/issues/FLUID-5735. I tried experimenting at first with a
Member
There was a problem hiding this comment. Ok, I think we are mixing up here the requirement to have executed the asynchronous fluid.textToSpeech.checkTTSSupport before beginning the test and the asynchronous tests themselves. The "test scheduling a test" workflow is ok (since this happens a lot in practice with QUnit) but can be cleaned up. All that we need is for the body of ttsTest to execute after the ttsSupport promise resolution, and the reject case is already fully self-sufficient, all we are adding to it is an extra assertion message. The most compact way of setting this up is probably to insert a ttsSupport.then(jqUnit.start, jqUnit.start) at line 185, and eliminate all the other material.
Member
Author
There was a problem hiding this comment. Understood - I simplified it and also extracted it as a general function pattern for choosing test execution / bypass based on a promise result in a way that behaves safely with the all-tests runner. |
||
| jqUnit.start(); | ||
| jqUnit.assert(ttsStatus); | ||
| }; | ||
|
|
||
| setTimeout(ttsTest, 1500); | ||
|
Member
There was a problem hiding this comment. Sorry, looks like I managed to comment on the commit for this rather than the pull
Member
Author
There was a problem hiding this comment. It's .5 seconds longer than the default timeout of |
||
|
|
||
| }); | ||
| })(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very short line wrapped comments - perhaps an IDE setting run amok?