Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
115073003:
Test that Guest profile is Incognito (Closed)
|
Created:
7 years ago by Dmitry Polukhin Modified:
6 years, 11 months ago CC:
chromium-reviews, rginda+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTest that Guest profile is Incognito
Follow up test for https://codereview.chromium.org/101413011
BUG=329498
TEST=unit_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243678
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase + comments #
Total comments: 2
Messages
Total messages: 16 (0 generated)
This test tests that we always return Incognito profile in Guest mode. Unfortunately due to deep mocking profiles and other things in this test there is no way to test that profile was properly initialized with disabled extension etc. But I think it is OK for unittest for ProfileManager. davemoore@ for OWNER review chrome/browser/profiles/profile_manager_unittest.cc pastarmovj@ for OWNER review chrome/browser/chromeos/settings/device_settings_service.cc Change in this file requires because UserLoggedIn initiates OWNER check that result in pending operation.
Please see comments. https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager_unittest.cc:346: ProfileManager::AllowGetDefaultProfile(); This function doesn't exist anymore (landed a patch yesterday or today). https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager_unittest.cc:349: Profile* primary_profile = profile_manager->GetPrimaryUserProfile(); Nit: Since Get*UserProfile function is static - and no one uses them this way - I wonder if we don't want to call the real functions. Note that even if UnittestGuestProfileManager would override that function we wouldn't probably want to call that since what sense does it make to test the test framework in that case? So shouldn't "ProfileManager::GetPrimaryUserProfile();" do it? https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager_unittest.cc:352: Profile* active_profile = profile_manager->GetActiveUserProfile(); Same here.
https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager_unittest.cc:346: ProfileManager::AllowGetDefaultProfile(); On 2013/12/20 23:39:57, Mr4D wrote: > This function doesn't exist anymore (landed a patch yesterday or today). Excellent! This was artifact that has nothing with this test. https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager_unittest.cc:349: Profile* primary_profile = profile_manager->GetPrimaryUserProfile(); On 2013/12/20 23:39:57, Mr4D wrote: > Nit: Since Get*UserProfile function is static - and no one uses them this way - > I wonder if we don't want to call the real functions. Note that even if > UnittestGuestProfileManager would override that function we wouldn't probably > want to call that since what sense does it make to test the test framework in > that case? So shouldn't "ProfileManager::GetPrimaryUserProfile();" do it? Done. Because these functions are not virtual overloading in UnittestGuestProfileManager wouldn't change anything. https://codereview.chromium.org/115073003/diff/1/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager_unittest.cc:352: Profile* active_profile = profile_manager->GetActiveUserProfile(); On 2013/12/20 23:39:57, Mr4D wrote: > Same here. Done.
lgtm
https://codereview.chromium.org/115073003/diff/70001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/115073003/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:324: class ProfileManagerGuestTest : public ProfileManagerTest { Please add another instance of this test that will enable multi-profiles https://chromiumcodereview.appspot.com/15305011 Should enable both command line + finch, example https://codereview.chromium.org/25364002/diff/15001/chrome/browser/metrics/me...
Dave, please do OWNER review. https://codereview.chromium.org/115073003/diff/70001/chrome/browser/profiles/... File chrome/browser/profiles/profile_manager_unittest.cc (right): https://codereview.chromium.org/115073003/diff/70001/chrome/browser/profiles/... chrome/browser/profiles/profile_manager_unittest.cc:324: class ProfileManagerGuestTest : public ProfileManagerTest { On 2013/12/25 07:48:24, Nikita Kostylev OOO Dec 26-29 wrote: > Please add another instance of this test that will enable multi-profiles > https://chromiumcodereview.appspot.com/15305011 > > Should enable both command line + finch, example > https://codereview.chromium.org/25364002/diff/15001/chrome/browser/metrics/me... Guest profile is no allowed in multi-profile session so such test crashes with NOTREACHED in ProfileHelper::GetUserIdHashFromProfile. Moreover it is fundamental limitation for us now so there is no sense to add such test.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/115073003/70001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
+ rlp@ for OWNER review chrome/browser/profiles/profile_manager_unittest.cc PTAL
profiles LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/115073003/70001
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dpolukhin@chromium.org/115073003/70001
Message was sent while issue was closed.
Change committed as 243678 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
