Deflake CommandLineTest

Delete CommandLineTest

Test is racy by design, and testing this properly is not
worth the effort required to add the proper hooks into
initialization.

BUG=

Committed: https://crrev.com/d6e13ee19aa46617da45c49289e9b658544c6e1d
Cr-Commit-Position: refs/heads/master@{#319083}

[boliu, 2015-03-03 00:15:26]

boliu@chromium.org changed reviewers:
+ torne@chromium.org

[boliu, 2015-03-03 00:15:26]

ptal

Failed 3 times in a row here:
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

Been flaking in other runs too, but our instrumentation script retries tests up
to 3 times so they don't turn the bots red.

Also I gave up trying to trace instrumentation code and see synchronization
between activity start and the test method, but I'm guessing there's none.

[Torne, 2015-03-03 10:53:50]

I think the right thing to do to ensure the Application has been created is to
wait for one of the actual components of the app to be created. Typically this
is by doing getActivity(), which does not return until the Activity's onCreate
has finished (which is, in turn, not called until the Application's onCreate has
finished). Will that work here?

Separately: Dianne strongly advocates not having a custom Application subclass,
and instead storing any global singleton stuff as just normal standalone java
singletons, which would also solve this problem. I realise we can't just get rid
of AwShellApplication because the use of a custom Application object is used in
several layers of chromium, but maybe someone should think about that at some
point :)

[Torne, 2015-03-03 10:55:13]

Actually, that might also be wrong. There is an "ApplicationTestCase" in
instrumentation specifically for testing stuff related to the Application life
cycle. That's probably the right approach :)

[boliu, 2015-03-03 15:03:25]

On 2015/03/03 10:53:50, Torne wrote:
> I think the right thing to do to ensure the Application has been created is to
> wait for one of the actual components of the app to be created. Typically this
> is by doing getActivity(), which does not return until the Activity's onCreate
> has finished (which is, in turn, not called until the Application's onCreate
has
> finished). Will that work here?

Tried that. Does *not* work.

If I was reading the instrumentation code correctly (big if), getActivity only
guarantee onCreate is scheduled, not finished.

On 2015/03/03 10:55:13, Torne wrote:
> Actually, that might also be wrong. There is an "ApplicationTestCase" in
> instrumentation specifically for testing stuff related to the Application life
> cycle. That's probably the right approach :)

Will look into this more today.

[Torne, 2015-03-03 15:08:28]

On 2015/03/03 15:03:25, boliu wrote:
> On 2015/03/03 10:53:50, Torne wrote:
> > I think the right thing to do to ensure the Application has been created is
to
> > wait for one of the actual components of the app to be created. Typically
this
> > is by doing getActivity(), which does not return until the Activity's
onCreate
> > has finished (which is, in turn, not called until the Application's onCreate
> has
> > finished). Will that work here?
> 
> Tried that. Does *not* work.
> 
> If I was reading the instrumentation code correctly (big if), getActivity only
> guarantee onCreate is scheduled, not finished.

That really doesn't seem like it should be the case, I can't see how any
instrumentation test could work if so :/

> On 2015/03/03 10:55:13, Torne wrote:
> > Actually, that might also be wrong. There is an "ApplicationTestCase" in
> > instrumentation specifically for testing stuff related to the Application
life
> > cycle. That's probably the right approach :)
> 
> Will look into this more today.

[Torne, 2015-03-03 15:08:28]

On 2015/03/03 15:03:25, boliu wrote:
> On 2015/03/03 10:53:50, Torne wrote:
> > I think the right thing to do to ensure the Application has been created is
to
> > wait for one of the actual components of the app to be created. Typically
this
> > is by doing getActivity(), which does not return until the Activity's
onCreate
> > has finished (which is, in turn, not called until the Application's onCreate
> has
> > finished). Will that work here?
> 
> Tried that. Does *not* work.
> 
> If I was reading the instrumentation code correctly (big if), getActivity only
> guarantee onCreate is scheduled, not finished.

That really doesn't seem like it should be the case, I can't see how any
instrumentation test could work if so :/

> On 2015/03/03 10:55:13, Torne wrote:
> > Actually, that might also be wrong. There is an "ApplicationTestCase" in
> > instrumentation specifically for testing stuff related to the Application
life
> > cycle. That's probably the right approach :)
> 
> Will look into this more today.

[boliu, 2015-03-04 00:11:58]

Ok, let's ignore the test runner part for now. AFAIT this test is racy by design
(or maybe things have changed since this was written)

So test goes:
1) Verify CommandLine instance is java, and append a switch
2) AwBrowserProcess.loadLibrary(), this sets the native CommandLine instance
3) Verify CommandLine instance is native, and the switch from 1) is still there

And AwShellApplication.onCreate goes (leaving out irrelevant steps):
a) CommandLine.initFromFile, this sets the java CommandLine instance
b) AwBrowserProcess.loadLibrary(), this sets the native CommandLine instance

Which means the only way this test passes is if 1) happens between a) and b). So
the test is only passing because AwBrowserProcess.loadLibrary() happens to take
a long time, and I can make the test fail by just sleeping for 1s at the
beginning of the test. Waiting for Application.onCreate to finish would have
just made the test fail for the same reason.

I'm tempted to just remove this test. There is no way to really test this
without adding more hooks into library loading and native init.

-----------------

Now test runners..

On 2015/03/03 15:08:28, Torne wrote:
> > If I was reading the instrumentation code correctly (big if), getActivity
only
> > guarantee onCreate is scheduled, not finished.
> 
> That really doesn't seem like it should be the case, I can't see how any
> instrumentation test could work if so :/

Actual testing shows this Application.onCreate is clearly not finished on main
thread by the time getActivity returns. But I think in general it's ok because
tests are not supposed to touch the application/activity/whatnot on the
instrumentation thread anyway, and posting and wait on main thread seems to be
good enough to avoid the race.

Tried ApplicationTestCase. Apparently an instance of the application is created
on the main thread even if I don't call createApplication (wat?). Then calling
createApplication creates the application on the *instrumentation thread* (wat?
x2). The createApplication call is synchronous, which doesn't really help much
with this test anyway.

I think I've sunk too much time into this already. Initialization is hard :/,
and android test classes are weird.

[Torne, 2015-03-04 11:46:05]

On 2015/03/04 00:11:58, boliu wrote:
> Ok, let's ignore the test runner part for now. AFAIT this test is racy by
design
> (or maybe things have changed since this was written)
> 
> So test goes:
> 1) Verify CommandLine instance is java, and append a switch
> 2) AwBrowserProcess.loadLibrary(), this sets the native CommandLine instance
> 3) Verify CommandLine instance is native, and the switch from 1) is still
there
> 
> And AwShellApplication.onCreate goes (leaving out irrelevant steps):
> a) CommandLine.initFromFile, this sets the java CommandLine instance
> b) AwBrowserProcess.loadLibrary(), this sets the native CommandLine instance
> 
> Which means the only way this test passes is if 1) happens between a) and b).
So
> the test is only passing because AwBrowserProcess.loadLibrary() happens to
take
> a long time, and I can make the test fail by just sleeping for 1s at the
> beginning of the test. Waiting for Application.onCreate to finish would have
> just made the test fail for the same reason.
> 
> I'm tempted to just remove this test. There is no way to really test this
> without adding more hooks into library loading and native init.

Agreed. This test seems to be testing something that we can't reliably determine
without adding weird test-only behaviour, and people are actually working on
refactoring LibraryLoader to be less of a confusing mess in the first place
which may well end up removing this webview-specific special case anyway. I
think it's safe to disable/remove the test.

As for the rest.. I guess the android test runners are weird and I don't
understand them :/

[boliu, 2015-03-04 15:01:46]

then stamp pls? PS2 already removes the whole file

[Torne, 2015-03-04 15:39:10]

On 2015/03/04 15:01:46, boliu wrote:
> then stamp pls? PS2 already removes the whole file

Oh, sorry, I didn't see you'd actually uploaded a new patch. LGTM.

[boliu, 2015-03-04 15:41:04]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-03-04 15:42:02]

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

[commit-bot: I haz the power, 2015-03-04 15:51:13]

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

[commit-bot: I haz the power, 2015-03-04 15:51:14]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[boliu, 2015-03-04 17:19:30]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-03-04 17:19:55]

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

[commit-bot: I haz the power, 2015-03-04 17:30:08]

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

[commit-bot: I haz the power, 2015-03-04 17:30:08]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[boliu, 2015-03-04 18:02:44]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2015-03-04 18:03:37]

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

[commit-bot: I haz the power, 2015-03-04 18:15:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-04 18:16:17]

Patchset 2 (id:??) landed as
https://crrev.com/d6e13ee19aa46617da45c49289e9b658544c6e1d
Cr-Commit-Position: refs/heads/master@{#319083}