Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1829023002:
Add fake for InstanceIDWithSubtype.java, in order to re-use unit test (Closed)
|
Created:
4 years, 9 months ago by johnme Modified:
4 years, 8 months ago CC:
chromium-reviews, zea+watch_chromium.org, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, johnme+watch_chromium.org, sdefresne+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@iid2jni Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd fake for InstanceIDWithSubtype.java, in order to re-use unit test
Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the
Google Play Services InstanceID class (but doesn't make network requests
of course).
Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled
on Android, verifying that the behavior there is the same as the
existing behavior that it is testing on desktop.
Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. this patch
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs
BUG=589461
Committed: https://crrev.com/8638071c84e5112bcbffee71da8db79378422546
Cr-Commit-Position: refs/heads/master@{#389128}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Rebae #Patch Set 4 : Tweaks #
Total comments: 12
Patch Set 5 : Stop using NestedMessagePumpAndroid #Patch Set 6 : Remove boilerplate, with InstanceIDBridge.BridgeAsyncTask<Result> #Patch Set 7 : Address review comments except moving FakeInstanceIDWithSubtype to javatests/ #Patch Set 8 : Move FakeInstanceIDWithSubtype to javatests/ #
Total comments: 38
Patch Set 9 : Address 2nd round review comments #
Total comments: 2
Patch Set 10 : Use scoped objects #Patch Set 11 : Fix indirect GN dep #Patch Set 12 : Fix GN #Patch Set 13 : Rebase #Patch Set 14 : Fix GYP #Patch Set 15 : Fix FindBugs #Patch Set 16 : Re-fix FindBugs #Dependent Patchsets: Messages
Total messages: 61 (30 generated)
Description was changed from ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch BUG=589461 ========== to ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1854093002 enables InstanceID by default 5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 ==========
johnme@chromium.org changed reviewers: + jianli@chromium.org
johnme@chromium.org changed reviewers: - jianli@chromium.org
Actually, perhaps it makes more sense for Peter to review this issue, as it's mostly all Java stuff
johnme@chromium.org changed reviewers: + peter@chromium.org
https://codereview.chromium.org/1829023002/diff/60001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/1829023002/diff/60001/components/components_t... components/components_tests.gyp:1413: '../content/content_shell_and_tests.gyp:layouttest_support_content', I am not comfortable with adding this dependency, sorry. The `layouttest_support_content` target depends on the test runner in //components/test_runner/, which means that you're adding an indirect dependency from any unit test part of //components to the test runner. https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:36: mId = randomBase64(11 /* length */); This implicitly depends on getId() to be called before getCreationTime()? Is that intentional? Can we initialize these values in the constructor instead? https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:47: public String getToken(String authorizedEntity, String scope) throws IOException { When would this be called? https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:55: throw new IOException(InstanceID.ERROR_MAIN_THREAD); The general flow of your code is now as follows: (1) InstanceIDDriverTest (2) InstanceIDAndroid (3) JNI -> InstanceIDWithSubtype.java (3a) JNI getId() -> :33 (3b) JNI getCreationTime() -> :42 (3c) JNI getToken() -> AsyncTask -> :52 (3d) JNI deleteToken() -> AsyncTask -> :67 (3e) JNI deleteInstanceID() -> AsyncTask -> :77 That means that all the complexity you're adding with the NestedSystemMessageHandler effectively tests the AsyncTasks and the four lines of logic we have in InstanceIDWithSubtype.getToken(). Since we know that the NestedSystemMessageHandler class is very much disliked (albeit a necessary evil) and the gain is very minor, let's instead override the JNI entry points in this FakeInstanceIDWithSubtype class and not bother with the asynchronousity. If you feel strongly about testing the extraStrings -> bundle conversion in InstanceIDWithSubtype.getToken(), you can extract it to a helper method and re-use it in the test as well. https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right): https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:113: static_cast<base::MessageLoopForUI*>(message_loop_.get())->Start(); You're casting an instance of base::MessageLoop to a subclass you know it to not be an instance of...?
https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:20: * Fake for InstanceIDWithSubtype. One higher-level concern I have with this approach is that we're going to ship the FakeInstanceIDWithSubtype with production builds. It would be much nicer if we can store this file in //components/gcm_driver/instance_id/android/javatests/ instead (and thus have it be test-only) and perhaps give InstanceIDWithSubtype a method to *set* the instance for a given subtype. It would be more plumbing, but shipping this isn't great.
Addressed review comments - thanks! https://codereview.chromium.org/1829023002/diff/60001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/1829023002/diff/60001/components/components_t... components/components_tests.gyp:1413: '../content/content_shell_and_tests.gyp:layouttest_support_content', On 2016/04/11 14:57:05, Peter Beverloo wrote: > I am not comfortable with adding this dependency, sorry. > > The `layouttest_support_content` target depends on the test runner in > //components/test_runner/, which means that you're adding an indirect dependency > from any unit test part of //components to the test runner. Removed. https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:20: * Fake for InstanceIDWithSubtype. On 2016/04/11 15:16:14, Peter Beverloo wrote: > One higher-level concern I have with this approach is that we're going to ship > the FakeInstanceIDWithSubtype with production builds. It would be much nicer if > we can store this file in //components/gcm_driver/instance_id/android/javatests/ > instead (and thus have it be test-only) and perhaps give InstanceIDWithSubtype a > method to *set* the instance for a given subtype. > > It would be more plumbing, but shipping this isn't great. I've uploaded two patchsets - the first has the other review comments applied, and the second moves FakeInstanceIDWithSubtype to javatests/ as suggested. I agree that it's cleaner not to compile it in prod builds, but it adds 122 lines of JNI boilerplate etc :-| https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:36: mId = randomBase64(11 /* length */); On 2016/04/11 14:57:05, Peter Beverloo wrote: > This implicitly depends on getId() to be called before getCreationTime()? Is > that intentional? Can we initialize these values in the constructor instead? The mCreationTime member is initialized to be 0 in the declaration, which is a valid value meaning not-yet-created. In practice this gets executed at construction time. What's the benefit of moving that into the constructor body? https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:47: public String getToken(String authorizedEntity, String scope) throws IOException { On 2016/04/11 14:57:05, Peter Beverloo wrote: > When would this be called? I faked the entire public API of InstanceID. This is the only method which doesn't currently get exercised by InstanceIDBridge, and it's only a few lines, so it seemed cleaner to leave it in, so any future Java code using the fake doesn't get surprised by accidentally calling non-fake methods. https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:55: throw new IOException(InstanceID.ERROR_MAIN_THREAD); On 2016/04/11 14:57:05, Peter Beverloo wrote: > The general flow of your code is now as follows: > > (1) InstanceIDDriverTest > (2) InstanceIDAndroid > (3) JNI -> InstanceIDWithSubtype.java > (3a) JNI getId() -> :33 > (3b) JNI getCreationTime() -> :42 > (3c) JNI getToken() -> AsyncTask -> :52 > (3d) JNI deleteToken() -> AsyncTask -> :67 > (3e) JNI deleteInstanceID() -> AsyncTask -> :77 > > That means that all the complexity you're adding with the > NestedSystemMessageHandler effectively tests the AsyncTasks and the four lines > of logic we have in InstanceIDWithSubtype.getToken(). > > Since we know that the NestedSystemMessageHandler class is very much disliked > (albeit a necessary evil) and the gain is very minor, let's instead override the > JNI entry points in this FakeInstanceIDWithSubtype class and not bother with the > asynchronousity. > > If you feel strongly about testing the extraStrings -> bundle conversion in > InstanceIDWithSubtype.getToken(), you can extract it to a helper method and > re-use it in the test as well. I managed to remove the dependency on NestedSystemMessageHandler by replacing my AsyncTasks with a new BridgeAsyncTask wrapper which uses AsyncTask.get() to workaround the fact that onPostExecute doesn't get run without NestedSystemMessageHandler. https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right): https://codereview.chromium.org/1829023002/diff/60001/components/gcm_driver/i... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:113: static_cast<base::MessageLoopForUI*>(message_loop_.get())->Start(); On 2016/04/11 14:57:05, Peter Beverloo wrote: > You're casting an instance of base::MessageLoop to a subclass you know it to not > be an instance of...? Bizarrely, this seems to be what you're supposed to do. It's what Kristian does in: https://code.google.com/p/chromium/codesearch#chromium/src/base/android/java_... And message_loop.h contains the following: // Do not add any member variables to MessageLoopForUI! This is important b/c // MessageLoopForUI is often allocated via MessageLoop(TYPE_UI). Any extra // data that you need should be stored on the MessageLoop's pump_ instance. static_assert(sizeof(MessageLoop) == sizeof(MessageLoopForUI), "MessageLoopForUI should not have extra member variables"); Anyway, thankfully this code all got removed now I stopped using NestedMessagePumpAndroid.
Let's continue with PS8. While it indeed is more plumbing, it allows us to avoid shipping testing infrastructure as part of production builds. https://codereview.chromium.org/1829023002/diff/140001/components/BUILD.gn File components/BUILD.gn (left): https://codereview.chromium.org/1829023002/diff/140001/components/BUILD.gn#ol... components/BUILD.gn:254: "//components/gcm_driver/instance_id:unit_tests", This affects iOS as well as Android. Is that intentional? https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:60: * never get executed, so instead we'll synchronously block until each task completes. Please avoid use of "we" in comments. (Also goes for :177.) Maybe phrase this as: * Will block returning the control flow to C++ until the asynchronous Java operation has * completed. To be used in tests where no nested message loop is available. The caller is * expected to reset this to "false" while tearing down. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:154: /** Custom AsyncTask wrapper to workaround onPostExecute not working in tests. */ Thank you for generalizing this! Please give this class a proper documentation block, and be more elaborate about why it's necessary. Right now you kind of have to go back and forth between this line, the definition and the comment on line 174+, let's unify that. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:173: task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); What's wrong with .execute()? The documentation says we probably don't want to use this. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:183: throw new IllegalStateException(e); // Shouldn't happen in tests :-p No smileys in comments please :-) https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:27: protected static FakeFactory sFakeFactoryForTesting = null; Maybe remove "protected" and mark this as @VisibleForTesting? https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:81: public static Map<String, InstanceIDWithSubtype> getInstanceIDsForTesting() { Why are you making sFakeFactoryForTesting visible to the sub-class, but require the use of getInstanceIDsForTesting() for clearing or removing a subtype? Let's trust ourselves to do the right thing (regarding the exception you throw on :83) and mark both as @VisibleForTesting? https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:88: public static boolean usingFake() { Unused. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:23: * Fake for InstanceIDWithSubtype. Please document how it's fake. Even "avoids hitting the network" would already be very valuable. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:49: public FakeInstanceIDWithSubtype(Context context, String subtype) { private? https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:76: throw new IOException(InstanceID.ERROR_MAIN_THREAD); nit: even one-line if-statements must have curly braces in Java. Elsewhere too. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:79: if (token == null && !"true".equals(extras.getString("fakeOffline"))) { Let's please introduce more detailed functionality like this in the tests that will actually use it. (Code search shows no results for "fakeOffline" so I presume it will be part of a follow-up patch?) https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:110: "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-_"; nit: statics go at the top of the class https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:111: /** Returns a random (not necessarily valid) base64url encoded string. */ Please give this a proper doc block. Also note that URL-safe base64 generated like this will always be decode-able, because the trailing equal-sign padding is generally optional for that encoding. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_android.h (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_android.h:32: // NestedMessagePumpAndroid which makes this unnecessary. No need to make this specific to environments, that's bound to go out of date. // Tests depending on InstanceID in an environment where no nested message // loops are available should enable blocking on asynchronous operations // using this mechanism. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:18: #include "components/gcm_driver/instance_id/instance_id_test_utils_android.h" Stick this in the #ifdef too? https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:115: InstanceIDAndroid::SetBlockOnAsyncTasksForTesting(true); Clean up as part of TearDown? They're statics so this could have side-effects for other tests ran as part of the same process too. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_test_utils_android.h (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_test_utils_android.h:17: static bool RegisterJni(JNIEnv* env); You never call this anywhere? https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_test_utils_android.h:19: // Call this (with use_fake=true) in SetUp of all tests using Instance IDs. This is not really helpful. Document what it does and why it's necessary, similar to the other comment.
Addressed 2nd round of review comments - thanks! https://codereview.chromium.org/1829023002/diff/140001/components/BUILD.gn File components/BUILD.gn (left): https://codereview.chromium.org/1829023002/diff/140001/components/BUILD.gn#ol... components/BUILD.gn:254: "//components/gcm_driver/instance_id:unit_tests", On 2016/04/15 00:00:17, Peter Beverloo wrote: > This affects iOS as well as Android. Is that intentional? No, I moved it from (!is_android && !is_ios) to !iOS https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:60: * never get executed, so instead we'll synchronously block until each task completes. On 2016/04/15 00:00:17, Peter Beverloo wrote: > Please avoid use of "we" in comments. (Also goes for :177.) Maybe phrase this > as: > > * Will block returning the control flow to C++ until the asynchronous Java > operation has > * completed. To be used in tests where no nested message loop is available. > The caller is > * expected to reset this to "false" while tearing down. Done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:154: /** Custom AsyncTask wrapper to workaround onPostExecute not working in tests. */ On 2016/04/15 00:00:17, Peter Beverloo wrote: > Thank you for generalizing this! > > Please give this class a proper documentation block, and be more elaborate about > why it's necessary. Right now you kind of have to go back and forth between this > line, the definition and the comment on line 174+, let's unify that. Done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:173: task.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR); On 2016/04/15 00:00:17, Peter Beverloo wrote: > What's wrong with .execute()? The documentation says we probably don't want to > use this. The default SERIAL_EXECUTOR is global to the app's process. So all AsyncTasks in Chrome using the default SERIAL_EXECUTOR would be blocked waiting for our network requests to complete. Using the THREAD_POOL_EXECUTOR avoids that; it does however mean that the tasks in this file can all execute in parallel, which should be fine (both InstanceID and my wrapper are careful to synchronize on shared state), though I grant that there's an increased possibility of subtle bugs. A 3rd alternative would be for this class to have its own Executor.newSingleThreadExecutor(), so these tasks are executed sequentially with respect to each other, but in parallel to other Clank tasks; but I'd much rather use an existing thread pool than spin up new threads just for IID. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDBridge.java:183: throw new IllegalStateException(e); // Shouldn't happen in tests :-p On 2016/04/15 00:00:17, Peter Beverloo wrote: > No smileys in comments please :-) Spoil sport :-p https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:27: protected static FakeFactory sFakeFactoryForTesting = null; On 2016/04/15 00:00:17, Peter Beverloo wrote: > Maybe remove "protected" and mark this as @VisibleForTesting? Marked it as @VisibleForTesting, but kept protected to hint that the subclass is the one accessing it for testing purposes. Also made FakeInstanceIDWithSubtype.clearDataAndSetEnabled set sFakeFactoryForTesting within its synchronized block, in case someone calls clearDataAndSetEnabled while an AsyncTask is executing getInstance. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:81: public static Map<String, InstanceIDWithSubtype> getInstanceIDsForTesting() { On 2016/04/15 00:00:17, Peter Beverloo wrote: > Why are you making sFakeFactoryForTesting visible to the sub-class, but require > the use of getInstanceIDsForTesting() for clearing or removing a subtype? Let's > trust ourselves to do the right thing (regarding the exception you throw on :83) > and mark both as @VisibleForTesting? Because I'm going to need access to getInstanceIDsForTesting() from a Java test in a later patch :p. But on second thoughts, I'll just mark sSubtypeInstances "@VisibleForTesting public" in the follow-on patch, which is neater. So done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/java/src/org/chromium/components/gcm_driver/instance_id/InstanceIDWithSubtype.java:88: public static boolean usingFake() { On 2016/04/15 00:00:17, Peter Beverloo wrote: > Unused. Removed. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:23: * Fake for InstanceIDWithSubtype. On 2016/04/15 00:00:17, Peter Beverloo wrote: > Please document how it's fake. Even "avoids hitting the network" would already > be very valuable. Done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:49: public FakeInstanceIDWithSubtype(Context context, String subtype) { On 2016/04/15 00:00:17, Peter Beverloo wrote: > private? Done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:76: throw new IOException(InstanceID.ERROR_MAIN_THREAD); On 2016/04/15 00:00:17, Peter Beverloo wrote: > nit: even one-line if-statements must have curly braces in Java. Elsewhere too. Done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:79: if (token == null && !"true".equals(extras.getString("fakeOffline"))) { On 2016/04/15 00:00:17, Peter Beverloo wrote: > Let's please introduce more detailed functionality like this in the tests that > will actually use it. (Code search shows no results for "fakeOffline" so I > presume it will be part of a follow-up patch?) Removed. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:110: "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-_"; On 2016/04/15 00:00:17, Peter Beverloo wrote: > nit: statics go at the top of the class The style guide says "Define fields either at the top of the file or immediately before the methods that use them." http://source.android.com/source/code-style.html#define-fields-in-standard-pl.... In this case the latter seems clearly more readable. Though even better would be to move it into the randomBase64 method, which I've done. Thanks to the string constant pool it should be equally efficient. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/android/javatests/src/org/chromium/components/gcm_driver/instance_id/FakeInstanceIDWithSubtype.java:111: /** Returns a random (not necessarily valid) base64url encoded string. */ On 2016/04/15 00:00:17, Peter Beverloo wrote: > Please give this a proper doc block. > > Also note that URL-safe base64 generated like this will always be decode-able, > because the trailing equal-sign padding is generally optional for that encoding. Ah good - removed "not necessarily valid". This is a proper doc block though. See http://source.android.com/source/code-style.html#use-javadoc-standard-comments https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_android.h (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_android.h:32: // NestedMessagePumpAndroid which makes this unnecessary. On 2016/04/15 00:00:17, Peter Beverloo wrote: > No need to make this specific to environments, that's bound to go out of date. > > // Tests depending on InstanceID in an environment where no nested message > // loops are available should enable blocking on asynchronous operations > // using this mechanism. Done (reworded). https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:18: #include "components/gcm_driver/instance_id/instance_id_test_utils_android.h" On 2016/04/15 00:00:18, Peter Beverloo wrote: > Stick this in the #ifdef too? Done. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:115: InstanceIDAndroid::SetBlockOnAsyncTasksForTesting(true); On 2016/04/15 00:00:17, Peter Beverloo wrote: > Clean up as part of TearDown? They're statics so this could have side-effects > for other tests ran as part of the same process too. Done (I'd been hoping to avoid the boilerplate, as it's never wrong for tests to run with these statics enabled, but it might cause people to forget to enable them in other tests, so fair enough). https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_test_utils_android.h (right): https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_test_utils_android.h:17: static bool RegisterJni(JNIEnv* env); On 2016/04/15 00:00:18, Peter Beverloo wrote: > You never call this anywhere? Blarg. Turns out it doesn't even do anything. Added it to components/test/run_all_unittests.cc and filed https://crbug.com/603936 to nuke it. https://codereview.chromium.org/1829023002/diff/140001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_test_utils_android.h:19: // Call this (with use_fake=true) in SetUp of all tests using Instance IDs. On 2016/04/15 00:00:18, Peter Beverloo wrote: > This is not really helpful. Document what it does and why it's necessary, > similar to the other comment. Done.
Description was changed from ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1854093002 enables InstanceID by default 5. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 ========== to ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1899753002 fixes strict mode violations 5. https://codereview.chromium.org/1854093002 enables InstanceID by default 6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 ==========
lgtm https://codereview.chromium.org/1829023002/diff/160001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right): https://codereview.chromium.org/1829023002/diff/160001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:116: InstanceIDTestUtilsAndroid::ClearDataAndSetUseFakeForTesting(true); Would we ever used them separately? Could trigger InstanceIDAndroid::SetBlockOnAsyncTasksForTesting from InstanceIDTestUtilsAndroid otherwise. Could also make one of these super awesome scoped objects that do this automagically for you :P
johnme@chromium.org changed reviewers: + caitkp@chromium.org
caitkp@chromium.org: Please review components/BUILD.gn and components/test/run_all_unittests.cc - thanks!
https://codereview.chromium.org/1829023002/diff/160001/components/gcm_driver/... File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right): https://codereview.chromium.org/1829023002/diff/160001/components/gcm_driver/... components/gcm_driver/instance_id/instance_id_driver_unittest.cc:116: InstanceIDTestUtilsAndroid::ClearDataAndSetUseFakeForTesting(true); On 2016/04/18 15:46:36, Peter Beverloo wrote: > Would we ever used them separately? Could trigger > InstanceIDAndroid::SetBlockOnAsyncTasksForTesting from > InstanceIDTestUtilsAndroid otherwise. > > Could also make one of these super awesome scoped objects that do this > automagically for you :P I made them scoped objects. I left them separate for now, since it would be reasonable for tests to use ScopedUseFakeInstanceIDAndroid but not ScopedBlockOnAsyncTasksForTesting if they have a nested Java loop like the upcoming Android browser_tests presumably will, (or ScopedBlockOnAsyncTasksForTesting but not ScopedUseFakeInstanceIDAndroid if they want to test against live GCM, but don't have a nested Java loop).
lgtm
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps180001 (title: "Use scoped objects")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps200001 (title: "Fix indirect GN dep")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps220001 (title: "Fix GN")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps240001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps260001 (title: "Fix GYP")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps280001 (title: "Fix FindBugs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Have you considered building locally?...
The CQ bit was checked by johnme@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1829023002/#ps300001 (title: "Re-fix FindBugs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1829023002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1829023002/300001
Message was sent while issue was closed.
Description was changed from ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1899753002 fixes strict mode violations 5. https://codereview.chromium.org/1854093002 enables InstanceID by default 6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 ========== to ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1899753002 fixes strict mode violations 5. https://codereview.chromium.org/1854093002 enables InstanceID by default 6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1899753002 fixes strict mode violations 5. https://codereview.chromium.org/1854093002 enables InstanceID by default 6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 ========== to ========== Add fake for InstanceIDWithSubtype.java, in order to re-use unit test Adds FakeInstanceIDWithSubtype, which closely mimics the behavior of the Google Play Services InstanceID class (but doesn't make network requests of course). Hence InstanceIDDriverTest (instance_id_driver_unittest.cc) is enabled on Android, verifying that the behavior there is the same as the existing behavior that it is testing on desktop. Part of a series of patches: 1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype 2. https://codereview.chromium.org/1830983002 adds JNI bindings 3. this patch 4. https://codereview.chromium.org/1899753002 fixes strict mode violations 5. https://codereview.chromium.org/1854093002 enables InstanceID by default 6. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs BUG=589461 Committed: https://crrev.com/8638071c84e5112bcbffee71da8db79378422546 Cr-Commit-Position: refs/heads/master@{#389128} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/8638071c84e5112bcbffee71da8db79378422546 Cr-Commit-Position: refs/heads/master@{#389128} |
