Fix ChromeVox next tests to fail instead of timing out where applicable.

Fix ChromeVox next tests to fail instead of timing out where applicable.

When calling an sync method in a test, you need to wrap the passed
callback in continueTest that will call testDone if the callback stops
aunexpectedly. This makes developing easier since the tests report
immediately instead of having to wait for a timeout.

R=dtseng@chromium.org
BUG=None

Committed: https://crrev.com/87608bd0935bfdb37b8ad490c68b795c55ffd18d
Cr-Commit-Position: refs/heads/master@{#320711}

[David Tseng, 2015-02-18 17:27:40]

I'm not sure this is the way I'd fix the cursors test. I'd perhaps try to
provide an option to mock_tts to be strict. Are you seeing these tests time out?

https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js
(left):

https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js:29:
GEN('#include "base/command_line.h"');
Thanks for removing this.

https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
File
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js
(right):

https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js:28:
}.bind(this)));
Doesn't seem like a good place to assume the caller wants a sync test; most
tests won't want this as they'll be waiting for speech.

[Peter Lundblad, 2015-02-19 14:23:12]

dtseng@chromium.org writes:
> I'm not sure this is the way I'd fix the cursors test. I'd perhaps try to
> provide an option to mock_tts to be strict. Are you seeing these tests time  
> out?

No, they don't time out. This CL don't fix any failures, but just makes
failures be reported immediately instead of causing the tests to hang and
eventuall
time out.  The change to the cursors test is just chaning from manually doing
runWithDocument -> chrome.automation.getTree to using the convenience
function provided by the test fixture base class.  Since the base method
was fixed to report failures properly, we get that as a bonus instead of having
to do it here as well.  Do you still have a problem with this? ;)

>
https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
> File
>
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js
> (left):
> 
>
https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js:29:
> GEN('#include "base/command_line.h"');
> Thanks for removing this.
> 
>
https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
> File
>
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js
> (right):
> 
>
https://codereview.chromium.org/938623003/diff/1/chrome/browser/resources/chr...
>
chrome/browser/resources/chromeos/chromevox/testing/chromevox_next_e2e_test_base.js:28:
> }.bind(this)));
> Doesn't seem like a good place to assume the caller wants a sync test;
> most tests won't want this as they'll be waiting for speech.

What makes you believe this?  continueTest creatse a wrapper
for a callback that will report failures. The WhenTestDone specifies
when testDone should be called by the wrapper. Setting that to ASSERT means
tha fatal failures will be reported after the wrapped callback is done, while
assuming that the callback will take care of running testDone, perhaps from
another
callback.  As I wrote in the CL description, we should call continueTest
every time w make an async call.  See test_api.js for more details.

Hope this clarifies,
//Peter

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

-- 

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

[David Tseng, 2015-02-19 19:22:25]

I think this is a good change, given test_api.js.

Thinking about it a little more, given any assertion, I'd expect testDone to get
called on failure immediately. At least in native gtest fixtures, that's the
distinction between assert and expect macros.

What do you think about wrapping all of our asserts to call testDone on failure?

[Peter Lundblad, 2015-02-20 10:31:28]

dtseng@chromium.org writes:
> I think this is a good change, given test_api.js.
> 
> Thinking about it a little more, given any assertion, I'd expect testDone  
> to get
> called on failure immediately. At least in native gtest fixtures, that's the
> distinction between assert and expect macros.
> 
> What do you think about wrapping all of our asserts to call testDone on  
> failure?

The way this works is that all assert functions (including the once we have
in assert_additions.js) throw Error objects. This typically causes
the test to finish early, as per the usual assert* semantics. Then, the
callback
created by continueTest will catch Errors, add them to the list of errors
for the test and call testDone(). I don't see what value making the asserts
call testDone direclty would add since:
- The asserts would have to throw anyways to cause the test to stop as
  required.  Otherwise, we'll probably have the test continue for a while
  until chrome gets a chance to kill it leading to misleading spurious
  errors afte the assert, making debugging harder.
- We need a callback wrapper anyways to take care of exceptions not caused by
  the asserts, such as mock failures and genuine unexpected exceptions
  caused by the code under test.

Perhaps it is possible to install some custom handler for unhandled
exceptions and halt the test instead of having to wrap the
callbacks.  I don't know how hard that would be and the benefit is not
clear to me (except for not having to wrap the completion callbacks
for async calls).

Thanks,
//Peter

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

[David Tseng, 2015-02-20 16:46:12]

Let me clarify by suggesting what I want:
assert* throws an exception and we fail the test immediately (see below on
how to do this).

Advantages:
- works with all assert* without installing custom wrappers everywhere
(including possibly classic tests).
- no need to even finish the current function.
- no need to pass around wrapped callbacks that call testDone.


I think (maybe?) one potential problem the author was trying to solve by
introducing this complexity was how to handle exceptions thrown within a
callback.
If you try/catch something like
try {
  chrome.automation.getDesktop(function(r) {
      throw 'exception!';
  });
} catch(e) {
  // Never reached.
}

your exception is actually uncaught.

I just looked this one up and if you assign
window.onerror
you can catch all exceptions thrown on a page.

So, assuming you have a full browser (which we do), you can just fail the
test immediately when you see an exception of any kind. So, this change
should maybe assign window.onerror (checking for the
presence of window since some v8 tests use test_api) and fail immediately.


On Fri, Feb 20, 2015 at 2:31 AM, <plundblad@chromium.org> wrote:

> dtseng@chromium.org writes:
> > I think this is a good change, given test_api.js.
> >
> > Thinking about it a little more, given any assertion, I'd expect testDone
> > to get
> > called on failure immediately. At least in native gtest fixtures, that's
> the
> > distinction between assert and expect macros.
> >
> > What do you think about wrapping all of our asserts to call testDone on
> > failure?
>
> The way this works is that all assert functions (including the once we have
> in assert_additions.js) throw Error objects. This typically causes
> the test to finish early, as per the usual assert* semantics. Then, the
> callback
> created by continueTest will catch Errors, add them to the list of errors
> for the test and call testDone(). I don't see what value making the asserts
> call testDone direclty would add since:
> - The asserts would have to throw anyways to cause the test to stop as
>   required.  Otherwise, we'll probably have the test continue for a while
>   until chrome gets a chance to kill it leading to misleading spurious
>   errors afte the assert, making debugging harder.
> - We need a callback wrapper anyways to take care of exceptions not caused
> by
>   the asserts, such as mock failures and genuine unexpected exceptions
>   caused by the code under test.
>
> Perhaps it is possible to install some custom handler for unhandled
> exceptions and halt the test instead of having to wrap the
> callbacks.  I don't know how hard that would be and the benefit is not
> clear to me (except for not having to wrap the completion callbacks
> for async calls).
>
> Thanks,
> //Peter
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

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

[Peter Lundblad, 2015-03-13 16:01:06]

PTAL

Hi,

To make our code more streamlined, I added a utility class that is
used in our test fixutres. Now we just have to wrap callbacks
that are handed to async functions in another callback that will consistently
call testDone() when all outstanding callbacks have been called. This is
similar
to what we have in the chrome.testing API.

Re. window.onerror:
extension APIs swallow exceptions and log them without calling this
event handler. See crbug.com/357568.

I've fixed all our async tests to use this helper, finding a minor bug that was
masked by testDone being called too early before.

Let me know what you think,
//Peter

David Tseng writes:
> Let me clarify by suggesting what I want:
> assert* throws an exception and we fail the test immediately (see below on
how
> to do this).
> 
> Advantages:
> - works with all assert* without installing custom wrappers everywhere
> (including possibly classic tests).
> - no need to even finish the current function.
> - no need to pass around wrapped callbacks that call testDone.
> 
> I think (maybe?) one potential problem the author was trying to solve by
> introducing this complexity was how to handle exceptions thrown within a
> callback.
> If you try/catch something like
> try {
>   chrome.automation.getDesktop(function(r) {
>       throw 'exception!';
>   });
> } catch(e) {
>   // Never reached.
> }
> 
> your exception is actually uncaught.
> 
> I just looked this one up and if you assign
> window.onerror
> you can catch all exceptions thrown on a page.
> 
> So, assuming you have a full browser (which we do), you can just fail the
test
> immediately when you see an exception of any kind. So, this change should
> maybe assign window.onerror (checking for the 
> presence of window since some v8 tests use test_api) and fail immediately.
> 
> On Fri, Feb 20, 2015 at 2:31 AM, <plundblad@chromium.org> wrote:
> 
>     dtseng@chromium.org writes:
>     > I think this is a good change, given test_api.js.
>     >
>     > Thinking about it a little more, given any assertion, I'd expect
>     testDone
>     > to get
>     > called on failure immediately. At least in native gtest fixtures,
that's
>     the
>     > distinction between assert and expect macros.
>     >
>     > What do you think about wrapping all of our asserts to call testDone on
>     > failure?
>    
>     The way this works is that all assert functions (including the once we
>     have
>     in assert_additions.js) throw Error objects. This typically causes
>     the test to finish early, as per the usual assert* semantics. Then, the
>     callback
>     created by continueTest will catch Errors, add them to the list of errors
>     for the test and call testDone(). I don't see what value making the
>     asserts
>     call testDone direclty would add since:
>     - The asserts would have to throw anyways to cause the test to stop as
>       required.  Otherwise, we'll probably have the test continue for a while
>       until chrome gets a chance to kill it leading to misleading spurious
>       errors afte the assert, making debugging harder.
>     - We need a callback wrapper anyways to take care of exceptions not
caused
>     by
>       the asserts, such as mock failures and genuine unexpected exceptions
>       caused by the code under test.
>    
>     Perhaps it is possible to install some custom handler for unhandled
>     exceptions and halt the test instead of having to wrap the
>     callbacks.  I don't know how hard that would be and the benefit is not
>     clear to me (except for not having to wrap the completion callbacks
>     for async calls).
>    
>     Thanks,
>     //Peter
>    
>     To unsubscribe from this group and stop receiving emails from it, send an
>     email to chromium-reviews+unsubscribe@chromium.org.
> 

-- 

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

[David Tseng, 2015-03-13 22:20:24]

lgtm

[Peter Lundblad, 2015-03-16 11:45:39]

The CQ bit was checked by plundblad@chromium.org

[commit-bot: I haz the power, 2015-03-16 11:45:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938623003/40001

[commit-bot: I haz the power, 2015-03-16 12:09:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-16 12:10:14]

Patchset 3 (id:??) landed as
https://crrev.com/87608bd0935bfdb37b8ad490c68b795c55ffd18d
Cr-Commit-Position: refs/heads/master@{#320711}