Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
836363002:
wifi_sync: add WifiConfigDelegate (Closed)
|
Created:
5 years, 11 months ago by mukesh agrawal Modified:
5 years, 11 months ago CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, maxbogue+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@submit-4.1-network-state-helper Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionwifi_sync: add WifiConfigDelegate
WifiConfigDelegate provides the ability to modify the platform's
Wi-Fi settings. This CL includes an abstract interface class, and
a concrete implementatin for ChromeOS.
This class will be used by WifiCredentialSyncableService, to
effect the changes requested by Chrome Sync.
BUG=chromium:431435
TEST=components_unittests --gtest_filter="Wifi*"
Committed: https://crrev.com/9d40dcfee9aa0ca890d187df2b31365e6c96878f
Cr-Commit-Position: refs/heads/master@{#311991}
Patch Set 1 #Patch Set 2 : minimize CL #Patch Set 3 : git checkout submit-4.4.0-add-delegate-factory #
Total comments: 38
Patch Set 4 : use proper unit testing, fix nits #
Total comments: 8
Patch Set 5 : exercise callbacks in unit tests #
Total comments: 12
Patch Set 6 : fix includes, EXPECT vs ASSERT, other nits #
Messages
Total messages: 23 (3 generated)
quiche@chromium.org changed reviewers: + erikwright@chromium.org, pstew@chromium.org, stevenjb@chromium.org
quiche@chromium.org changed required reviewers: + erikwright@chromium.org, stevenjb@chromium.org
The build failure is due to this being an incremental CL. (I didn't realize the trybots don't have logic to pull in dependent CLs.)
Message was sent while issue was closed.
PS2 removes the FakeWifiConfigDelegate, and the WifiConfigDelegateFactoryChromeOs. They're not needed just yet, and removing them should make this CL more reasonably sized.
This will be used by https://codereview.chromium.org/849693002/ https://codereview.chromium.org/836303004/
https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:28: DISALLOW_COPY_AND_ASSIGN(WifiConfigDelegate); not required (nor the include) because this class is abstract. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.cc:58: base::Bind(DoNothingWithString), // success_callback These appear to be optional, although the documentation doesn't explicitly say that. If they are optional, I would update the docs to say so, and then, here, just do "network_handler::StringResultCallback()", which creates a null callback. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. // WifiConfigDelegate implementation. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:40: // documentation for details of this object's lifetime. Remove "See ...". It is self-evident that the lifetime of a raw pointer is not managed by this class, and presumed unless stated otherwise that it must remain valid. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:30: chromeos::DBusThreadManager::Initialize(); This test is visibly highly dependent on a lot of complex state, including non-obvious things like having to RunUntilIdle, etc. This will be prone to flakiness and will need to be updated over time as the "magic incantation" needed to get everything right evolves. Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a pure-virtual interface and you only need to mock one method of it in order to verify that your class calls it with correct parameters when appropriate. The others, you can implement with NOTIMPLEMENTED().
https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:18: WifiConfigDelegate() {} No need for constructor in abstract class. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:19: virtual ~WifiConfigDelegate() {} This can be protected instead of using DISALLOW... https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.cc:58: base::Bind(DoNothingWithString), // success_callback On 2015/01/13 19:03:13, erikwright wrote: > These appear to be optional, although the documentation doesn't explicitly say > that. If they are optional, I would update the docs to say so, and then, here, > just do "network_handler::StringResultCallback()", which creates a null > callback. Correct. We should update the 'Note on callbacks' comment in network_configuration_handler.h. I will add that some time soon if you don't want to do that here. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. On 2015/01/13 19:03:13, erikwright wrote: > // WifiConfigDelegate implementation. Or just 'WifiConfigDelegate'. "implementation" is implied. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:38: const std::string user_hash_; nit: WS between commented members. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:30: chromeos::DBusThreadManager::Initialize(); On 2015/01/13 19:03:14, erikwright wrote: > This test is visibly highly dependent on a lot of complex state, including > non-obvious things like having to RunUntilIdle, etc. > > This will be prone to flakiness and will need to be updated over time as the > "magic incantation" needed to get everything right evolves. > > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a > pure-virtual interface and you only need to mock one method of it in order to > verify that your class calls it with correct parameters when appropriate. The > others, you can implement with NOTIMPLEMENTED(). While we could create a custom MNCH class and create stubs for the required methods (which would make this a true unit test in that we would only be testing the behavior of this class), I don't agree that we should. I would suggest that the behavior of this class by itself is not very interesting. Rather, the relationship between this class and MNCH is the more important thing to test. We created test interfaces for the fake Shill clients (as used below) specifically to allow this sort of integration test. We maintain many of them and they have not been historically flaky. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:32: network_handler_ = chromeos::NetworkHandler::Get(); I would avoid storing global singletons even in a test. Instead just use NetworkHandler::Get(). https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:46: ->AddProfile(kProfilePath, kUserHash); We don't really need to do this now since we don't yet handle the behavior for an existing network, but when we do we'll want to add an existing service here and test the behavior when setting properties for the existing service. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:66: } nit: WS https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:81: } nit: WS
On 2015/01/13 23:45:40, stevenjb wrote: > On 2015/01/13 19:03:14, erikwright wrote: > > This test is visibly highly dependent on a lot of complex state, including > > non-obvious things like having to RunUntilIdle, etc. > > > > This will be prone to flakiness and will need to be updated over time as the > > "magic incantation" needed to get everything right evolves. > > > > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a > > pure-virtual interface and you only need to mock one method of it in order to > > verify that your class calls it with correct parameters when appropriate. The > > others, you can implement with NOTIMPLEMENTED(). > > While we could create a custom MNCH class and create stubs for the required > methods (which would make this a true unit test in that we would only be testing > the behavior of this class), I don't agree that we should. > > I would suggest that the behavior of this class by itself is not very > interesting. Rather, the relationship between this class and MNCH is the more > important thing to test. > > We created test interfaces for the fake Shill clients (as used below) > specifically to allow this sort of integration test. We maintain many of them > and they have not been historically flaky. I agree that we want to test the integration. But that doesn't necessarily mean using a real MNCH here. Instead, we can use the sync integration tests to exercise the integration between sync, the wifi_sync component, and chromeos/network. (See https://codereview.chromium.org/843483004/patch/1/10006 for an idea of what that looks like. That CL is a little out-of-date, but it should get the idea across.) How does that approach sound?
Normally I would expect the sync integration tests to just test sync, i.e. with a stub non platform specific WifiConfigDelegate, but we could include a chromeos specific integration test with a real delegate as well. I am fine with either approach. On Tue, Jan 13, 2015 at 5:07 PM, <quiche@chromium.org> wrote: > On 2015/01/13 23:45:40, stevenjb wrote: > >> On 2015/01/13 19:03:14, erikwright wrote: >> > This test is visibly highly dependent on a lot of complex state, >> including >> > non-obvious things like having to RunUntilIdle, etc. >> > >> > This will be prone to flakiness and will need to be updated over time >> as the >> > "magic incantation" needed to get everything right evolves. >> > >> > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler >> is a >> > pure-virtual interface and you only need to mock one method of it in >> order >> > to > >> > verify that your class calls it with correct parameters when >> appropriate. >> > The > >> > others, you can implement with NOTIMPLEMENTED(). >> > > While we could create a custom MNCH class and create stubs for the >> required >> methods (which would make this a true unit test in that we would only be >> > testing > >> the behavior of this class), I don't agree that we should. >> > > I would suggest that the behavior of this class by itself is not very >> interesting. Rather, the relationship between this class and MNCH is the >> more >> important thing to test. >> > > We created test interfaces for the fake Shill clients (as used below) >> specifically to allow this sort of integration test. We maintain many of >> them >> and they have not been historically flaky. >> > > I agree that we want to test the integration. But that doesn't necessarily > mean > using a real MNCH here. Instead, we can use the sync integration tests to > exercise the integration between sync, the wifi_sync component, and > chromeos/network. (See https://codereview.chromium. > org/843483004/patch/1/10006 > for an idea of what that looks like. That CL is a little out-of-date, but > it > should get the idea across.) > > How does that approach sound? > > https://codereview.chromium.org/836363002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:18: WifiConfigDelegate() {} On 2015/01/13 23:45:39, stevenjb wrote: > No need for constructor in abstract class. Correct, iff DISALLOW is removed. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:19: virtual ~WifiConfigDelegate() {} On 2015/01/13 23:45:39, stevenjb wrote: > This can be protected instead of using DISALLOW... DISALLOW has nothing to do with the access modifier for the destructor. Making this protected would prevent (or complicate) the destruction of instances of this class. I don't think that's intended or desirable here. As mentioned below, DISALLOW is not required here anyway because of pure-virtual methods. Basically, when defining interfaces do not use DISALLOW. Use it on implementations. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. On 2015/01/13 23:45:39, stevenjb wrote: > On 2015/01/13 19:03:13, erikwright wrote: > > // WifiConfigDelegate implementation. > > Or just 'WifiConfigDelegate'. "implementation" is implied. No. The comment I provided is a convention in Chrome code. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:30: chromeos::DBusThreadManager::Initialize(); On 2015/01/13 23:45:39, stevenjb wrote: > On 2015/01/13 19:03:14, erikwright wrote: > > This test is visibly highly dependent on a lot of complex state, including > > non-obvious things like having to RunUntilIdle, etc. > > > > This will be prone to flakiness and will need to be updated over time as the > > "magic incantation" needed to get everything right evolves. > > > > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a > > pure-virtual interface and you only need to mock one method of it in order to > > verify that your class calls it with correct parameters when appropriate. The > > others, you can implement with NOTIMPLEMENTED(). > > While we could create a custom MNCH class and create stubs for the required > methods (which would make this a true unit test in that we would only be testing > the behavior of this class), I don't agree that we should. > > I would suggest that the behavior of this class by itself is not very > interesting. Rather, the relationship between this class and MNCH is the more > important thing to test. > > We created test interfaces for the fake Shill clients (as used below) > specifically to allow this sort of integration test. We maintain many of them > and they have not been historically flaky. This test is not the place to verify that calling MNCH::CreateConfiguration does what it's supposed to do. This is a unit test. This unit is expected to call MNCH::CreateConfiguration in response to calls to AddToLocalNetworks. That is what should be tested here. You are correct that the class under test is very simple. That's why the unit test will be very simple. An integration test elsewhere can cover the operation of the entire system. Overly coupled unit tests like this one impede development and refactoring because small changes in one part of the system end up needing corresponding changes in a multitude of unit tests that _should_ be unrelated.
https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. On 2015/01/14 14:51:26, erikwright wrote: > On 2015/01/13 23:45:39, stevenjb wrote: > > On 2015/01/13 19:03:13, erikwright wrote: > > > // WifiConfigDelegate implementation. > > > > Or just 'WifiConfigDelegate'. "implementation" is implied. > > No. The comment I provided is a convention in Chrome code. Opinions vary on this. Chat with pkasting@ for more background. All of the above are perfectly fine.
Thanks, all. Please take another look. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:18: WifiConfigDelegate() {} On 2015/01/13 23:45:39, stevenjb wrote: > No need for constructor in abstract class. Done. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:18: WifiConfigDelegate() {} On 2015/01/14 14:51:26, erikwright wrote: > On 2015/01/13 23:45:39, stevenjb wrote: > > No need for constructor in abstract class. > > Correct, iff DISALLOW is removed. Acknowledged. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:19: virtual ~WifiConfigDelegate() {} On 2015/01/14 14:51:26, erikwright wrote: > On 2015/01/13 23:45:39, stevenjb wrote: > > This can be protected instead of using DISALLOW... > > DISALLOW has nothing to do with the access modifier for the destructor. Making > this protected would prevent (or complicate) the destruction of instances of > this class. I don't think that's intended or desirable here. > > As mentioned below, DISALLOW is not required here anyway because of pure-virtual > methods. > > Basically, when defining interfaces do not use DISALLOW. Use it on > implementations. Acknowledged. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate.h:28: DISALLOW_COPY_AND_ASSIGN(WifiConfigDelegate); On 2015/01/13 19:03:13, erikwright wrote: > not required (nor the include) because this class is abstract. Done. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.cc:58: base::Bind(DoNothingWithString), // success_callback On 2015/01/13 19:03:13, erikwright wrote: > These appear to be optional, although the documentation doesn't explicitly say > that. If they are optional, I would update the docs to say so, and then, here, > just do "network_handler::StringResultCallback()", which creates a null > callback. Done. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.cc:58: base::Bind(DoNothingWithString), // success_callback On 2015/01/13 23:45:39, stevenjb wrote: > On 2015/01/13 19:03:13, erikwright wrote: > > These appear to be optional, although the documentation doesn't explicitly say > > that. If they are optional, I would update the docs to say so, and then, here, > > just do "network_handler::StringResultCallback()", which creates a null > > callback. > > Correct. We should update the 'Note on callbacks' comment in > network_configuration_handler.h. I will add that some time soon if you don't > want to do that here. Working on updating the NetworkConfigurationHandler documentation in https://codereview.chromium.org/799313004/. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. On 2015/01/13 19:03:13, erikwright wrote: > // WifiConfigDelegate implementation. Done. Went with "BaseClass implenentation." for consistency with wifi_credential_syncable_service.h. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:38: const std::string user_hash_; On 2015/01/13 23:45:39, stevenjb wrote: > nit: WS between commented members. Done. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:40: // documentation for details of this object's lifetime. On 2015/01/13 19:03:14, erikwright wrote: > Remove "See ...". It is self-evident that the lifetime of a raw pointer is not > managed by this class, and presumed unless stated otherwise that it must remain > valid. Done. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:30: chromeos::DBusThreadManager::Initialize(); On 2015/01/13 19:03:14, erikwright wrote: > This test is visibly highly dependent on a lot of complex state, including > non-obvious things like having to RunUntilIdle, etc. > > This will be prone to flakiness and will need to be updated over time as the > "magic incantation" needed to get everything right evolves. > > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a > pure-virtual interface and you only need to mock one method of it in order to > verify that your class calls it with correct parameters when appropriate. The > others, you can implement with NOTIMPLEMENTED(). Done. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:30: chromeos::DBusThreadManager::Initialize(); On 2015/01/13 23:45:39, stevenjb wrote: > On 2015/01/13 19:03:14, erikwright wrote: > > This test is visibly highly dependent on a lot of complex state, including > > non-obvious things like having to RunUntilIdle, etc. > > > > This will be prone to flakiness and will need to be updated over time as the > > "magic incantation" needed to get everything right evolves. > > > > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a > > pure-virtual interface and you only need to mock one method of it in order to > > verify that your class calls it with correct parameters when appropriate. The > > others, you can implement with NOTIMPLEMENTED(). > > While we could create a custom MNCH class and create stubs for the required > methods (which would make this a true unit test in that we would only be testing > the behavior of this class), I don't agree that we should. > > I would suggest that the behavior of this class by itself is not very > interesting. Rather, the relationship between this class and MNCH is the more > important thing to test. > > We created test interfaces for the fake Shill clients (as used below) > specifically to allow this sort of integration test. We maintain many of them > and they have not been historically flaky. Acknowledged. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:30: chromeos::DBusThreadManager::Initialize(); On 2015/01/14 14:51:26, erikwright wrote: > On 2015/01/13 23:45:39, stevenjb wrote: > > On 2015/01/13 19:03:14, erikwright wrote: > > > This test is visibly highly dependent on a lot of complex state, including > > > non-obvious things like having to RunUntilIdle, etc. > > > > > > This will be prone to flakiness and will need to be updated over time as the > > > "magic incantation" needed to get everything right evolves. > > > > > > Luckily, you don't need any of it. ManagedNetworkConfigurationHandler is a > > > pure-virtual interface and you only need to mock one method of it in order > to > > > verify that your class calls it with correct parameters when appropriate. > The > > > others, you can implement with NOTIMPLEMENTED(). > > > > While we could create a custom MNCH class and create stubs for the required > > methods (which would make this a true unit test in that we would only be > testing > > the behavior of this class), I don't agree that we should. > > > > I would suggest that the behavior of this class by itself is not very > > interesting. Rather, the relationship between this class and MNCH is the more > > important thing to test. > > > > We created test interfaces for the fake Shill clients (as used below) > > specifically to allow this sort of integration test. We maintain many of them > > and they have not been historically flaky. > > This test is not the place to verify that calling MNCH::CreateConfiguration does > what it's supposed to do. > > This is a unit test. This unit is expected to call MNCH::CreateConfiguration in > response to calls to AddToLocalNetworks. That is what should be tested here. > > You are correct that the class under test is very simple. That's why the unit > test will be very simple. > > An integration test elsewhere can cover the operation of the entire system. > > Overly coupled unit tests like this one impede development and refactoring > because small changes in one part of the system end up needing corresponding > changes in a multitude of unit tests that _should_ be unrelated. Acknowledged. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:32: network_handler_ = chromeos::NetworkHandler::Get(); On 2015/01/13 23:45:39, stevenjb wrote: > I would avoid storing global singletons even in a test. Instead just use > NetworkHandler::Get(). Done. (Offending code removed.) https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:46: ->AddProfile(kProfilePath, kUserHash); On 2015/01/13 23:45:39, stevenjb wrote: > We don't really need to do this now since we don't yet handle the behavior for > an existing network, but when we do we'll want to add an existing service here > and test the behavior when setting properties for the existing service. Acknowledged. https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:66: } On 2015/01/13 23:45:39, stevenjb wrote: > nit: WS Done. (Offending code removed.) https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:81: } On 2015/01/13 23:45:39, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:21: class FakeManagedNetworkConfigurationHandler Taking a cue from the advice on FakeWifiConfigDelegate, I've placed the FakeManagedNetworkConfigurationHandler in this file. (This file is the only place that uses it.)
https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. On 2015/01/14 17:08:56, stevenjb wrote: > On 2015/01/14 14:51:26, erikwright wrote: > > On 2015/01/13 23:45:39, stevenjb wrote: > > > On 2015/01/13 19:03:13, erikwright wrote: > > > > // WifiConfigDelegate implementation. > > > > > > Or just 'WifiConfigDelegate'. "implementation" is implied. > > > > No. The comment I provided is a convention in Chrome code. > > Opinions vary on this. Chat with pkasting@ for more background. All of the above > are perfectly fine. I'm curious if there is a review thread somewhere you can point me to. If you've already discussed this with him I don't need to disturb him about it. https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:62: ++create_configuration_count_; You don't really need a call count, since your tests each exercise a single invocation. But you should be testing the success/failure callbacks of your code under test. The simplest way to do it, then, would be to declare members: bool called_; // init to false StringResultCallback callback_; ErrorCallback error_callback_; And in this method: EXPECT_FALSE(called_); called_ = true; callback_ = callback; error_callback_ = error_callback; And then in your test: ASSERT_TRUE(create_configuration_called()); if (!callback_.is_null()) callback_.Run() And also have at least one test that invokes the error callback. Note that this is actually an example of code that is much easier to test using the mock-based strategy here, than it would have been with the more complex initial test strategy. Consider this in the perspective of Bruce Dawson's talk yesterday where he pointed out that error handling code is rife with bugs because it is rarely tested. https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:107: size_t GetCreateConfigurationCount() const { nit: accessor style: size_t create_configuration_call_count() const {} https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:147: size_t GetCreateConfigurationCount() const { nit: accessor style: create_configuration_call_count
https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos.h (right): https://codereview.chromium.org/836363002/diff/40001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos.h:32: // Implementation of WifiConfigDelegate. On 2015/01/15 15:12:50, erikwright wrote: > On 2015/01/14 17:08:56, stevenjb wrote: > > On 2015/01/14 14:51:26, erikwright wrote: > > > On 2015/01/13 23:45:39, stevenjb wrote: > > > > On 2015/01/13 19:03:13, erikwright wrote: > > > > > // WifiConfigDelegate implementation. > > > > > > > > Or just 'WifiConfigDelegate'. "implementation" is implied. > > > > > > No. The comment I provided is a convention in Chrome code. > > > > Opinions vary on this. Chat with pkasting@ for more background. All of the > above > > are perfectly fine. > > I'm curious if there is a review thread somewhere you can point me to. If you've > already discussed this with him I don't need to disturb him about it. https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/Bz8WN-mlS... Brought up again more recently: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/overri...
FWIW, I find trivial unit tests like this one cost more to maintain than they save finding logic errors. That said, I'd rather not bog this review down in a philosophical discussion, we can re-examine the tests later as the implementation gains complexity. lgtm
Thanks, folks. erik: PTAL steven: you're welcome to TAL if you'd like, of course. :) https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:62: ++create_configuration_count_; On 2015/01/15 15:12:51, erikwright wrote: > You don't really need a call count, since your tests each exercise a single > invocation. But you should be testing the success/failure callbacks of your code > under test. > > The simplest way to do it, then, would be to declare members: > > bool called_; // init to false > StringResultCallback callback_; > ErrorCallback error_callback_; > > And in this method: > > EXPECT_FALSE(called_); > called_ = true; > callback_ = callback; > error_callback_ = error_callback; > > And then in your test: > > ASSERT_TRUE(create_configuration_called()); > if (!callback_.is_null()) > callback_.Run() > > And also have at least one test that invokes the error callback. > > Note that this is actually an example of code that is much easier to test using > the mock-based strategy here, than it would have been with the more complex > initial test strategy. Consider this in the perspective of Bruce Dawson's talk > yesterday where he pointed out that error handling code is rife with bugs > because it is rarely tested. Done. Thanks for the detailed advice here. I've updated the CL accordingly. I definitely see your point about exercising the error callback. But I'd like to ask a question about the need to exercise the success callback. Namely: given that the the current implementation always passes a null success callback, why do we want to exercise the success callback in the unit tests? Is it, e.g., to a) more fully document how the ManagedNetworkConfigurationHandler interacts with this class, b) to allow for future changes that use non-null success callbacks, or c) something else? https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:107: size_t GetCreateConfigurationCount() const { On 2015/01/15 15:12:51, erikwright wrote: > nit: accessor style: > > size_t create_configuration_call_count() const {} Done. https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:147: size_t GetCreateConfigurationCount() const { On 2015/01/15 15:12:50, erikwright wrote: > nit: accessor style: create_configuration_call_count Done.
LGTM. With regards to the test being overkill, in fact it would be overkill to define a delegate interface for a single method but I understand that the delegate will become more complex over time as the functionality is rounded out. Otherwise, a simple callback would suffice, and this test would be part of the test of whoever builds that callback. https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/60001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:62: ++create_configuration_count_; On 2015/01/15 19:05:55, mukesh agrawal wrote: > On 2015/01/15 15:12:51, erikwright wrote: > > You don't really need a call count, since your tests each exercise a single > > invocation. But you should be testing the success/failure callbacks of your > code > > under test. > > > > The simplest way to do it, then, would be to declare members: > > > > bool called_; // init to false > > StringResultCallback callback_; > > ErrorCallback error_callback_; > > > > And in this method: > > > > EXPECT_FALSE(called_); > > called_ = true; > > callback_ = callback; > > error_callback_ = error_callback; > > > > And then in your test: > > > > ASSERT_TRUE(create_configuration_called()); > > if (!callback_.is_null()) > > callback_.Run() > > > > And also have at least one test that invokes the error callback. > > > > Note that this is actually an example of code that is much easier to test > using > > the mock-based strategy here, than it would have been with the more complex > > initial test strategy. Consider this in the perspective of Bruce Dawson's talk > > yesterday where he pointed out that error handling code is rife with bugs > > because it is rarely tested. > > Done. > > Thanks for the detailed advice here. I've updated the CL accordingly. > > I definitely see your point about exercising the error callback. But I'd like to > ask a question about the need to exercise the success callback. > > Namely: given that the the current implementation always passes a null success > callback, why do we want to exercise the success callback in the unit tests? Is > it, e.g., to a) more fully document how the ManagedNetworkConfigurationHandler > interacts with this class, b) to allow for future changes that use non-null > success callbacks, or c) something else? The fact that the success callback is null is not a part of the public contract of the class under test. It can change in the future. You could easily see someone adding code to log something, or other purposes in the future. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:21: using chromeos::network_handler::DictionaryResultCallback; #include chromeos/network/network_handler_callbacks.h https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:34: NOTIMPLEMENTED(); #include logging.h https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:162: if (!create_configuration_success_callback().is_null()) { nit: braces not required. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:176: const StringResultCallback& create_configuration_success_callback() const { nit: this accessor is not required since you only invoke it from another member function. You could inline it. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:198: EXPECT_TRUE(create_configuration_called()); nit: I would make all of these ASSERTs since running the callback doesn't make sense if they are false. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:219: EXPECT_TRUE(create_configuration_success_callback().is_null()); nit: not required. You already verified that the method wasn't called.
thanks, erik. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... File components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc (right): https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:21: using chromeos::network_handler::DictionaryResultCallback; On 2015/01/16 14:42:15, erikwright wrote: > #include chromeos/network/network_handler_callbacks.h Done. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:34: NOTIMPLEMENTED(); On 2015/01/16 14:42:15, erikwright wrote: > #include logging.h Done. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:162: if (!create_configuration_success_callback().is_null()) { On 2015/01/16 14:42:15, erikwright wrote: > nit: braces not required. Done. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:176: const StringResultCallback& create_configuration_success_callback() const { On 2015/01/16 14:42:15, erikwright wrote: > nit: this accessor is not required since you only invoke it from another member > function. You could inline it. Done. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:198: EXPECT_TRUE(create_configuration_called()); On 2015/01/16 14:42:15, erikwright wrote: > nit: I would make all of these ASSERTs since running the callback doesn't make > sense if they are false. Done. https://codereview.chromium.org/836363002/diff/80001/components/wifi_sync/wif... components/wifi_sync/wifi_config_delegate_chromeos_unittest.cc:219: EXPECT_TRUE(create_configuration_success_callback().is_null()); On 2015/01/16 14:42:15, erikwright wrote: > nit: not required. You already verified that the method wasn't called. Done.
The CQ bit was checked by quiche@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/836363002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/9d40dcfee9aa0ca890d187df2b31365e6c96878f Cr-Commit-Position: refs/heads/master@{#311991} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
