Move TranslateManager unit tests out of the browser_tests target.

Move TranslateManager unit tests out of the browser_tests target.

These are unit tests so they should be in the unit_tests target. A
TODO says this did not work before, but they appear to pass as unit
tests now, so this is just a code move.

BUG=270918
R=droger@chromium.org, hajimehoshi@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255661

[sky, 2014-03-03 23:07:02]

-sky
+hajimehoshi

[hajimehoshi, 2014-03-04 02:35:57]

+droger, miguelg (CC)

Thank you for your refactoring!

https://codereview.chromium.org/186063007/diff/40001/chrome/browser/translate...
File chrome/browser/translate/translate_manager_unittest.cc (right):

https://codereview.chromium.org/186063007/diff/40001/chrome/browser/translate...
chrome/browser/translate/translate_manager_unittest.cc:93: #if
!defined(OS_ANDROID)
Is it possible to execute these tests on Android?

[droger, 2014-03-04 09:00:39]

This does not look like a unittest of TranslateManager, but rather some kind of
integration test.
I'd rather have a different file name.
Maybe translate_manager_render_view_host_unittest, to match the actual name of
the tests?

[danakj, 2014-03-04 15:39:23]

https://codereview.chromium.org/186063007/diff/40001/chrome/browser/translate...
File chrome/browser/translate/translate_manager_unittest.cc (right):

https://codereview.chromium.org/186063007/diff/40001/chrome/browser/translate...
chrome/browser/translate/translate_manager_unittest.cc:93: #if
!defined(OS_ANDROID)
On 2014/03/04 02:35:57, hajimehoshi wrote:
> Is it possible to execute these tests on Android?

No, RenderViewContextMenu does not get compiled into the android build. There
may be other problems as well.

[danakj, 2014-03-04 15:48:16]

PTAL

On Tue, Mar 4, 2014 at 4:00 AM, <droger@chromium.org> wrote:

> This does not look like a unittest of TranslateManager, but rather some
> kind of
> integration test.
> I'd rather have a different file name.
> Maybe translate_manager_render_view_host_unittest, to match the actual
> name of
> the tests?
>

Sure, done.

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

[droger, 2014-03-04 15:55:39]

On 2014/03/04 15:48:16, danakj wrote:
> Sure, done.

Thanks!

[droger, 2014-03-04 15:58:33]

lgtm

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:5:
#include "chrome/browser/translate/translate_manager.h"
Nit: this should not be at the top.

[danakj, 2014-03-04 16:01:51]

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:5:
#include "chrome/browser/translate/translate_manager.h"
On 2014/03/04 15:58:33, droger wrote:
> Nit: this should not be at the top.

I thought it should, since this file is testing the translate manager? 

"In dir/foo.cc or dir/foo_test.cc, whose main purpose is to implement or test
the stuff in dir2/foo2.h, order your includes as follows:

dir2/foo2.h (preferred location — see details below).
C system files.
C++ system files.
Other libraries' .h files.
Your project's .h files."

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

[droger, 2014-03-04 16:10:24]

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:5:
#include "chrome/browser/translate/translate_manager.h"
My opinion is that this file is not testing the TranslateManager API
(TranslateManager is almost not used in this file), and thus is not a unit test
for TranslateManager.
It is more of a general integration test for translate.

I won't block the CL on this though

[danakj, 2014-03-04 16:12:21]

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/186063007/diff/90004/chrome/browser/translate...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:5:
#include "chrome/browser/translate/translate_manager.h"
On 2014/03/04 16:10:25, droger wrote:
> My opinion is that this file is not testing the TranslateManager API
> (TranslateManager is almost not used in this file), and thus is not a unit
test
> for TranslateManager.
> It is more of a general integration test for translate.
> 
> I won't block the CL on this though

OK, removed.

[hajimehoshi, 2014-03-04 16:28:12]

lgtm

[danakj, 2014-03-04 16:39:06]

Thanks!

[danakj, 2014-03-04 16:39:26]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-04 16:39:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-04 21:13:28]

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

[commit-bot: I haz the power, 2014-03-04 21:13:29]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[danakj, 2014-03-04 21:15:04]

InMemoryURLIndexTest.Retrieval keeps failing which is not one of the tests I
moved, trying again.

[danakj, 2014-03-04 21:15:07]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:17:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-04 21:22:32]

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

[commit-bot: I haz the power, 2014-03-04 21:22:32]

Try jobs failed on following builders: linux_chromium_rel

[danakj, 2014-03-04 21:23:22]

On 2014/03/04 21:22:32, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders: linux_chromium_rel

That bot fails a different ComponentUpdaterPingManagerTest.PingManagerTest.
Still trying again.

[danakj, 2014-03-04 21:23:25]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:27:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-04 21:32:26]

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

[commit-bot: I haz the power, 2014-03-04 21:32:26]

Try jobs failed on following builders: linux_chromium_rel

[Paweł Hajdan Jr., 2014-03-04 21:48:01]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:53:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-04 21:56:36]

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

[commit-bot: I haz the power, 2014-03-04 21:56:37]

Try jobs failed on following builders: linux_chromium_rel

[Paweł Hajdan Jr., 2014-03-04 22:12:16]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-03-04 22:14:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-04 22:55:13]

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

[commit-bot: I haz the power, 2014-03-04 22:55:13]

Try jobs failed on following builders: linux_chromium_rel

[danakj, 2014-03-05 00:16:21]

Somehow these tests end up causing the
ComponentUpdaterPingManagerTest.PingManagerTest test to crash:

[ RUN      ] ComponentUpdaterPingManagerTest.PingManagerTest
[6207:6207:0304/190940:359764225816:FATAL:url_fetcher_core.cc(390)] Check
failed: network_task_runner_->BelongsToCurrentThread(). 
 [0x7f5fa11060ce] base::debug::StackTrace::StackTrace()
 [0x7f5fa1188e75] logging::LogMessage::~LogMessage()
 [0x7f5fa0b2ee65] net::URLFetcherCore::OnResponseStarted()
 [0x7f5fa0b414da] net::URLRequest::NotifyResponseStarted()

Even if this test is run before the TranslateManager tests.

[danakj, 2014-03-05 19:31:40]

On 2014/03/05 00:16:21, danakj wrote:
> Somehow these tests end up causing the
> ComponentUpdaterPingManagerTest.PingManagerTest test to crash:
> 
> [ RUN      ] ComponentUpdaterPingManagerTest.PingManagerTest
> [6207:6207:0304/190940:359764225816:FATAL:url_fetcher_core.cc(390)] Check
> failed: network_task_runner_->BelongsToCurrentThread(). 
>  [0x7f5fa11060ce] base::debug::StackTrace::StackTrace()
>  [0x7f5fa1188e75] logging::LogMessage::~LogMessage()
>  [0x7f5fa0b2ee65] net::URLFetcherCore::OnResponseStarted()
>  [0x7f5fa0b414da] net::URLRequest::NotifyResponseStarted()
> 
> Even if this test is run before the TranslateManager tests.

Looks like this test was broken before this CL, I can reproduce the same crash
on trunk. Filed https://code.google.com/p/chromium/issues/detail?id=349547 about
it.

[danakj, 2014-03-05 19:42:48]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-05 19:47:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-05 20:04:52]

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

[commit-bot: I haz the power, 2014-03-05 20:04:53]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[danakj, 2014-03-05 20:07:49]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-05 20:08:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-05 23:56:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/110001

[commit-bot: I haz the power, 2014-03-06 01:08:49]

Change committed as 255192

[danakj, 2014-03-06 20:06:28]

Reopening, looks like a test in here is not too flaky for the CQ, but is for the
memory waterfall.

[danakj, 2014-03-06 21:54:16]

PTAL 

The latest patch resolves one test causing the next to fail by adding
TranslateService::InitializeForTesting() and
TranslateService::ShutdownForTesting(). Before one test causing
TranslateDownloadManager::Shutdown() to happen meant that no other test could
use the manager.

[hajimehoshi, 2014-03-07 03:54:49]

Does this problem happen only on unit tests?

lgtm with nits

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nits: no (c)

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:46: // This path is only used by
tests.
Can you change this comment?

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:53: if (!g_translate_service)
nits: {}

[droger, 2014-03-07 08:08:28]

lgtm

[danakj, 2014-03-07 16:29:52]

> Does this problem happen only on unit tests?

AFAIK yes. This happens on unit tests because we run each test one after the
other in the same process.

Browser tests launch a full browser and tear it down, so the cleanup there is, I
think, a bit more rigorous between tests.

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_manager_render_view_host_unittest.cc
(right):

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_manager_render_view_host_unittest.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/03/07 03:54:50, hajimehoshi wrote:
> nits: no (c)

Done.

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:46: // This path is only used by
tests.
On 2014/03/07 03:54:50, hajimehoshi wrote:
> Can you change this comment?

Sure, though I'm not sure what to change it to. I think this comment's truth is
unaffected by this change. It's just used by a different set of tests (browser
tests).

Since we have high latency, I'll CQ this but I'm happy to land a followup to
update the comment if you let me know what it should say.

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:53: if (!g_translate_service)
On 2014/03/07 03:54:50, hajimehoshi wrote:
> nits: {}

oops, thanks. Done.

[danakj, 2014-03-07 16:30:23]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/186063007/170001

[hajimehoshi, 2014-03-07 16:56:37]

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:46: // This path is only used by
tests.
On 2014/03/07 16:29:53, danakj wrote:
> On 2014/03/07 03:54:50, hajimehoshi wrote:
> > Can you change this comment?
> 
> Sure, though I'm not sure what to change it to. I think this comment's truth
is
> unaffected by this change. It's just used by a different set of tests (browser
> tests).
> 
> Since we have high latency, I'll CQ this but I'm happy to land a followup to
> update the comment if you let me know what it should say.

Sure. However, it might be confusing that this comment mentions 'test' while
both Shutdown and ShutdownForTesting exist. How about s/by tests/by browser
tests/ ?

[danakj, 2014-03-07 17:08:28]

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
File chrome/browser/translate/translate_service.cc (right):

https://codereview.chromium.org/186063007/diff/150001/chrome/browser/translat...
chrome/browser/translate/translate_service.cc:46: // This path is only used by
tests.
On 2014/03/07 16:56:38, hajimehoshi wrote:
> On 2014/03/07 16:29:53, danakj wrote:
> > On 2014/03/07 03:54:50, hajimehoshi wrote:
> > > Can you change this comment?
> > 
> > Sure, though I'm not sure what to change it to. I think this comment's truth
> is
> > unaffected by this change. It's just used by a different set of tests
(browser
> > tests).
> > 
> > Since we have high latency, I'll CQ this but I'm happy to land a followup to
> > update the comment if you let me know what it should say.
> 
> Sure. However, it might be confusing that this comment mentions 'test' while
> both Shutdown and ShutdownForTesting exist. How about s/by tests/by browser
> tests/ ?

Got it!

[danakj, 2014-03-07 18:10:56]

Committed patchset #9 manually as r255661 (presubmit successful).