FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732

waharnum · 2016-08-04T18:00:57Z

This PR addresses the bug described at https://issues.fluidproject.org/browse/FLUID-5936

Additionally, it rewrites the existing tests (but does not change what they test) to use the IoC testing framework to make test hangs related to async timeouts or non-firing events more visible.

merge upstream master

…Chrome

…nous behaviour of TTS between browsers.

amb26 · 2016-08-04T19:50:14Z

These tests hang for me at "Test Pause and Resume Events":

Thu Aug 04 2016 20:49:42 GMT+0100 (GMT Daylight Time): Test case listener has not responded after 5000ms - at sequence pos 5 of 6 sequence element Object {listener: "fluid.tests.textToSpeech.testResume", args: Array[1], event: "{tts}.events.onResume"} of fixture Test Pause and Resume Events

Configuration: Chrome 52.0.2743.116 m (64-bit) on Windows 7 64-bit

amb26 · 2016-08-04T19:50:41Z

tests/component-tests/textToSpeech/js/TextToSpeechTests.js

-    }
+    };
+
+    // fluid.setLogging(fluid.logLevel.TRACE);


Stray comment

waharnum · 2016-08-04T19:57:03Z

Thanks @amb26 - booting up my Windows VM to check. @jobara has mentioned there was an issue with pause/resume in Chrome that caused that test to be skipped via browser detection previously, it's possible that's now Windows-only.

…work on all browsers between OSX and Windows.

waharnum · 2016-08-05T17:08:48Z

The best way of achieving reliable tests with the current implementation (in both senses I used it above) appears to be short delays in issuing pause/resume commands, which also more closely emulates a typical user interaction with things like pause/resume controls.

From what research I've done (and what information is easily available), I think that various browser implementations of TTS have somewhat differing asynchronous behaviours, both within the different browsers, and within the browser's interaction with the operating system APIs to perform TTS.

This leads to differences in when certain TTS events the wrapper component depends on to fire its own events occur, both generally between different browsers/OSs, and in individual instances.

A long way of saying that introducing a short delay when invoking pause/resume leads to reliably passing tests on OS X and Windows.

amb26 · 2016-08-05T17:11:34Z

This doesn't strike me as very reliable. If the component is not responsive to events on a certain timescale, this should be built into its implementation rather than left as an artefact of the test cases. We should work to absorb whatever cross-platform slop there is into our own interface rather than special-case our testing requirements.

waharnum · 2016-08-05T17:34:48Z

I'm of two minds about this (and trying to not have one or both of them influenced by the mind that would like to move on from the pit of cross-browser/cross-OS TTS issues...):

Argument for Leaving the Component As-Is

I think it's less that the component itself is not responsive to events on a certain timescale than that underlying browser TTS events the component relies upon do not appear to be responsive to events on a certain timescale. I know this may sound like developer finger-pointing, but I think it has at least some basis in reality. I believe you'd see similar issues non-asychronously issuing rapid pause/resume commands through the TTS interface - events get lost, race conditions occur, etc.

Argument for Absorbing Cross-Platform Slop

I think it would probably be possible to make some improvements as part of this PR that might help to deal with the cross-platform issues. In particular, I haven't traced all the scenarios fully, but I believe there may be instances where internal component state can diverge from the state of the browser TTS interface around whether or not speech is playing/paused.

So I can try refactoring to:

make the component state more reflective of / coupled to the browser TTS state. This may introduce other issues, due to bugs such as https://bugs.chromium.org/p/chromium/issues/detail?id=425553&q=SpeechSynthesis&colspec=ID%20Pri%20M%20Week%20ReleaseBlock%20Cr%20Status%20Owner%20Summary%20OS%20Modified
introduce the slight delays I found improved test reliability into the component's invocation of the browser TTS. This might improve race conditions and non-firing events.

Writing this out makes me realize I should be of the second mind, as I'm probably in the best position to do that work I'll be right now, as opposed to if I step away from it for weeks until I "have more time". 😛

…om tests

waharnum · 2016-08-08T18:48:45Z

I've moved the approach of issuing asynchronous pause/resume commands to the speechSynthesis object from the tests to the main component, and refactored the invokers to use this approach (the fluid.textToSpeech.asyncSpeechSynthesisControl function) rather than invoking the speechSynthesis commands directly.

A functionally imperceptible delay via setTimeout (10ms) before issuing commands results in consistently passing tests in TTS-supporting browsers on both Mac and Windows.

waharnum · 2016-08-15T13:16:13Z

@amb26, is there anything else that you think should be done on this one? It would be nice to get it in for the 2.0 release.

waharnum · 2016-08-15T17:57:16Z

update to say that I'm also looking at https://issues.fluidproject.org/projects/FLUID/issues/FLUID-5735 as part of this - tests are still failing when run as part of all-tests, but I believe I've pinned down why and should have a fix shortly.

…e all-tests runner from not handling these tests properly

waharnum · 2016-08-15T19:39:34Z

I believe FLUID-5735 should now be fixed as part of this PR by commit 5ca02a8 - the issue was with the approach taken to deciding whether or not to execute the test. Wrapping it in the fluid.textToSpeech.checkTTSSupport promise caused the all-tests runner to believe it was OK to move on from this test to the next one, which caused this test to fail (0 assertions run) and then causing the next test in line to fail with an incorrect number of assertions, as the tests that were being run as a result of the promise resolving were then run as part of the next test context.

I resolved this by wrapping the promise-based structure in an asyncTest structure; this results in the intended outcome of the tests being run or skipped based on the results of the promise ("does the test environment truly support TTS?"), while also playing nicely with the all-tests environment.

amb26 · 2016-08-16T20:27:13Z

src/components/textToSpeech/js/TextToSpeech.js

@@ -142,6 +142,16 @@ var fluid_2_0_0 = fluid_2_0_0 || {};
        }
    });

+    // Issue functions to the speechSynthesis interface


Very short line wrapped comments - perhaps an IDE setting run amok?

amb26 · 2016-08-29T23:50:38Z

src/components/textToSpeech/js/TextToSpeech.js

+        }
+    };
+
+    fluid.textToSpeech.issueControlRequest = function (that, modelBoolPath, controlName, invert) {


This can be eliminated and the invokers replaced with a "changePath" record - http://docs.fluidproject.org/infusion/development/ChangeApplierAPI.html#declarative-style-for-triggering-a-change

merge upstream master

…t linting rule.

waharnum · 2016-09-01T13:00:36Z

@amb26 - I've implemented throttle-based control, as you recommended, and reduced the dependence on functions for managing control vs. model paths & model listeners. At this point we should decide if the changes could go in their current form for Infusion 2.0 or if further work is needed - I don't think I have more cycles to spend on this.

waharnum · 2016-09-01T13:41:20Z

I also want to flag the comment at #732 (comment) - if we remove the non-model events for pause/resume, we'll need to update the documentation as well to say those events have been removed and that the alternative is to use model listeners.

amb26 · 2016-09-01T17:26:37Z

src/components/textToSpeech/js/TextToSpeech.js

@@ -170,7 +242,11 @@ var fluid_2_0_0 = fluid_2_0_0 || {};
            paused: false
        };

-        if (!that.queue.length) {
+        if (that.queue.currentUtterance) {
+            that.queue.currentUtterance = undefined;


I actually meant to put it in the queue item, rather than to rebase the entire queue structure.
So, on line 280 we would have that.queue.push({text: text, utterance: toSpeak})
queue can then just go back to being a [] and the rest of the changes reverted

Agree this is better; I had to refactor the queue behaviour a little so that.queue.shift happens at handleEnd rather than handleStart or we run into the same premature garbage collection behaviour in Safari that we were seeing previously. I don't think this is a problematic change (it also lets handleStart simply be a changePath record rather than a function), but please let me know if you foresee an issue with that behavioural change.

amb26 · 2016-09-01T17:29:39Z

Thanks, @waharnum - changes look great, with a minor tweak. Thanks for flagging the required docs update - this change is ok at present since we haven't yet made a release with this component included.

waharnum · 2016-09-01T19:27:53Z

Thanks @amb26 - I'll make that change to the queue structure (and fix the lint), and do a separate docs pull request to remove the onPause/onResume events.

…leEnd, not handleStart.

waharnum · 2016-09-01T22:09:01Z

@amb26 / @jobara:

queue updated as suggested
parallel documentation pull at https://github.com/fluid-project/infusion-docs/pull/99/files (JIRA at https://issues.fluidproject.org/projects/FLUID/issues/FLUID-5936)

amb26 · 2016-09-02T15:56:12Z

Hey there @waharnum - thanks so much for the marathon work on this, and pushing it the extra mile to the end!

waharnum and others added 6 commits July 27, 2016 17:06

Merge pull request #6 from fluid-project/master

f3de9d5

merge upstream master

FLUID-5936: use correct API event handler for end of speech

8a0255f

FLUID-5936: add a comment about the tests currently being skipped in …

a66947d

…Chrome

FLUID-5936: rewrite tests to use the IoC framework

e85d86a

FLUID-5936: restructure tests to better handle variations in asynchro…

2dba2cf

…nous behaviour of TTS between browsers.

FLUID-5936: linting

03f3dd1

amb26 reviewed Aug 4, 2016
View reviewed changes

tests/component-tests/textToSpeech/js/TextToSpeechTests.js

}

};

// fluid.setLogging(fluid.logLevel.TRACE);

Copy link

Member

amb26 Aug 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray comment

waharnum added 5 commits August 5, 2016 07:33

FLUID-5936: potential fix for test failures on Windows

8be2d97

FLUID-5936: continuing the quest for a sequencing approach that will …

15b6cf6

…work on all browsers between OSX and Windows.

FLUID-5936: issue pause/resume commands in test with slight delay

a92d6ed

FLUID-5936: set volume to 0

9143c35

FLUID-5936: linting

53099b7

waharnum added 3 commits August 8, 2016 10:16

FLUID-5936: 50ms timeout for control interaction

d6077c5

FLUID-5936: move asynchronous command issuance into main component fr…

d05ac06

…om tests

FLUID-5936: reset volume to 0

3f162b6

FLUID-5936: use an asyncTest wrapper around the promise to prevent th…

5ca02a8

…e all-tests runner from not handling these tests properly

amb26 reviewed Aug 16, 2016
View reviewed changes

FLUID-5936: add a comment about setTimeout of asyncTest wrapper

13ba66a

amb26 reviewed Aug 29, 2016
View reviewed changes

waharnum and others added 3 commits August 30, 2016 08:09

Merge pull request #8 from fluid-project/master

543b95d

merge upstream master

Merge branch 'master' into FLUID-5936

71f116b

NOJIRA: add keyword-spacing rule to linter, correct two files for tha…

df09711

…t linting rule.

waharnum mentioned this pull request Aug 30, 2016

NOJIRA: add keyword-spacing rule to linter #741

Merged

waharnum and others added 9 commits August 30, 2016 08:37

Merge branch 'keyword-spacing' into FLUID-5936

98a0330

FLUID-5936: lintin

00bedf1

FLUID-5936: comment in accidental comment out.

e9cd164

FLUID-5936: place currentUtterance in queue structure with texts.

71bc4ec

FLUID-5936: add some tests for the currentUtterance implementation.

5b2c2da

FLUID-5936: remove onpause/onresume events

75f3de3

FLUID-5936: modelizing

f4d3152

FLUID-5936: implement throttle-based control

e9e6069

FLUID-5936: remove unneeded function.

265f3fe

amb26 reviewed Sep 1, 2016
View reviewed changes

waharnum added 2 commits September 1, 2016 17:36

FLUID-5936: store utterance as part of queue array.

839f735

FLUID-5936: refactor queue implementation to clear queue item on hand…

fe384eb

…leEnd, not handleStart.

waharnum mentioned this pull request Sep 1, 2016

FLUID-5950: remove events removed due to FLUID-5936; add documentation of model paths. fluid-project/infusion-docs#99

Merged

amb26 merged commit fe384eb into fluid-project:master Sep 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732

FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732

waharnum commented Aug 4, 2016

amb26 commented Aug 4, 2016 •

edited

Loading

amb26 Aug 4, 2016

waharnum commented Aug 4, 2016

waharnum commented Aug 5, 2016

amb26 commented Aug 5, 2016

waharnum commented Aug 5, 2016

waharnum commented Aug 8, 2016

waharnum commented Aug 15, 2016

waharnum commented Aug 15, 2016

waharnum commented Aug 15, 2016 •

edited

Loading

amb26 Aug 16, 2016

amb26 Aug 29, 2016

waharnum commented Sep 1, 2016

waharnum commented Sep 1, 2016

amb26 Sep 1, 2016

waharnum Sep 1, 2016

amb26 commented Sep 1, 2016

waharnum commented Sep 1, 2016

waharnum commented Sep 1, 2016

amb26 commented Sep 2, 2016

FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732

FLUID-5936: fix bug in fluid.textToSpeech.checkTTSSupport, rewrite tests to use IoC testing #732

Conversation

waharnum commented Aug 4, 2016

amb26 commented Aug 4, 2016 • edited Loading

amb26 Aug 4, 2016

Choose a reason for hiding this comment

waharnum commented Aug 4, 2016

waharnum commented Aug 5, 2016

amb26 commented Aug 5, 2016

waharnum commented Aug 5, 2016

waharnum commented Aug 8, 2016

waharnum commented Aug 15, 2016

waharnum commented Aug 15, 2016

waharnum commented Aug 15, 2016 • edited Loading

amb26 Aug 16, 2016

Choose a reason for hiding this comment

amb26 Aug 29, 2016

Choose a reason for hiding this comment

waharnum commented Sep 1, 2016

waharnum commented Sep 1, 2016

amb26 Sep 1, 2016

Choose a reason for hiding this comment

waharnum Sep 1, 2016

Choose a reason for hiding this comment

amb26 commented Sep 1, 2016

waharnum commented Sep 1, 2016

waharnum commented Sep 1, 2016

amb26 commented Sep 2, 2016

amb26 commented Aug 4, 2016 •

edited

Loading

waharnum commented Aug 15, 2016 •

edited

Loading