Sync: local data verification for Android tests

Sync: local data verification for Android tests

This CL dramatically improves the ability to verify local Sync data in
Java Android tests. This is done by plumbing through the
ProfileSyncService's GetAllNodes method through JNI.

This new util will also allow for the server-side modification and
deletion of Sync entities in a future CL.

An older version of local data verification
(relying only on entity counts) has been removed.

BUG=365774
Committed: https://crrev.com/ff27ca20c9913a184c7c04550fc26640bcf5c9b4
Cr-Commit-Position: refs/heads/master@{#330842}

[pval...(no longer on Chromium), 2015-05-14 23:17:36]

pvalenzuela@chromium.org changed reviewers:
+ nyquist@chromium.org, zea@chromium.org

[pval...(no longer on Chromium), 2015-05-14 23:17:36]

I think you both can provide valuable feedback here, so I'd appreciate both of
you taking a look.

[Nicolas Zea, 2015-05-15 20:09:22]

LGTM with one comment

https://codereview.chromium.org/1127233008/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/1127233008/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.cc:79: if
(!base::JSONWriter::Write(result.get(), &json_string)) {
Check for a null scoped ptr as well?

[pval...(no longer on Chromium), 2015-05-19 01:38:23]

nyquist@: ping

https://codereview.chromium.org/1127233008/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/1127233008/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service_android.cc:79: if
(!base::JSONWriter::Write(result.get(), &json_string)) {
On 2015/05/15 20:09:22, Nicolas Zea wrote:
> Check for a null scoped ptr as well?

Done. Note: I had to rebase and then do this as the JSONWriter API recently
changed.

[nyquist, 2015-05-19 06:42:49]

Mostly just nits, but overall looks great

https://codereview.chromium.org/1127233008/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/1127233008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:619:
public static void onGetAllNodesResult(GetAllNodesCallback callback, String
nodes) {
Nit: Could this be private? We don't want this to be called from anywhere else,
do we?

A note here is that JNI-methods doesn't have to be public or anything like that
when they are accessed from native. The visibility is only related to other Java
classes' usage of the method.

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:360:
new ProfileSyncService.GetAllNodesCallback();
I'm not a super fan of polling myself, so how about:

final Semaphore semaphore = new Semaphore(0);
ProfileSyncService.GetAllNodesCallback callback =
        new ProfileSyncService.GetAllNodesCallback() {
    @Override
    public void onResult(String nodesString) {
        super(nodesString);
        semaphore.release();
    }
};

And then remove the criteria and finished-stuff, and instead the try-statement
have:

assertTrue("Semaphore should have been released.",
        semaphore.tryAcquire(UI_TIMEOUT_MS, TimeUnit.MILLISECONDS)

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:383:
* contains the same data as a *Specifics protocol buffer (e.g.,
TypedUrlSpecifics).
Nit: Remove the extra * here?

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:408:
* the same as the data stored in a *Specifics protocol buffer (e.g.,
TypedUrlSpecifics).
Nit: Remove the extra * here as well?

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:425:
datatypeNodes = datatypeObject.getJSONArray("nodes");
Nit: Would we want to break here?

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:432:
JSONObject typedUrl = datatypeNodes.getJSONObject(i);
Nit: This isn't necessarily typedUrl anymore, is it?

[pval...(no longer on Chromium), 2015-05-19 23:38:55]

https://codereview.chromium.org/1127233008/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/1127233008/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:619:
public static void onGetAllNodesResult(GetAllNodesCallback callback, String
nodes) {
On 2015/05/19 06:42:49, nyquist wrote:
> Nit: Could this be private? We don't want this to be called from anywhere
else,
> do we?
> 
> A note here is that JNI-methods doesn't have to be public or anything like
that
> when they are accessed from native. The visibility is only related to other
Java
> classes' usage of the method.

yep, done

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java
(right):

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:360:
new ProfileSyncService.GetAllNodesCallback();
On 2015/05/19 06:42:49, nyquist wrote:
> I'm not a super fan of polling myself, so how about:
> 
> final Semaphore semaphore = new Semaphore(0);
> ProfileSyncService.GetAllNodesCallback callback =
>         new ProfileSyncService.GetAllNodesCallback() {
>     @Override
>     public void onResult(String nodesString) {
>         super(nodesString);
>         semaphore.release();
>     }
> };
> 
> And then remove the criteria and finished-stuff, and instead the try-statement
> have:
> 
> assertTrue("Semaphore should have been released.",
>         semaphore.tryAcquire(UI_TIMEOUT_MS, TimeUnit.MILLISECONDS)

Done.

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:383:
* contains the same data as a *Specifics protocol buffer (e.g.,
TypedUrlSpecifics).
On 2015/05/19 06:42:49, nyquist wrote:
> Nit: Remove the extra * here?

Done. I was trying to indicate that it would be a FooSpecifics or BarSpecifics,
but I think this new way is easier to understand.

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:408:
* the same as the data stored in a *Specifics protocol buffer (e.g.,
TypedUrlSpecifics).
On 2015/05/19 06:42:49, nyquist wrote:
> Nit: Remove the extra * here as well?

Done.

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:425:
datatypeNodes = datatypeObject.getJSONArray("nodes");
On 2015/05/19 06:42:49, nyquist wrote:
> Nit: Would we want to break here?

Done.

https://codereview.chromium.org/1127233008/diff/40001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/util/browser/sync/SyncTestUtil.java:432:
JSONObject typedUrl = datatypeNodes.getJSONObject(i);
On 2015/05/19 06:42:49, nyquist wrote:
> Nit: This isn't necessarily typedUrl anymore, is it?

thanks for catching this. forgot to rename after moving the code around.

[nyquist, 2015-05-20 00:02:28]

lgtm

[pval...(no longer on Chromium), 2015-05-20 00:06:18]

The CQ bit was checked by pvalenzuela@chromium.org

[pval...(no longer on Chromium), 2015-05-20 00:06:18]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1127233008/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-05-20 00:21:39]

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

[commit-bot: I haz the power, 2015-05-20 01:42:02]

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

[commit-bot: I haz the power, 2015-05-20 01:42:05]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[pval...(no longer on Chromium), 2015-05-20 18:22:49]

CQ caught a thread issue (calling PSS.java on a non-UI thread).

nyquist@, PTAL?

[nyquist, 2015-05-20 22:07:45]

lgtm

[pval...(no longer on Chromium), 2015-05-20 22:11:50]

The CQ bit was checked by pvalenzuela@chromium.org

[pval...(no longer on Chromium), 2015-05-20 22:11:51]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1127233008/#ps80001
(title: "fix threading")

[commit-bot: I haz the power, 2015-05-20 22:13:12]

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

[commit-bot: I haz the power, 2015-05-20 23:34:30]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-20 23:35:52]

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