Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1306163003:
Dynamically create Gaia accounts when running E2E sync integration tests. (Closed)
|
Created:
5 years, 4 months ago by shadi Modified:
5 years, 3 months ago Reviewers:
pval...(no longer on Chromium) CC:
chromium-reviews, tim+watch_chromium.org, pvalenzuela+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDynamically create Gaia accounts when running E2E sync integration tests.
Tests would request a new Gaia account only when running
against external servers AND when no --sync-user-for-test
flag is present.
BUG=525165
Committed: https://crrev.com/13088190f26447b081913e80da9ebe6e341650c2
Cr-Commit-Position: refs/heads/master@{#347856}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : fix comments #
Total comments: 6
Patch Set 3 : fix comments #Patch Set 4 : default create_gaia_account_at_runtime_ to false #
Messages
Total messages: 20 (9 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
shadi@google.com changed reviewers: + pvalenzuela@chromium.org
shadi@google.com changed reviewers: + shadi@google.com
PTAL
please wrap commit message to <=72 chars https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:229: if (cl->HasSwitch(switches::kSyncPasswordForTest)) can we simplify the logic in this method to only set password_ once? https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:233: }else { space after } https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:304: DVLOG(1) << "Create Gaia account request return code = " << i believe the trailing << here should be on the next line (aligned with the << above) https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:267: // Flag to indicate the need to create Gaia user account at runtime. This can s/Flag to indicate/Indicates/ IMO flag has the strong connotation of command-line flag https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:293: // Create a Gaia account at runtime by accessing TestDataSetupServer server it's probably best to remove the reference to TestDataSetupServer since it is not in Chromium https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:298: bool CreateGaiaAccount(const std::string& user_name, should user_name be username instead?
pvalenzuela@chromium.org changed reviewers: - shadi@google.com
PTAL. I ran a test locally and it passed too. https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:229: if (cl->HasSwitch(switches::kSyncPasswordForTest)) On 2015/08/26 19:26:33, pvalenzuela wrote: > can we simplify the logic in this method to only set password_ once? PTAL at new logic. https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:233: }else { On 2015/08/26 19:26:33, pvalenzuela wrote: > space after } Done. https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:304: DVLOG(1) << "Create Gaia account request return code = " << On 2015/08/26 19:26:33, pvalenzuela wrote: > i believe the trailing << here should be on the next line (aligned with the << > above) Done. https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:267: // Flag to indicate the need to create Gaia user account at runtime. This can On 2015/08/26 19:26:33, pvalenzuela wrote: > s/Flag to indicate/Indicates/ > > IMO flag has the strong connotation of command-line flag Done. https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:293: // Create a Gaia account at runtime by accessing TestDataSetupServer server On 2015/08/26 19:26:34, pvalenzuela wrote: > it's probably best to remove the reference to TestDataSetupServer since it is > not in Chromium Done. https://codereview.chromium.org/1306163003/diff/40001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:298: bool CreateGaiaAccount(const std::string& user_name, On 2015/08/26 19:26:34, pvalenzuela wrote: > should user_name be username instead? Done.
https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:225: // servers then we assume the need to automatically create a Gaia account. I think you can remove everything up to "We assume the need to..." the boolean logic is inferred from the code and I think it reduces the clutter here to have a shorter comment. https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:234: if (cl->HasSwitch(switches::kSyncPasswordForTest)) { Consider using the ternary (?) operator here? https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:290: // which use local authentication servers. "external servers which use local authentication servers" seems a bit confusing (external vs. local). How about: "external servers which support this functionality" or similar?
Thanks for the review. https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.cc (right): https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:225: // servers then we assume the need to automatically create a Gaia account. On 2015/09/04 16:48:32, pvalenzuela wrote: > I think you can remove everything up to "We assume the need to..." the boolean > logic is inferred from the code and I think it reduces the clutter here to have > a shorter comment. Done. https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.cc:234: if (cl->HasSwitch(switches::kSyncPasswordForTest)) { On 2015/09/04 16:48:32, pvalenzuela wrote: > Consider using the ternary (?) operator here? Done. https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... File chrome/browser/sync/test/integration/sync_test.h (right): https://codereview.chromium.org/1306163003/diff/60001/chrome/browser/sync/tes... chrome/browser/sync/test/integration/sync_test.h:290: // which use local authentication servers. On 2015/09/04 16:48:32, pvalenzuela wrote: > "external servers which use local authentication servers" seems a bit confusing > (external vs. local). > > How about: "external servers which support this functionality" or similar? Done.
lgtm
The CQ bit was checked by shadi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306163003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306163003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by shadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pvalenzuela@chromium.org Link to the patchset: https://codereview.chromium.org/1306163003/#ps100001 (title: "default create_gaia_account_at_runtime_ to false")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1306163003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1306163003/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/13088190f26447b081913e80da9ebe6e341650c2 Cr-Commit-Position: refs/heads/master@{#347856} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
