Run shell tests on android.

Run shell tests on android.

This CL enables shell tests on android.

For this, it embeds test applications in the test apk, and copied those
on disk as needed. It also refactor FileHelper so that it keeps a
timestamp of the last apk it was used with to allow no to re-extract
files that are already up to date.

R=ppi@chromium.org, viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/7d036f0410828ad28863d4659b4260ffcdf9f602

[qsr, 2014-12-19 16:26:09]

qsr@chromium.org changed reviewers:
+ ppi@chromium.org, viettrungluu@chromium.org

[qsr, 2014-12-19 16:26:09]

Please disregard changes to build/android. Those are chromium changes that I
will push there and then roll chromium.

Trung - Are you the right reviewer for shell/shell_test_base.cc ? We do not have
history on this file, so I do not remember who started those tests
ppi -> Can you look at the java changes.

[viettrungluu, 2014-12-19 18:31:35]

(I think I originally wrote shell_test_base.*, though tim hacked on them more
than me ... though he's off doing something else now.)

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.cc
File shell/shell_test_base.cc (right):

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.cc...
shell/shell_test_base.cc:39: void ShellTestBase::SetUpTestApplications() {
Are you missing a shell_test_base_android.cc from this CL?

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.h
File shell/shell_test_base.h (right):

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.h#...
shell/shell_test_base.h:10: #include "base/files/file_path.h"
I don't think you need this....

[qsr, 2014-12-22 10:52:32]

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.cc
File shell/shell_test_base.cc (right):

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.cc...
shell/shell_test_base.cc:39: void ShellTestBase::SetUpTestApplications() {
On 2014/12/19 18:31:34, viettrungluu wrote:
> Are you missing a shell_test_base_android.cc from this CL?

 Yes, sorry. Added.

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.h
File shell/shell_test_base.h (right):

https://codereview.chromium.org/815993004/diff/20001/shell/shell_test_base.h#...
shell/shell_test_base.h:10: #include "base/files/file_path.h"
On 2014/12/19 18:31:34, viettrungluu wrote:
> I don't think you need this....

Done.

[ppi, 2014-12-22 12:02:25]

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
File shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java (right):

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:29: public static
final String TAG = "MojoFileHelper";
nit: everything is Mojo in this world, I would probably call it "FileHelper"

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:36: // Looks for a
timestamp file on disk that indicates the version of the APK that
Maybe use the javadoc style (matching other per-method comments in this
package).

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:37: // the
resource paks were extracted from. Returns null if a timestamp was found
what is resource pak? should we say "assets", or do we mean resources here (as
per the apk terminology)?

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:39: // a String
that represents the filename of a timestamp to create.
This calls for splitting this up - can we extract the method to compute the
expected timestamp and have another to check if the files match the expected
one?

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:41: final String
timestampPrefix = "asset_timestamp-";
let's make it private static final field like the others?

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:74: // timestamp
file is already up-to date.
nit: capitalize the first word.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:115: // Worst case
we don't write a timestamp, so we'll re-extract the asset next time.
nit: s/Worst case/In the worst case/

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
File shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java (right):

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:17: * Helper
method for ShellTestBase
nit: add period at the end.

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:25: * Extract
the mojo application from the apk assets and returns the directory where they
are.
s/Extract/Extracts/

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:25: * Extract
the mojo application from the apk assets and returns the directory where they
are.
s/application/applications

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:28: private
static String extractMojoApplication(Context context) throws IOException {
s/extractMojoApplication/extractMojoApplications/

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_an...
File shell/shell_test_base_android.cc (right):

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_an...
shell/shell_test_base_android.cc:32: // Extract mojo application, and set the
resolve base URL to the directory
s/application/applications/

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_un...
File shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_un...
shell/shell_test_base_unittest.cc:163:
ConnectToService(GURL("http://example.com/non_existent_service"),
why this change?

[qsr, 2014-12-22 12:44:20]

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
File shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java (right):

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:29: public static
final String TAG = "MojoFileHelper";
On 2014/12/22 12:02:25, ppi wrote:
> nit: everything is Mojo in this world, I would probably call it "FileHelper"

 Everything is mojo, but this is what appear in adb logcat that contains all the
log of all the application on the device. Prefixing this with mojo helps finding
those logs.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:36: // Looks for a
timestamp file on disk that indicates the version of the APK that
On 2014/12/22 12:02:25, ppi wrote:
> Maybe use the javadoc style (matching other per-method comments in this
> package).

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:37: // the
resource paks were extracted from. Returns null if a timestamp was found
On 2014/12/22 12:02:25, ppi wrote:
> what is resource pak? should we say "assets", or do we mean resources here (as
> per the apk terminology)?

 Because I copied paste this comment. Fixed.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:39: // a String
that represents the filename of a timestamp to create.
On 2014/12/22 12:02:25, ppi wrote:
> This calls for splitting this up - can we extract the method to compute the
> expected timestamp and have another to check if the files match the expected
> one?

 As discussed, I prefer not to cut this method because it would otherwise
obliged me to either keep more state when using it, or to access the disk more.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:41: final String
timestampPrefix = "asset_timestamp-";
On 2014/12/22 12:02:25, ppi wrote:
> let's make it private static final field like the others?

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:74: // timestamp
file is already up-to date.
On 2014/12/22 12:02:25, ppi wrote:
> nit: capitalize the first word.

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo/shell/FileHelper.java:115: // Worst case
we don't write a timestamp, so we'll re-extract the asset next time.
On 2014/12/22 12:02:25, ppi wrote:
> nit: s/Worst case/In the worst case/

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
File shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java (right):

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:17: * Helper
method for ShellTestBase
On 2014/12/22 12:02:25, ppi wrote:
> nit: add period at the end.

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:25: * Extract
the mojo application from the apk assets and returns the directory where they
are.
On 2014/12/22 12:02:25, ppi wrote:
> s/application/applications

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:25: * Extract
the mojo application from the apk assets and returns the directory where they
are.
On 2014/12/22 12:02:25, ppi wrote:
> s/Extract/Extracts/

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/android/tests/src/...
shell/android/tests/src/org/chromium/mojo/shell/ShellTestBase.java:28: private
static String extractMojoApplication(Context context) throws IOException {
On 2014/12/22 12:02:25, ppi wrote:
> s/extractMojoApplication/extractMojoApplications/

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_an...
File shell/shell_test_base_android.cc (right):

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_an...
shell/shell_test_base_android.cc:32: // Extract mojo application, and set the
resolve base URL to the directory
On 2014/12/22 12:02:25, ppi wrote:
> s/application/applications/

Done.

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_un...
File shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/815993004/diff/40001/shell/shell_test_base_un...
shell/shell_test_base_unittest.cc:163:
ConnectToService(GURL("http://example.com/non_existent_service"),
On 2014/12/22 12:02:25, ppi wrote:
> why this change?

Because this was convoluted and wrong. What was happening is that 
ConnectToServiceViaNetwork was not settings the base url of the mojo resolver,
and hoping that it would go to the network (which was not the case), passing a
http url force the shell to try to go to the network service.

[ppi, 2014-12-22 12:51:03]

lgtm, thanks

[viettrungluu, 2014-12-22 17:52:39]

lgtm w/nits

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
File shell/shell_test_base_android.cc (right):

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
shell/shell_test_base_android.cc:19: namespace {
nit: blank line after

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
shell/shell_test_base_android.cc:22: static bool inited = false;
nit: I think you should just call it "initialized"; otherwise we'd have to
discuss if it should be "inited" or "initted".

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
shell/shell_test_base_android.cc:29: }
nit: blank line before, and "// namespace" comment

[qsr, 2014-12-23 09:39:08]

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
File shell/shell_test_base_android.cc (right):

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
shell/shell_test_base_android.cc:19: namespace {
On 2014/12/22 17:52:39, viettrungluu wrote:
> nit: blank line after

Done.

https://codereview.chromium.org/815993004/diff/100001/shell/shell_test_base_a...
shell/shell_test_base_android.cc:29: }
On 2014/12/22 17:52:39, viettrungluu wrote:
> nit: blank line before, and "// namespace" comment

Done.

[qsr, 2014-12-23 09:39:23]

Committed patchset #7 (id:120001) manually as
7d036f0410828ad28863d4659b4260ffcdf9f602 (presubmit successful).