Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
843483004:
sync: add more integration tests for wifi_sync (Closed)
|
Created:
5 years, 11 months ago by mukesh agrawal Modified:
5 years, 10 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.3-syncable-service Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionsync: add more integration tests for wifi_sync
Add tests which exercise wifi_sync's ability to add networks.
These tests add a network to the platform wifi config, and then
verify that the new network appears in other sync clients.
BUG=chromium:431435
TEST=sync_integration_tests --gtest_filter="*Wifi*"
Committed: https://crrev.com/0c9f1c75eb83c5aa231696e49f72d004e6e10026
Cr-Commit-Position: refs/heads/master@{#315070}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 18
Patch Set 3 : address feedback on PS2 from stevenjb #Patch Set 4 : add assertions for |credential| #Patch Set 5 : fix compilation errors #
Total comments: 6
Patch Set 6 : use directory-level deps (instead of file-level) #
Total comments: 34
Patch Set 7 : address feedback from pneubeck, on PS6 #
Total comments: 3
Patch Set 8 : address pneubeck's comments on PS7 #
Total comments: 3
Patch Set 9 : rebase, and resolve (trivial) conflict #Messages
Total messages: 30 (11 generated)
quiche@chromium.org changed reviewers: + erikwright@chromium.org, pavely@chromium.org, stevenjb@chromium.org
quiche@chromium.org changed required reviewers: + pavely@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.
quiche@chromium.org changed reviewers: - erikwright@chromium.org, pavely@chromium.org, stevenjb@chromium.org
Message was sent while issue was closed.
quiche@chromium.org changed required reviewers: - pavely@chromium.org, stevenjb@chromium.org
Message was sent while issue was closed.
This CL depends on https://codereview.chromium.org/876833002/
quiche@chromium.org changed reviewers: + pavely@chromium.org, stevenjb@chromium.org
quiche@chromium.org changed required reviewers: + pavely@chromium.org, stevenjb@chromium.org
quiche@chromium.org changed reviewers: + erikwright@chromium.org
stevenjb@chromium.org changed reviewers: + pneubeck@chromium.org
+pneubeck@ - can you also take a look at wifi_credentials_helper_chromeos.cc? Thanks. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:21: // SyncTest https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:54: "fake-ssid", wifi_sync::SECURITY_CLASS_PSK, "fake_passphrase")); I would call MakeWifiCredential separately and assert that it succeeds (see comment in MakeWifiCredential). https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:57: ASSERT_TRUE(wifi_credentials_helper::ProfileMatchesVerifier(0)); The last two could probably be EXPECT instead of assert? https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:21: // SyncTest https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:59: ASSERT_TRUE(wifi_credentials_helper::AllProfilesMatch()); EXPECT? https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:31: namespace { WS https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:98: return SetUpChromeOs(); This is a little confusing since it is not defined here. I would put the chromeos specific code in the namespace wifi_credentials_helper::chromeos and explicitly call wifi_credentials_helper::chromeos::SetuUpChromeOS (and I would go ahead and use the redundant naming to be extra clear). https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:156: return *credential; The implict copy here is confusing. I would have MakeWifiCredential return a scoped_ptr and do the copy in the caller. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:12: namespace wifi_credentials_helper { I would add a 'chromeos' namespace here, just to be extra clear, and qualify the calls to these helpers.
Thanks + PTAL. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:21: On 2015/01/28 21:29:43, stevenjb wrote: > // SyncTest Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:54: "fake-ssid", wifi_sync::SECURITY_CLASS_PSK, "fake_passphrase")); On 2015/01/28 21:29:43, stevenjb wrote: > I would call MakeWifiCredential separately and assert that it succeeds (see > comment in MakeWifiCredential). > Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:57: ASSERT_TRUE(wifi_credentials_helper::ProfileMatchesVerifier(0)); On 2015/01/28 21:29:43, stevenjb wrote: > The last two could probably be EXPECT instead of assert? Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:21: On 2015/01/28 21:29:43, stevenjb wrote: > // SyncTest Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:59: ASSERT_TRUE(wifi_credentials_helper::AllProfilesMatch()); On 2015/01/28 21:29:43, stevenjb wrote: > EXPECT? Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:31: namespace { On 2015/01/28 21:29:43, stevenjb wrote: > WS Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:98: return SetUpChromeOs(); On 2015/01/28 21:29:43, stevenjb wrote: > This is a little confusing since it is not defined here. I would put the > chromeos specific code in the namespace wifi_credentials_helper::chromeos and > explicitly call wifi_credentials_helper::chromeos::SetuUpChromeOS (and I would > go ahead and use the redundant naming to be extra clear). Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:156: return *credential; On 2015/01/28 21:29:43, stevenjb wrote: > The implict copy here is confusing. I would have MakeWifiCredential return a > scoped_ptr and do the copy in the caller. Done. https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h (right): https://codereview.chromium.org/843483004/diff/20001/chrome/browser/sync/test... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:12: namespace wifi_credentials_helper { On 2015/01/28 21:29:43, stevenjb wrote: > I would add a 'chromeos' namespace here, just to be extra clear, and qualify the > calls to these helpers. Done.
lgtm https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:26: } nit: There should still be WS between method implementations. If non overridden methods followed this, use another comment to separate those. https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:26: } WS
LGTM. https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/DEPS (right): https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/DEPS:2: "+components/onc/onc_constants.h", # wifi_credentials tests It seems exceptional, to me, to name a single file like this. I would just use components/onc unless there is some reason that would be inappropriate.
Thanks. I'll ping pavely for the final LGTM. https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/DEPS (right): https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/DEPS:2: "+components/onc/onc_constants.h", # wifi_credentials tests On 2015/01/29 15:51:34, erikwright wrote: > It seems exceptional, to me, to name a single file like this. I would just use > components/onc unless there is some reason that would be inappropriate. Done.
https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:46: IN_PROC_BROWSER_TEST_F(SingleClientWifiCredentialsSyncTest, SingleCredential) { is there a reason why this is a browser test but the file doesn't have the suffix _browsertest.cc ? https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:59: EXPECT_TRUE(wifi_credentials_helper::ProfileMatchesVerifier(0)); to not rely on some function that compares all profiles (and which might have some bug itself), you could 'manually' compare the content in profile_b with the credential above. i.e. something like EXPECT_EQ(1, GetCredentialsForProfile(profile_b_index).size()); EXPECT_EQ("fake-ssid", GetCredentialsForProfile(profile_b_index)[0]->ssid); https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:30: } usually we put empty lines between function /definitions/. probably 'git cl format' cleans this up for you (or git cl lint might complain maybe). https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:61: EXPECT_TRUE(wifi_credentials_helper::AllProfilesMatch()); same comment as for the other test https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:37: NOTREACHED(); note that this does not crash on release builds (thus also not on non-dbg test bots) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:49: return kProfilePrefix + ChromeOsUserHashForBrowserContext(context); if this is for testing only, it should be irrelevant what path you're using. you could clarify that in a comment. otherwise this would copy the logic how to construct the profile path and would have to be kept in sync. which would be problematic. please consider using NetworkProfileHandler::GetProfileForUserhash instead to get the profile path in production code. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:54: DCHECK(::chromeos::DBusThreadManager::Get()->GetShillProfileClient()); the DCHECKs (here and in the other Get*()) are not really required as stuff will crash when dereferencing the returned pointer anyways and these crashes are nearby the Get*() call, thus easy to trace back. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:97: return true; return value not required? https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:105: if (!onc_properties) { nit: equivalent to CHECK(onc_properties) << ... https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:12: namespace wifi_credentials_helper { some of this code is for testing only, please consider making that more clear. E.g. at each function (function name or at least a comment) or the namespace&filename. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:19: // Performs ChromeOS-specific setup for a given sync client, given should be called only after SetUpChromeOs ? https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:21: bool SetupClientForProfileChromeOs(const Profile* profile); if these function can use BrowserContext instead of Profile, please do so.
pneubeck: thanks and PTAL. https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:26: } On 2015/01/28 23:02:50, stevenjb wrote: > nit: There should still be WS between method implementations. If non overridden > methods followed this, use another comment to separate those. Done. https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/80001/chrome/browser/sync/test... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:26: } On 2015/01/28 23:02:50, stevenjb wrote: > WS Done. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:46: IN_PROC_BROWSER_TEST_F(SingleClientWifiCredentialsSyncTest, SingleCredential) { On 2015/01/30 14:38:45, pneubeck wrote: > is there a reason why this is a browser test but the file doesn't have the > suffix _browsertest.cc ? Just following the convention in this part of the tree. (All of the subclasses of SyncTest have browser tests, and none are named _browsertest.cc.) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:59: EXPECT_TRUE(wifi_credentials_helper::ProfileMatchesVerifier(0)); On 2015/01/30 14:38:45, pneubeck wrote: > to not rely on some function that compares all profiles (and which might have > some bug itself), you could 'manually' compare the content in profile_b with the > credential above. > i.e. something like > EXPECT_EQ(1, GetCredentialsForProfile(profile_b_index).size()); > EXPECT_EQ("fake-ssid", GetCredentialsForProfile(profile_b_index)[0]->ssid); I could, but that would diverge from the convention for sync integration tests. (I believe they'are all structured to factor the comparison logic out into a helper file.) Apart from consistent code structuring, the helper method also provides error messages that are consistent with other data types. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:30: } On 2015/01/30 14:38:45, pneubeck wrote: > usually we put empty lines between function /definitions/. > probably 'git cl format' cleans this up for you (or git cl lint might complain > maybe). Done. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:61: EXPECT_TRUE(wifi_credentials_helper::AllProfilesMatch()); On 2015/01/30 14:38:45, pneubeck wrote: > same comment as for the other test Same response. :) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:37: NOTREACHED(); On 2015/01/30 14:38:45, pneubeck wrote: > note that this does not crash on release builds (thus also not on non-dbg test > bots) Thanks for the info. I wasn't aware of that, but I think that behavior is acceptable. I expect that, before anyone enables this code for other platforms, they will build and test in debug mode. (I believe that's a reasonable expectation, since git cl try includes dbg bots. Let me know if I'm mistaken.) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:49: return kProfilePrefix + ChromeOsUserHashForBrowserContext(context); On 2015/01/30 14:38:45, pneubeck wrote: > if this is for testing only, it should be irrelevant what path you're using. you > could clarify that in a comment. Are you referring to the use of kProfilePrefix (which is primarily stylistic), or the fact that we derive the profile path from the browser context? The latter has a functional purpose. (We need distinct profiles for each SyncableService. Otherwise, we'll mistakenly believe that the clients are synchronized, because they're sharing network profile data.) I'm happy to add comments to explain either, or both points. I just want to make sure that I don't over-comment. > please consider using NetworkProfileHandler::GetProfileForUserhash instead to > get the profile path in production code. Acknowledged. The production code currently doesn't deal with profile paths, since MNCH operates on either a user hash, or a service path. But I'll keep this in mind in case I add code that does use profile paths. (There is some code in wifi_sync/network_state_helper_chromeos.cc, but right now it's only called from sync integration tests. I have an open bug to move that code into this directory: crbug.com/449020.) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:54: DCHECK(::chromeos::DBusThreadManager::Get()->GetShillProfileClient()); On 2015/01/30 14:38:45, pneubeck wrote: > the DCHECKs (here and in the other Get*()) are not really required as stuff will > crash when dereferencing the returned pointer anyways and these crashes are > nearby the Get*() call, thus easy to trace back. Agreed that a nullptr would cause a crash. The reason I've placed DCHECKs is so that I can get a lead on the error without having to pull up a debugger, and deciphering a stack trace that may be complicated by optimizations. And since these are DCHECKs, they won't bloat/slow release code. For those reasons, I'd like to keep the DCHECKs in place. Let me know if you still believe they should be removed. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:97: return true; On 2015/01/30 14:38:45, pneubeck wrote: > return value not required? I think your point is that, since the function always returns true, the return value could be omitted. That's true. But then I'd need to add a "return true" to the ChromeOS specific code in wifi_credentials_helper.cc:SetupClientForProfile. I believe the code is cleaner as-is. But I can change it if you disagree. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:105: if (!onc_properties) { On 2015/01/30 14:38:45, pneubeck wrote: > nit: equivalent to > CHECK(onc_properties) << ... Done. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:12: namespace wifi_credentials_helper { On 2015/01/30 14:38:45, pneubeck wrote: > some of this code is for testing only, please consider making that more clear. > E.g. at each function (function name or at least a comment) or the > namespace&filename. Actually, _all_ of this code is for testing only :). This code exists only to support the tests in {single,two}_client_wifi_credentials_sync_test.cc. I believe that the fact that this is test support code is communicated by the location of the code ("sync/test/integration"), and the file name ("helper"). Given that all of these functions are for testing, I think per-function comments/names would be noise. I could add a file-level comment, if you'd like. (I think it would be inconsistent with the other helpers in this directory, but I don't feel too strongly about that.) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:19: // Performs ChromeOS-specific setup for a given sync client, given On 2015/01/30 14:38:45, pneubeck wrote: > should be called only after SetUpChromeOs ? Done. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:21: bool SetupClientForProfileChromeOs(const Profile* profile); On 2015/01/30 14:38:45, pneubeck wrote: > if these function can use BrowserContext instead of Profile, please do so. Done.
lgtm
lgtm with a few comments for consideration https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:59: EXPECT_TRUE(wifi_credentials_helper::ProfileMatchesVerifier(0)); On 2015/01/30 22:28:14, mukesh agrawal wrote: > On 2015/01/30 14:38:45, pneubeck wrote: > > to not rely on some function that compares all profiles (and which might have > > some bug itself), you could 'manually' compare the content in profile_b with > the > > credential above. > > i.e. something like > > EXPECT_EQ(1, GetCredentialsForProfile(profile_b_index).size()); > > EXPECT_EQ("fake-ssid", GetCredentialsForProfile(profile_b_index)[0]->ssid); > > I could, but that would diverge from the convention for sync integration tests. > (I believe they'are all structured to factor the comparison logic out into a > helper file.) > > Apart from consistent code structuring, the helper method also provides error > messages that are consistent with other data types. my point was more about what you're comparing with. I don't see that you're comparing with |credential| directly. Instead there is additional code involved that must correctly store the credential in the verifier. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:49: return kProfilePrefix + ChromeOsUserHashForBrowserContext(context); On 2015/01/30 22:28:14, mukesh agrawal wrote: > On 2015/01/30 14:38:45, pneubeck wrote: > > if this is for testing only, it should be irrelevant what path you're using. > you > > could clarify that in a comment. > > Are you referring to the use of kProfilePrefix (which is primarily stylistic), > or the fact that we derive the profile path from the browser context? The latter > has a functional purpose. (We need distinct profiles for each SyncableService. > Otherwise, we'll mistakenly believe that the clients are synchronized, because > they're sharing network profile data.) > > I'm happy to add comments to explain either, or both points. I just want to make > sure that I don't over-comment. > > > please consider using NetworkProfileHandler::GetProfileForUserhash instead to > > get the profile path in production code. > > Acknowledged. > > The production code currently doesn't deal with profile paths, since MNCH > operates on either a user hash, or a service path. But I'll keep this in mind in > case I add code that does use profile paths. > > (There is some code in wifi_sync/network_state_helper_chromeos.cc, but right now > it's only called from sync integration tests. I have an open bug to move that > code into this directory: crbug.com/449020.) makes sense. I'd just add a short comment in front of the return, like "Return arbitrary, but distinct profile paths per context." https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:54: DCHECK(::chromeos::DBusThreadManager::Get()->GetShillProfileClient()); On 2015/01/30 22:28:14, mukesh agrawal wrote: > On 2015/01/30 14:38:45, pneubeck wrote: > > the DCHECKs (here and in the other Get*()) are not really required as stuff > will > > crash when dereferencing the returned pointer anyways and these crashes are > > nearby the Get*() call, thus easy to trace back. > > Agreed that a nullptr would cause a crash. > > The reason I've placed DCHECKs is so that I can get a lead on the error without > having to pull up a debugger, and deciphering a stack trace that may be > complicated by optimizations. And since these are DCHECKs, they won't bloat/slow > release code. > > For those reasons, I'd like to keep the DCHECKs in place. Let me know if you > still believe they should be removed. no, keep them if you find them helpful. just keep them at a reasonable number / don't bloat the code with them. e.g. this function is at the limit, IMO (more DCHECK than actual code). (this function might be more readable by assign the DBusThreadManager to a variable) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:97: return true; On 2015/01/30 22:28:14, mukesh agrawal wrote: > On 2015/01/30 14:38:45, pneubeck wrote: > > return value not required? > > I think your point is that, since the function always returns true, the return > value could be omitted. > > That's true. But then I'd need to add a "return true" to the ChromeOS specific > code in wifi_credentials_helper.cc:SetupClientForProfile. > > I believe the code is cleaner as-is. But I can change it if you disagree. If you want to keep it, I'd suggest to at least document what the return value means. Exposing to the caller that this function can never fail, is however preferable IMO. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:12: namespace wifi_credentials_helper { On 2015/01/30 22:28:14, mukesh agrawal wrote: > On 2015/01/30 14:38:45, pneubeck wrote: > > some of this code is for testing only, please consider making that more clear. > > E.g. at each function (function name or at least a comment) or the > > namespace&filename. > > Actually, _all_ of this code is for testing only :). This code exists only to > support the tests in {single,two}_client_wifi_credentials_sync_test.cc. > > I believe that the fact that this is test support code is communicated by the > location of the code ("sync/test/integration"), and the file name ("helper"). > > Given that all of these functions are for testing, I think per-function > comments/names would be noise. I could add a file-level comment, if you'd like. > (I think it would be inconsistent with the other helpers in this directory, but > I don't feel too strongly about that.) sorry, I actually missed the /test/ part of the path. again, this is not so common in chrome. https://codereview.chromium.org/843483004/diff/120001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right): https://codereview.chromium.org/843483004/diff/120001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:101: return wifi_credentials_helper::chromeos::SetUpChromeOs(); Please remove the 'return'.
https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc:59: EXPECT_TRUE(wifi_credentials_helper::ProfileMatchesVerifier(0)); On 2015/01/31 10:26:40, pneubeck wrote: > On 2015/01/30 22:28:14, mukesh agrawal wrote: > > On 2015/01/30 14:38:45, pneubeck wrote: > > > to not rely on some function that compares all profiles (and which might > have > > > some bug itself), you could 'manually' compare the content in profile_b with > > the > > > credential above. > > > i.e. something like > > > EXPECT_EQ(1, GetCredentialsForProfile(profile_b_index).size()); > > > EXPECT_EQ("fake-ssid", > GetCredentialsForProfile(profile_b_index)[0]->ssid); > > > > I could, but that would diverge from the convention for sync integration > tests. > > (I believe they'are all structured to factor the comparison logic out into a > > helper file.) > > > > Apart from consistent code structuring, the helper method also provides error > > messages that are consistent with other data types. > > my point was more about what you're comparing with. I don't see that you're > comparing with |credential| directly. Instead there is additional code involved > that must correctly store the credential in the verifier. AIUI, the verifier is actually an important part of the test framework. It's a SyncableService, but does not participate in the sync protocol. It provides protection against the case where all sync clients converge to a consistent, but incorrect, state. I've taken your suggestion, and applied it to the verifier. (i.e., first check that the verifier got the credential, then check profiles against the verifier.) https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:49: return kProfilePrefix + ChromeOsUserHashForBrowserContext(context); On 2015/01/31 10:26:40, pneubeck wrote: > On 2015/01/30 22:28:14, mukesh agrawal wrote: > > On 2015/01/30 14:38:45, pneubeck wrote: > > > if this is for testing only, it should be irrelevant what path you're using. > > you > > > could clarify that in a comment. > > > > Are you referring to the use of kProfilePrefix (which is primarily stylistic), > > or the fact that we derive the profile path from the browser context? The > latter > > has a functional purpose. (We need distinct profiles for each SyncableService. > > Otherwise, we'll mistakenly believe that the clients are synchronized, because > > they're sharing network profile data.) > > > > I'm happy to add comments to explain either, or both points. I just want to > make > > sure that I don't over-comment. > > > > > please consider using NetworkProfileHandler::GetProfileForUserhash instead > to > > > get the profile path in production code. > > > > Acknowledged. > > > > The production code currently doesn't deal with profile paths, since MNCH > > operates on either a user hash, or a service path. But I'll keep this in mind > in > > case I add code that does use profile paths. > > > > (There is some code in wifi_sync/network_state_helper_chromeos.cc, but right > now > > it's only called from sync integration tests. I have an open bug to move that > > code into this directory: crbug.com/449020.) > > makes sense. > I'd just add a short comment in front of the return, like "Return arbitrary, but > distinct profile paths per context." Done. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:54: DCHECK(::chromeos::DBusThreadManager::Get()->GetShillProfileClient()); On 2015/01/31 10:26:40, pneubeck wrote: > On 2015/01/30 22:28:14, mukesh agrawal wrote: > > On 2015/01/30 14:38:45, pneubeck wrote: > > > the DCHECKs (here and in the other Get*()) are not really required as stuff > > will > > > crash when dereferencing the returned pointer anyways and these crashes are > > > nearby the Get*() call, thus easy to trace back. > > > > Agreed that a nullptr would cause a crash. > > > > The reason I've placed DCHECKs is so that I can get a lead on the error > without > > having to pull up a debugger, and deciphering a stack trace that may be > > complicated by optimizations. And since these are DCHECKs, they won't > bloat/slow > > release code. > > > > For those reasons, I'd like to keep the DCHECKs in place. Let me know if you > > still believe they should be removed. > > no, keep them if you find them helpful. > just keep them at a reasonable number / don't bloat the code with them. e.g. > this function is at the limit, IMO (more DCHECK than actual code). > > (this function might be more readable by assign the DBusThreadManager to a > variable) Acknowledged. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:97: return true; On 2015/01/31 10:26:40, pneubeck wrote: > On 2015/01/30 22:28:14, mukesh agrawal wrote: > > On 2015/01/30 14:38:45, pneubeck wrote: > > > return value not required? > > > > I think your point is that, since the function always returns true, the return > > value could be omitted. > > > > That's true. But then I'd need to add a "return true" to the ChromeOS specific > > code in wifi_credentials_helper.cc:SetupClientForProfile. > > > > I believe the code is cleaner as-is. But I can change it if you disagree. > > If you want to keep it, I'd suggest to at least document what the return value > means. > Exposing to the caller that this function can never fail, is however preferable > IMO. Done. https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h (right): https://codereview.chromium.org/843483004/diff/100001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.h:12: namespace wifi_credentials_helper { On 2015/01/31 10:26:40, pneubeck wrote: > On 2015/01/30 22:28:14, mukesh agrawal wrote: > > On 2015/01/30 14:38:45, pneubeck wrote: > > > some of this code is for testing only, please consider making that more > clear. > > > E.g. at each function (function name or at least a comment) or the > > > namespace&filename. > > > > Actually, _all_ of this code is for testing only :). This code exists only to > > support the tests in {single,two}_client_wifi_credentials_sync_test.cc. > > > > I believe that the fact that this is test support code is communicated by the > > location of the code ("sync/test/integration"), and the file name ("helper"). > > > > Given that all of these functions are for testing, I think per-function > > comments/names would be noise. I could add a file-level comment, if you'd > like. > > (I think it would be inconsistent with the other helpers in this directory, > but > > I don't feel too strongly about that.) > > sorry, I actually missed the /test/ part of the path. again, this is not so > common in chrome. Acknowledged. https://codereview.chromium.org/843483004/diff/120001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper.cc (right): https://codereview.chromium.org/843483004/diff/120001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper.cc:101: return wifi_credentials_helper::chromeos::SetUpChromeOs(); On 2015/01/31 10:26:40, pneubeck wrote: > Please remove the 'return'. Done. https://codereview.chromium.org/843483004/diff/120001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper.h (right): https://codereview.chromium.org/843483004/diff/120001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper.h:10: #include "base/compiler_specific.h" No longer needed, after removal of WARN_UNUSED_RESULT. https://codereview.chromium.org/843483004/diff/140001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc (right): https://codereview.chromium.org/843483004/diff/140001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/two_client_wifi_credentials_sync_test.cc:15: Removed stale using. https://codereview.chromium.org/843483004/diff/140001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper.h (right): https://codereview.chromium.org/843483004/diff/140001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper.h:11: #include "components/wifi_sync/wifi_credential.h" Can't forward-declare any more, since we need to reference a nested typedef (WifiCredential::WifiCredentialSet). https://codereview.chromium.org/843483004/diff/140001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc (right): https://codereview.chromium.org/843483004/diff/140001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/wifi_credentials_helper_chromeos.cc:117: base::MessageLoop::current()->RunUntilIdle(); Needed since CreateConfiguration is asynchronous. Previously, we'd done this implicitly via AwaitCommitActivityCompletion and AwaitMutualSyncCycleCompletion.
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/843483004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/843483004/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0c9f1c75eb83c5aa231696e49f72d004e6e10026 Cr-Commit-Position: refs/heads/master@{#315070} |
