Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1747223003:
Fix NetworkChangeNotifierTests on Lolipop+ devices. (Closed)
|
Created:
4 years, 9 months ago by bhallam Modified:
4 years, 8 months ago Reviewers:
pauljensen CC:
cbentzel+watch_chromium.org, chromium-reviews, xunjieli Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix NetworkChangeNotifierTests on Lolipop+ devices.
Fixing flaky test by removing erroneous asserts in the
org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver
callback implementation.
On real Lollipop+ devices which have access to a real and unpredictable
ConnectivityManager, the strict assertions will fail in a non-deterministic manner.
Refactored one test and added one test to the suite.
BUG=591103
Committed: https://crrev.com/3cb9af21193cd5f7d65fdac7d0b351ca97ab3669
Cr-Commit-Position: refs/heads/master@{#389755}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed CR comments #
Total comments: 14
Patch Set 3 : Addressing pauljensen's comments #
Total comments: 4
Patch Set 4 : Updated commit subject line and addressed comments #Messages
Total messages: 25 (8 generated)
bhallam@amazon.com changed reviewers: + asanka@chromium.org
Hi Asanka, Can you please look over the code review? Thanks Mohit
asanka@chromium.org changed reviewers: + xunjieli@chromium.org - asanka@chromium.org
Passing over to xunjieli.
Have you done the individual contributor agreement at https://cla.developers.google.com/about/google-individual?csw=1 ? https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:630: // Insert a mocked dummy implementation for the ConnectivityDelegate. Why do we use a mock ConnectivityManagerDelegate?
Replying to the question. https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:630: // Insert a mocked dummy implementation for the ConnectivityDelegate. On 2016/03/01 19:55:10, xunjieli wrote: > Why do we use a mock ConnectivityManagerDelegate? My understanding here is that we insert the mock delegate so that we dont have to deal with the unpredictable behavior of the connectivity manager. I looked at the waterfall layout did not show these particular tests failing because i think they dont run ContentShell tests run on dedicated Lolipop devices. Also tests in the current suite seem to employ the same behavior of inserting the mock delegate ; See #testNetworkCallbacks()
On 2016/03/01 19:55:10, xunjieli wrote: > Have you done the individual contributor agreement at > https://cla.developers.google.com/about/google-individual?csw=1 ? > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java > (right): > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:630: > // Insert a mocked dummy implementation for the ConnectivityDelegate. > Why do we use a mock ConnectivityManagerDelegate? Hi xunjieli I have also been added to the CCLA. Can you please take a look again ? Thanks :)
Description was changed from ========== Fix Failing Tests in org.chromium.net. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ========== to ========== Fix Failing Tests in org.chromium.net. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ==========
xunjieli@chromium.org changed reviewers: + pauljensen@chromium.org - xunjieli@chromium.org
On 2016/03/10 22:33:09, bhallam wrote: > On 2016/03/01 19:55:10, xunjieli wrote: > > Have you done the individual contributor agreement at > > https://cla.developers.google.com/about/google-individual?csw=1 ? > > > > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > > File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java > > (right): > > > > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > > net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:630: > > // Insert a mocked dummy implementation for the ConnectivityDelegate. > > Why do we use a mock ConnectivityManagerDelegate? > > Hi xunjieli > I have also been added to the CCLA. > Can you please take a look again ? > > Thanks :) Sorry, I didn't notice the update until now. Paul, could you take a look?
https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:309: final Context context = getInstrumentation().getTargetContext(); I don't think we need "final" here. I'm generally a proponent of "final" but not when it's used unnecessarily on local variables and complicates/lengthens simple functions. Here's a good reference on use of final: https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:347: assertFalse(receiver.isReceiverRegisteredForTesting()); how about just taking these last two statements and moving them to the end of the previous test function? It hurts readability to duplicate everything else. https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:630: // Insert a mocked dummy implementation for the ConnectivityDelegate. On 2016/03/01 20:32:55, bhallam wrote: > On 2016/03/01 19:55:10, xunjieli wrote: > > Why do we use a mock ConnectivityManagerDelegate? > > My understanding here is that we insert the mock delegate so that we dont have > to deal with the unpredictable > behavior of the connectivity manager. > > I looked at the waterfall layout did not show these particular tests failing > because i think they dont run ContentShell tests run on dedicated Lolipop > devices. > > Also tests in the current suite seem to employ the same behavior of inserting > the mock delegate ; > See #testNetworkCallbacks() This test must use the real ConnectivityManagerDelegate. The point of the test is to verify it doesn't crash. There's no point in verifying a mock doesn't crash.
On 2016/03/17 12:15:14, pauljensen wrote: > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java > (right): > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:309: > final Context context = getInstrumentation().getTargetContext(); > I don't think we need "final" here. I'm generally a proponent of "final" but > not when it's used unnecessarily on local variables and complicates/lengthens > simple functions. > Here's a good reference on use of final: > https://codereview.chromium.org/1492583002/diff/60001/components/cronet/andro... > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:347: > assertFalse(receiver.isReceiverRegisteredForTesting()); > how about just taking these last two statements and moving them to the end of > the previous test function? It hurts readability to duplicate everything else. > > https://codereview.chromium.org/1747223003/diff/1/net/android/javatests/src/o... > net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:630: > // Insert a mocked dummy implementation for the ConnectivityDelegate. > On 2016/03/01 20:32:55, bhallam wrote: > > On 2016/03/01 19:55:10, xunjieli wrote: > > > Why do we use a mock ConnectivityManagerDelegate? > > > > My understanding here is that we insert the mock delegate so that we dont have > > to deal with the unpredictable > > behavior of the connectivity manager. > > > > I looked at the waterfall layout did not show these particular tests failing > > because i think they dont run ContentShell tests run on dedicated Lolipop > > devices. > > > > Also tests in the current suite seem to employ the same behavior of inserting > > the mock delegate ; > > See #testNetworkCallbacks() > > This test must use the real ConnectivityManagerDelegate. The point of the test > is to verify it doesn't crash. There's no point in verifying a mock doesn't > crash. I have re-introduced the previous test here as well org.chromium.net.NetworkChangeNotifierTest#testQueryableAPIsDoNotCrash()
https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:301: * Tests that the receiver registers directly when pre-defined policy allows I don't understand the wording here. Perhaps "...registers for connectivity broadcasts during construction when the registration policy dictates." https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:307: public void testNetworkChangeNotifierUpdatedInConstructorDueToRegistrationPolicy() I don't understand this name, I think the old name is fine, or you could change Registers to RegistersWhenAppropriate or RegistersWhenPolicyDictates. https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:588: unnecessary white space change https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:602: unnecessary white space change https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:609: * values from the inserted mock connectivity manager. connectivity manager->ConnectivityManager https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:662: assertEquals(333, ncn.getDefaultNetId()); can you test that this returns NetId.INVALID if pre-lollipop? https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:667: assertEquals(4, ncn.getNetworksAndTypes().length); can you test that this returns an empty array pre-lollipop?
https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:301: * Tests that the receiver registers directly when pre-defined policy allows On 2016/04/06 11:33:21, pauljensen wrote: > I don't understand the wording here. Perhaps "...registers for connectivity > broadcasts during construction when the registration policy dictates." Done. https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:307: public void testNetworkChangeNotifierUpdatedInConstructorDueToRegistrationPolicy() On 2016/04/06 11:33:21, pauljensen wrote: > I don't understand this name, I think the old name is fine, or you could change > Registers to RegistersWhenAppropriate or RegistersWhenPolicyDictates. Done. https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:588: On 2016/04/06 11:33:22, pauljensen wrote: > unnecessary white space change Done. https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:602: On 2016/04/06 11:33:21, pauljensen wrote: > unnecessary white space change Done. https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:609: * values from the inserted mock connectivity manager. On 2016/04/06 11:33:21, pauljensen wrote: > connectivity manager->ConnectivityManager Done. https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:662: assertEquals(333, ncn.getDefaultNetId()); On 2016/04/06 11:33:21, pauljensen wrote: > can you test that this returns NetId.INVALID if pre-lollipop? I refactored the test so that for API levels < 21 it verifies that the NetworkChangeNotifierAutoDetect instance returns the default values. I can not inject a non null mock delegate for testing on API levels < 21 as the class android.net.Network is valid only on API level 21+. Thoughts? https://codereview.chromium.org/1747223003/diff/20001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:667: assertEquals(4, ncn.getNetworksAndTypes().length); On 2016/04/06 11:33:21, pauljensen wrote: > can you test that this returns an empty array pre-lollipop? Same as above.
https://codereview.chromium.org/1747223003/diff/40001/net/android/javatests/s... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/40001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:627: ncn.setConnectivityManagerDelegateForTests(null); is this line necessary? https://codereview.chromium.org/1747223003/diff/40001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:666: assertEquals(4, ncn.getNetworksAndTypes().length); can we verify the NetIds and Types here?
Can you please update the subject line of the commit description also, it's very vague/broad.
https://codereview.chromium.org/1747223003/diff/40001/net/android/javatests/s... File net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java (right): https://codereview.chromium.org/1747223003/diff/40001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:627: ncn.setConnectivityManagerDelegateForTests(null); On 2016/04/14 13:46:34, pauljensen wrote: > is this line necessary? Done. https://codereview.chromium.org/1747223003/diff/40001/net/android/javatests/s... net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java:666: assertEquals(4, ncn.getNetworksAndTypes().length); On 2016/04/14 13:46:34, pauljensen wrote: > can we verify the NetIds and Types here? Done.
Description was changed from ========== Fix Failing Tests in org.chromium.net. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ========== to ========== Fix NetworkChangeNotifierTests on Lolipop+ devices. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ==========
The CQ bit was checked by pauljensen@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1747223003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1747223003/60001
Message was sent while issue was closed.
Description was changed from ========== Fix NetworkChangeNotifierTests on Lolipop+ devices. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ========== to ========== Fix NetworkChangeNotifierTests on Lolipop+ devices. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix NetworkChangeNotifierTests on Lolipop+ devices. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 ========== to ========== Fix NetworkChangeNotifierTests on Lolipop+ devices. Fixing flaky test by removing erroneous asserts in the org.chromium.net.NetworkChangeNotifierTest.TestNetworkChangeNotifierAutoDetectObserver callback implementation. On real Lollipop+ devices which have access to a real and unpredictable ConnectivityManager, the strict assertions will fail in a non-deterministic manner. Refactored one test and added one test to the suite. BUG=591103 Committed: https://crrev.com/3cb9af21193cd5f7d65fdac7d0b351ca97ab3669 Cr-Commit-Position: refs/heads/master@{#389755} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3cb9af21193cd5f7d65fdac7d0b351ca97ab3669 Cr-Commit-Position: refs/heads/master@{#389755} |
