Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
243103007:
Tests for supervised users password sync: (Closed)
|
Created:
6 years, 8 months ago by Denis Kuznetsov (DE-MUC) Modified:
6 years, 7 months ago Reviewers:
Nikita (slow) CC:
chromium-reviews, pam+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. |
DescriptionTests for supervised users password sync:
First two test cases:
password change detected when SU is signed in, only su is signed in after
password change is detected when manager is signed in.
R=nkostylev@chromium.org
BUG=354607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266892
Patch Set 1 #Patch Set 2 : Use sprint instead of concatenation, +1 test #
Total comments: 40
Patch Set 3 : Fix nits #Patch Set 4 : add comments to checks #Patch Set 5 : Mark generated sync changes as add/update #Patch Set 6 : Fix erroneous commit #
Messages
Total messages: 32 (0 generated)
Overall looks good, as discussed: * Cleanup (uncomment) commented code * Change append to using string printf * When possible leave empty string before comment as it improves code readability.
Ready for review?
On 2014/04/22 12:04:55, Nikita Kostylev wrote: > Ready for review? yes, please take a look
https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc (right): https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:160: // Make sure user is already in list. nit: Insert empty line before comment. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:162: // We wait for token now. Press cancel button at this point. nit: Insert empty line. >> We wait for token now. Should you also add verification for that or not? https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:166: IN_PROC_BROWSER_TEST_( What this macros does? https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/managed/managed_user_password_browsertest.cc (right): https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_password_browsertest.cc:75: IN_PROC_BROWSER_TEST_F(SupervisedUserPasswordTest, This file doesn't have a single comment. Please at least add short comments for each test case describing what it does. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/managed/managed_user_test_base.cc (right): https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:50: nit: Drop empty line (or insert empty line after constant. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:51: std::string current_page = "$('managed-user-creation').currentPage_"; const char kCurrentPage[] = "..."; https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:69: CHECK(HasChanges()); Why not ASSERT or EXPECT? We should be using them instead CHECK/DCHECK in tests. Please change all of these instances in this CL. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:195: scoped_utility_.reset(NULL); Is this needed? Can you just rely on scoped_ptr destructor? https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:287: // Log in as manager. nit: insert empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:290: // OAuth token is valid. nit: insert empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:344: if (check_homedir_calls) { nit: drop {} https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:347: // Log in as supervised user, make sure that everything works. nit: insert empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:349: // Created supervised user have to be first in a list. nit: insert empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:353: if (check_homedir_calls) { nit: drop {} https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:364: // Created supervised user have to be first in a list. nit: insert empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:378: nit: drop empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:380: // Created supervised user have to be first in a list. nit: insert empty line. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/managed/managed_user_test_base.h (right): https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.h:11: #include "base/strings/utf_string_conversions.h" Please cleanup includes. It looks like not all of them are really in use in this header. https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.h:44: const char kStubEthernetServicePath[] = "eth0"; nit: Move to .cc
https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... File chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc (right): https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:160: // Make sure user is already in list. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: Insert empty line before comment. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:162: // We wait for token now. Press cancel button at this point. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: Insert empty line. > > >> We wait for token now. > > Should you also add verification for that or not? Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:166: IN_PROC_BROWSER_TEST_( On 2014/04/23 09:50:17, Nikita Kostylev wrote: > What this macros does? this is expansion of IN_PROC_BROWSER_TEST_F. It let us have fixture for SupervisedUserTransactionCleanupTest with SupervisedUserTransactionCleanupTest2 superclass. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_creation_browsertest.cc:166: IN_PROC_BROWSER_TEST_( On 2014/04/23 09:50:17, Nikita Kostylev wrote: > What this macros does? It is the expansion of IN_PROC_BROWSER_TEST_F. This is used to add a fixture for SupervisedUserTransactionCleanupTest with SupervisedUserTransactionCleanupTest2 as a base class. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... File chrome/browser/chromeos/login/managed/managed_user_password_browsertest.cc (right): https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_password_browsertest.cc:75: IN_PROC_BROWSER_TEST_F(SupervisedUserPasswordTest, On 2014/04/23 09:50:17, Nikita Kostylev wrote: > This file doesn't have a single comment. > Please at least add short comments for each test case describing what it does. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... File chrome/browser/chromeos/login/managed/managed_user_test_base.cc (right): https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:50: On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: Drop empty line (or insert empty line after constant. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:51: std::string current_page = "$('managed-user-creation').currentPage_"; On 2014/04/23 09:50:17, Nikita Kostylev wrote: > const char kCurrentPage[] = "..."; Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:69: CHECK(HasChanges()); On 2014/04/23 09:50:17, Nikita Kostylev wrote: > Why not ASSERT or EXPECT? We should be using them instead CHECK/DCHECK in > tests. > > Please change all of these instances in this CL. These are sanity checks, not things we try to validate in tests. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:195: scoped_utility_.reset(NULL); On 2014/04/23 09:50:17, Nikita Kostylev wrote: > Is this needed? Can you just rely on scoped_ptr destructor? Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:287: // Log in as manager. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: insert empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:290: // OAuth token is valid. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: insert empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:344: if (check_homedir_calls) { On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: drop {} Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:347: // Log in as supervised user, make sure that everything works. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: insert empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:349: // Created supervised user have to be first in a list. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: insert empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:353: if (check_homedir_calls) { On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: drop {} Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:364: // Created supervised user have to be first in a list. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: insert empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:378: On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: drop empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:380: // Created supervised user have to be first in a list. On 2014/04/23 09:50:17, Nikita Kostylev wrote: > nit: insert empty line. Done. https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... File chrome/browser/chromeos/login/managed/managed_user_test_base.h (right): https://chromiumcodereview.appspot.com/243103007/diff/20001/chrome/browser/ch... chrome/browser/chromeos/login/managed/managed_user_test_base.h:11: #include "base/strings/utf_string_conversions.h" On 2014/04/23 09:50:17, Nikita Kostylev wrote: > Please cleanup includes. It looks like not all of them are really in use in this > header. Done.
https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/managed/managed_user_test_base.cc (right): https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:69: CHECK(HasChanges()); On 2014/04/24 17:44:09, Denis Kuznetsov wrote: > On 2014/04/23 09:50:17, Nikita Kostylev wrote: > > Why not ASSERT or EXPECT? We should be using them instead CHECK/DCHECK in > > tests. > > > > Please change all of these instances in this CL. > > These are sanity checks, not things we try to validate in tests. Still this is just a test and should not ever crash. Just put an ASSERT and test will fail properly. Besides now you're just crashing in some place in the test of for someone (sheriff) looking in the logs this failure would be seem as something wrong with the test i.e. you're no specifying any info message. Summary: use ASSERT with a log message.
Is this approach fine? https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/managed/managed_user_test_base.cc (right): https://codereview.chromium.org/243103007/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/managed/managed_user_test_base.cc:69: CHECK(HasChanges()); On 2014/04/25 06:43:59, Nikita Kostylev wrote: > On 2014/04/24 17:44:09, Denis Kuznetsov wrote: > > On 2014/04/23 09:50:17, Nikita Kostylev wrote: > > > Why not ASSERT or EXPECT? We should be using them instead CHECK/DCHECK in > > > tests. > > > > > > Please change all of these instances in this CL. > > > > These are sanity checks, not things we try to validate in tests. > > Still this is just a test and should not ever crash. > Just put an ASSERT and test will fail properly. > > Besides now you're just crashing in some place in the test of for someone > (sheriff) looking in the logs this failure would be seem as something wrong with > the test i.e. you're no specifying any info message. > > Summary: use ASSERT with a log message. ASSERT can be used only inside tests (that macro expects that this is a method of test class). Still, if this expectation does not meet, then we will crash at the next line, where we access front() of empty vector. So this should be CHECK, but I will add extra message here.
lgtm
The CQ bit was checked by nkostylev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/243103007/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by antrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/243103007/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by antrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/243103007/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by antrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/243103007/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by nkostylev@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/243103007/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium
The CQ bit was checked by antrim@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/antrim@chromium.org/243103007/90001
Message was sent while issue was closed.
Change committed as 266892 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
