Add tests for braille commands.

Add tests for braille routing commands, fix chromevox and reenable background tests.

This adds a new mockFeedback class that can handle both speech and braille
output.  This class superseeds MockTts.

Existing cvox2 background tests that had bit-rotted because they were disabled
were fixed and reenabled.  Braille output was added where it makes sense.
A new end-to-end test for braille routing was added.

BUG=451362, 491242
Committed: https://crrev.com/9818bff801f0ff43c143361d2f0032392d124405
Cr-Commit-Position: refs/heads/master@{#346229}

[Peter Lundblad, 2015-08-21 12:30:27]

plundblad@chromium.org changed reviewers:
+ dmazzoni@chromium.org, dtseng@chromium.org

[Peter Lundblad, 2015-08-21 12:30:28]

Hi,

IN addition to the new mock, this fixes and reenables a few tests that had
bit-rotted. I haven't observed any flakiness anymore, but we won't know until it
is on all the bots.

[Peter Lundblad, 2015-08-21 12:30:58]

The CQ bit was checked by plundblad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-21 12:32:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302763002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302763002/100001

[commit-bot: I haz the power, 2015-08-21 13:10:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 13:10:00]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2015-08-24 19:05:07]

Love it!

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
(right):

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:130:
.expectSpeech('of test')
Do you think an expectSpeechAndBraille helper would be helpful since they're the
same often?

Alternatively it could take two arguments:

    .expectSpeechAndBraille(['Heading 2', 'foxtraut'], 'h2 foxtraut');

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
(right):

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:1: //
Copyright 2014 The Chromium Authors. All rights reserved.
2015?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:6: *
@fileoverview This file contains the |MockFeedback| class which is a
Could you document what happens to initial speech before the first match? For
example, "ChromeVox is enabled", etc. - will it be ignored? Will other
unexpected utterances after the first real match be ignored or will they be an
error?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:14: *
Expectaations can be added using the |expectSpeech| and |expectBraille|
Expectaations -> Expectations

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:42: *
@param {function=} opt_finishedCallback Called when all expectaions have
expectations

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:98: *
Install mock objects as |cvox.CrhomeVox.tts| and |cvox.ChromeVox.braille|
ChromeVox

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:103:
function MockTts() {}
I know this is legal syntax but it looks funny, like it could be an
unintentional error - how about blank lines before and after it, or else var
MockTts = new function();?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:119: * Adds
an expectation for a spoken utterance.
Or utterances?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:120: *
@param {...string} var_args
Document this, i.e. clarify that each argument is a separate utterance? Also,
note that this can be a regex?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:131:
describe: function() { return 'Speak \'' + text + '\''; }
Could this be toString()?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:191:
replay: function() {
Do you think it would be possible to raise an exception if replay() is never
called?

Maybe you could integrate it into the setup/teardown for a text fixture - assert
that replay() was called in TearDown...

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:209: *
@param {string} textSTring
textString

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:217:
callback: properties ? properties.endCallback : undefined});
Do you need to handle startCallback too?

If we wanted this to be used to assert earcons too, startCallback might be
useful

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:294: for
(var i = 0, candidate; candidate = pending[i]; ++i) {
This seems to allow additional utterances - for example if I expect "hello",
"world", it will match "hello", "alien", "world". Did you want to try to address
that or should I try to work that in when I rebase my change
(https://codereview.chromium.org/1295213003/) on top of this?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:320: *
Returns its first argumennt.  Moved out of the cosntructor to satisfy the
constructor

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
File
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback_test.unitjs
(right):

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback_test.unitjs:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
2015?

https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback_test.unitjs:110:
TEST_F('MockFeedbackUnitTest', 'expectAfterGoThrows', function() {
Should this be expectAfterReplayThrows?

[Peter Lundblad, 2015-08-24 23:05:42]

PTAL

dmazzoni@chromium.org writes:
> Love it!
> 
> 
>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> File
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs
> (right):
> 
>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
>
chrome/browser/resources/chromeos/chromevox/cvox2/background/background_test.extjs:130:
> .expectSpeech('of test')
> Do you think an expectSpeechAndBraille helper would be helpful since
> they're the same often?
> 
> Alternatively it could take two arguments:
> 
>      .expectSpeechAndBraille(['Heading 2', 'foxtraut'], 'h2 foxtraut');

I thought about that while writin this particular test, but looking at the tests
overall, braille and speech usually differ. We could easily add such a method if
it is deemed useful furthe rdown.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> File
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js
> (right):
> 
>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:1:
> // Copyright 2014 The Chromium Authors. All rights reserved.
> 2015?

Ah, done;)

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:6:
> * @fileoverview This file contains the |MockFeedback| class which is a
> Could you document what happens to initial speech before the first
> match? For example, "ChromeVox is enabled", etc. - will it be ignored?
> Will other unexpected utterances after the first real match be ignored
> or will they be an error?

Made the text about this mock being lean its own paragraph.  I think we might
want to
make it stricter when we've convinced ourselves that this wouldn't make the
tests
flaky (see discussion about your CL).

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:14:
> * Expectaations can be added using the |expectSpeech| and
> |expectBraille|
> Expectaations -> Expectations

done.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:42:
> * @param {function=} opt_finishedCallback Called when all expectaions
> have
> expectations

Another variant of spelling, fixed.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:98:
> * Install mock objects as |cvox.CrhomeVox.tts| and
> |cvox.ChromeVox.braille|
> ChromeVox

Sigh, done.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:103:
> function MockTts() {}
> I know this is legal syntax but it looks funny, like it could be an
> unintentional error - how about blank lines before and after it, or else
> var MockTts = new function();?

Changed to the var syntax with blank lines.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:119:
> * Adds an expectation for a spoken utterance.
> Or utterances?

Fixed.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:120:
> * @param {...string} var_args
> Document this, i.e. clarify that each argument is a separate utterance?
> Also, note that this can be a regex?

Done.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:131:
> describe: function() { return 'Speak \'' + text + '\''; }
> Could this be toString()?

Done.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:191:
> replay: function() {
> Do you think it would be possible to raise an exception if replay() is
> never called?
> 
> Maybe you could integrate it into the setup/teardown for a text fixture
> - assert that replay() was called in TearDown...

I actually had a tearDown method initially intended for this, but then I
realized
ath as it is set up now, tests that don't call replay will time out
and never call tearDown in the first place, so this seemed rather pointless.


>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:209:
> * @param {string} textSTring
> textString

Done.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:217:
> callback: properties ? properties.endCallback : undefined});
> Do you need to handle startCallback too?
> 
> If we wanted this to be used to assert earcons too, startCallback might
> be useful

Added startCallback support. It is not used, but I added a test for it.
I think adding earocn support is a good idea for the future.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:294:
> for (var i = 0, candidate; candidate = pending[i]; ++i) {
> This seems to allow additional utterances - for example if I expect
> "hello", "world", it will match "hello", "alien", "world". Did you want
> to try to address that or should I try to work that in when I rebase my
> change (https://codereview.chromium.org/1295213003/) on top of this?

See above about leanness. Let's continue that discussion and perhaps make it
more strict in a follow up cl. I'd like to avoid introducing another source
of flakiness when reenabling all of these tests.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback.js:320:
> * Returns its first argumennt.  Moved out of the cosntructor to satisfy
> the
> constructor

This function wasn't needed anymore, remvoed.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
> File
> chrome/browser/resources/chromeos/chromevox/testing/mock_feedback_test.unitjs
> (right):
> 
>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
>
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback_test.unitjs:1:
> // Copyright 2014 The Chromium Authors. All rights reserved.
> 2015?

Done.

>
https://codereview.chromium.org/1302763002/diff/100001/chrome/browser/resourc...
>
chrome/browser/resources/chromeos/chromevox/testing/mock_feedback_test.unitjs:110:
> TEST_F('MockFeedbackUnitTest', 'expectAfterGoThrows', function() {
> Should this be expectAfterReplayThrows?

Yes, it should.  replay was called 'go' for a while, but I changed my mind.
Done.

> https://codereview.chromium.org/1302763002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2015-08-25 20:42:54]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-08-25 20:42:55]

lgtm

[commit-bot: I haz the power, 2015-08-25 20:43:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302763002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302763002/120001

[commit-bot: I haz the power, 2015-08-25 20:54:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-25 20:54:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Peter Lundblad, 2015-08-25 21:16:32]

plundblad@chromium.org changed reviewers:
+ brettw@chromium.org

[Peter Lundblad, 2015-08-25 21:16:33]

+Brett for js2gtest.gni

[brettw, 2015-08-26 22:31:06]

https://codereview.chromium.org/1302763002/diff/120001/chrome/test/base/js2gt...
File chrome/test/base/js2gtest.gni (right):

https://codereview.chromium.org/1302763002/diff/120001/chrome/test/base/js2gt...
chrome/test/base/js2gtest.gni:39: inputs += invoker.extra_gen_files
What does this name mean? I read this as "extra generated files" but it seems to
be used as "extra input files".

Can you add documentation for this above the template (it's bad that the other
ones don't have this, but at least we can start now) and consider renaming it if
the comment doesn't make the selected name clear?

[Peter Lundblad, 2015-08-27 20:49:07]

@brettw: PTAL

brettw@chromium.org writes:
> 
>
https://codereview.chromium.org/1302763002/diff/120001/chrome/test/base/js2gt...
> File chrome/test/base/js2gtest.gni (right):
> 
>
https://codereview.chromium.org/1302763002/diff/120001/chrome/test/base/js2gt...
> chrome/test/base/js2gtest.gni:39: inputs += invoker.extra_gen_files
> What does this name mean? I read this as "extra generated files" but it
> seems to be used as "extra input files".

I renamed it to gen_include_files to be more specific and hopefully clearer.
It's input to the generation step.

> Can you add documentation for this above the template (it's bad that the
> other ones don't have this, but at least we can start now) and consider
> renaming it if the comment doesn't make the selected name clear?

Done, documented all arguments.

> https://codereview.chromium.org/1302763002/

-- 

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brettw, 2015-08-28 19:22:42]

gn build lgtm

[Peter Lundblad, 2015-08-28 20:41:05]

The CQ bit was checked by plundblad@chromium.org

[Peter Lundblad, 2015-08-28 20:41:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1302763002/#ps140001
(title: "Document gn template args, rename extra_gen_files to
gen_include_files")

[commit-bot: I haz the power, 2015-08-28 20:41:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1302763002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1302763002/140001

[commit-bot: I haz the power, 2015-08-28 20:47:53]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-08-28 20:48:35]

Patchset 8 (id:??) landed as
https://crrev.com/9818bff801f0ff43c143361d2f0032392d124405
Cr-Commit-Position: refs/heads/master@{#346229}