Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
216883004:
Browsertests for improved reset options: powerwash and rollback. (Closed)
|
Created:
6 years, 9 months ago by merkulova Modified:
6 years, 8 months ago CC:
chromium-reviews, arv+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. |
DescriptionBrowsertests for improved reset options: powerwash and rollback.
BUG=355073
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261415
Patch Set 1 #Patch Set 2 : Unused js API removed. #
Total comments: 12
Patch Set 3 : Required fixes added. #
Total comments: 12
Patch Set 4 : FakeUpdateEngine style fixes. #
Total comments: 1
Patch Set 5 : Const issue. #
Messages
Total messages: 33 (0 generated)
Initial version, so no other reviewers yet.
https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/reset_browsertest.cc (right): https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:28: const char kTestUser2[] = "test-user2@gmail.com"; Do you really need two users? I thought, not. https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:30: } // anonymous namespace nit: delete anonymous. https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:41: content::WebContents* GetWebContents() { Looks like you do exactly what LoginManagerTest::InitializeWebContents(). Try to use LoginManagerTest::web_contents(). https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:70: void SetUp_() { s/SetUp_/RegisterUsers/ https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:77: ASSERT_TRUE(content::ExecuteScript( Could you please create a shortcut and use it instead of content::ExecuteScript: bool ExecuteScript(const std::string& script) { return content::ExecuteScript(...); } https://codereview.chromium.org/216883004/diff/20001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.h (right): https://codereview.chromium.org/216883004/diff/20001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:63: int rollback_call_count() { Both methods can be folded to a single line.
https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/reset_browsertest.cc (right): https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:28: const char kTestUser2[] = "test-user2@gmail.com"; On 2014/03/31 13:57:57, ygorshenin1 wrote: > Do you really need two users? I thought, not. Done. https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:30: } // anonymous namespace On 2014/03/31 13:57:57, ygorshenin1 wrote: > nit: delete anonymous. Done. https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:41: content::WebContents* GetWebContents() { On 2014/03/31 13:57:57, ygorshenin1 wrote: > Looks like you do exactly what LoginManagerTest::InitializeWebContents(). Try to > use LoginManagerTest::web_contents(). Done. https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:70: void SetUp_() { On 2014/03/31 13:57:57, ygorshenin1 wrote: > s/SetUp_/RegisterUsers/ Done. https://codereview.chromium.org/216883004/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/login/reset_browsertest.cc:77: ASSERT_TRUE(content::ExecuteScript( On 2014/03/31 13:57:57, ygorshenin1 wrote: > Could you please create a shortcut and use it instead of content::ExecuteScript: > bool ExecuteScript(const std::string& script) { > return content::ExecuteScript(...); > } Done. https://codereview.chromium.org/216883004/diff/20001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.h (right): https://codereview.chromium.org/216883004/diff/20001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:63: int rollback_call_count() { On 2014/03/31 13:57:57, ygorshenin1 wrote: > Both methods can be folded to a single line. Done.
lgtm
+oshima@ for /chromeos/dbus/* files
https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.cc (right): https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.cc:11: reboot_after_update_call_count_(0), initialize can_rollback_stub_result_ https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.h (right): https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:55: void set_can_callback_check_result(bool result); you may inline this. https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:58: int reboot_after_update_call_count() {return reboot_after_update_call_count_;} const space betten { and return https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:61: int rollback_call_count() { const https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:66: int can_rollback_call_count() {return can_rollback_call_count_;} const https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:72: bool can_rollback_stub_result; private member variable should end with "_"
https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.cc (right): https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.cc:11: reboot_after_update_call_count_(0), On 2014/03/31 20:41:54, oshima wrote: > initialize can_rollback_stub_result_ Done. https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.h (right): https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:55: void set_can_callback_check_result(bool result); On 2014/03/31 20:41:54, oshima wrote: > you may inline this. Done. https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:58: int reboot_after_update_call_count() {return reboot_after_update_call_count_;} On 2014/03/31 20:41:54, oshima wrote: > const > space betten { and return Done. https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:61: int rollback_call_count() { On 2014/03/31 20:41:54, oshima wrote: > const Done. https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:66: int can_rollback_call_count() {return can_rollback_call_count_;} On 2014/03/31 20:41:54, oshima wrote: > const Done. https://codereview.chromium.org/216883004/diff/40001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:72: bool can_rollback_stub_result; On 2014/03/31 20:41:54, oshima wrote: > private member variable should end with "_" Done.
https://codereview.chromium.org/216883004/diff/60001/chromeos/dbus/fake_updat... File chromeos/dbus/fake_update_engine_client.h (right): https://codereview.chromium.org/216883004/diff/60001/chromeos/dbus/fake_updat... chromeos/dbus/fake_update_engine_client.h:60: const int reboot_after_update_call_count() { Sorry if I wasn't clear. I meant this should be const method. same for other accessors.
lgtm
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/216883004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_rel
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/216883004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/216883004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/216883004/80001
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 merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/216883004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by merkulova@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/merkulova@chromium.org/216883004/80001
Message was sent while issue was closed.
Change committed as 261415 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
