Fix tests flakes.

Fix tests flakes.

BUG=623727
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/258b0c4bcf5b0d277ee34d883f69ba15219f29e3
Cr-Commit-Position: refs/heads/master@{#403227}

[David Tseng, 2016-06-30 01:01:11]

Description was changed from

==========
Fix tests flakes.
==========

to

==========
Fix tests flakes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-06-30 01:23:09]

Description was changed from

==========
Fix tests flakes.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix tests flakes.

BUG=623727
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[David Tseng, 2016-06-30 01:23:09]

dtseng@chromium.org changed reviewers:
+ kbr@chromium.org

[David Tseng, 2016-06-30 01:23:42]

Thanks for alerting me to this flake. Let's see if this helps.

[Ken Russell (switch to Gerrit), 2016-06-30 01:45:03]

Thanks David for your quick attention to this!

I don't know the semantics of these tests, nor the CallbackHelper class. That
having been said, it's not obvious to me why this CL would change the behavior.
In this tryjob run:

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

the same assertion failure is still happening:

[21287:21287:0629/182944:FATAL:keep_alive_registry.cc(53)] Check failed:
!g_browser_process->IsShuttingDown(). 

but it's not causing the tests to fail any more. Is that expected?

Is there another possible fix? dgn@ suggested to run pending tasks on
TearDown(). Is there a way to invoke this from these JavaScript tests? Or is
there a way to call base::RunLoop::QuitWhenIdle()? Would that have the desired
behavior?

Alternatively, could notifications be disabled if they're not relevant to these
tests?

[David Tseng, 2016-06-30 02:38:36]

Probably sent prematurely; the test failures suggested a framework level
issue with the js gtests.

Patchset two addresses the notification listener issue directly; I'll keep
an eye on the try bots to see if it clears up the error in question.


On Wed, Jun 29, 2016 at 6:45 PM, <kbr@chromium.org> wrote:

> Thanks David for your quick attention to this!
>
> I don't know the semantics of these tests, nor the CallbackHelper class.
> That
> having been said, it's not obvious to me why this CL would change the
> behavior.
> In this tryjob run:
>
> https://build.chromium.org/p/tryserver.chromium.linux/
> builders/linux_chromium_chromeos_ozone_rel_ng/builds/
> 194480/steps/chromevox_tests%20%28with%20patch%29/logs/stdio
>
> the same assertion failure is still happening:
>
> [21287:21287:0629/182944:FATAL:keep_alive_registry.cc(53)] Check failed:
> !g_browser_process->IsShuttingDown().
>
> but it's not causing the tests to fail any more. Is that expected?
>
> Is there another possible fix? dgn@ suggested to run pending tasks on
> TearDown(). Is there a way to invoke this from these JavaScript tests? Or
> is
> there a way to call base::RunLoop::QuitWhenIdle()? Would that have the
> desired
> behavior?
>
> Alternatively, could notifications be disabled if they're not relevant to
> these
> tests?
>
>
> https://codereview.chromium.org/2103053006/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2016-06-30 18:08:27]

Thanks for continuing to push on this.

Patch set 3 looks good; no assertion failures in the try jobs.


On 2016/06/30 02:38:36, David Tseng wrote:
> Probably sent prematurely; the test failures suggested a framework level
> issue with the js gtests.
> 
> Patchset two addresses the notification listener issue directly; I'll keep
> an eye on the try bots to see if it clears up the error in question.
> 
> 
> On Wed, Jun 29, 2016 at 6:45 PM, <mailto:kbr@chromium.org> wrote:
> 
> > Thanks David for your quick attention to this!
> >
> > I don't know the semantics of these tests, nor the CallbackHelper class.
> > That
> > having been said, it's not obvious to me why this CL would change the
> > behavior.
> > In this tryjob run:
> >
> > https://build.chromium.org/p/tryserver.chromium.linux/
> > builders/linux_chromium_chromeos_ozone_rel_ng/builds/
> > 194480/steps/chromevox_tests%20%28with%20patch%29/logs/stdio
> >
> > the same assertion failure is still happening:
> >
> > [21287:21287:0629/182944:FATAL:keep_alive_registry.cc(53)] Check failed:
> > !g_browser_process->IsShuttingDown().
> >
> > but it's not causing the tests to fail any more. Is that expected?
> >
> > Is there another possible fix? dgn@ suggested to run pending tasks on
> > TearDown(). Is there a way to invoke this from these JavaScript tests? Or
> > is
> > there a way to call base::RunLoop::QuitWhenIdle()? Would that have the
> > desired
> > behavior?
> >
> > Alternatively, could notifications be disabled if they're not relevant to
> > these
> > tests?
> >
> >
> > https://codereview.chromium.org/2103053006/
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2016-06-30 18:18:28]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2016-06-30 18:18:28]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-06-30 18:18:29]

lgtm

I think David's traveling by now. Let's land this change.

[commit-bot: I haz the power, 2016-06-30 18:19:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-30 18:26:41]

Description was changed from

==========
Fix tests flakes.

BUG=623727
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix tests flakes.

BUG=623727
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-30 18:26:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-30 18:28:26]

Description was changed from

==========
Fix tests flakes.

BUG=623727
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix tests flakes.

BUG=623727
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/258b0c4bcf5b0d277ee34d883f69ba15219f29e3
Cr-Commit-Position: refs/heads/master@{#403227}
==========

[commit-bot: I haz the power, 2016-06-30 18:28:27]

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

[Ken Russell (switch to Gerrit), 2016-06-30 18:28:54]

On 2016/06/30 18:18:29, dmazzoni wrote:
> lgtm
> 
> I think David's traveling by now. Let's land this change.

Aweseme. Thanks David and Dominic. This was a significant source of flakes on
the CQ and it's great that they'll be resolved.