Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2843513002:
Added a test for crash in DeviceDisabledScreen (Closed)
|
Created:
3 years, 8 months ago by Ivan Šandrk Modified:
3 years, 8 months ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded a test for crash in DeviceDisabledScreen
Follow up CL to crrev.com/2815893002
TEST=Manual. This test was crashing without the fix in cr/2815893002, now it's working fine.
BUG=709518
Review-Url: https://codereview.chromium.org/2843513002
Cr-Commit-Position: refs/heads/master@{#466719}
Committed: https://chromium.googlesource.com/chromium/src/+/29c3639d106664d10b7b42aa4765701dec803b5e
Patch Set 1 #
Total comments: 10
Patch Set 2 : Nits #Messages
Total messages: 24 (16 generated)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Description was changed from ========== Added a test for crash in DeviceDisabledScreen BUG=crbug.com/709518 ========== to ========== Added a test for crash in DeviceDisabledScreen BUG=709518 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + achuith@chromium.org, emaxx@chromium.org
Hi Achuith, Max, ptal!
Description was changed from ========== Added a test for crash in DeviceDisabledScreen BUG=709518 ========== to ========== Added a test for crash in DeviceDisabledScreen Follow up CL to crrev.com/2815893002 BUG=709518 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Added a test for crash in DeviceDisabledScreen Follow up CL to crrev.com/2815893002 BUG=709518 ========== to ========== Added a test for crash in DeviceDisabledScreen Follow up CL to crrev.com/2815893002 TEST=Manual. This test was crashing without the fix in cr/2815893002, now it's working fine. BUG=709518 ==========
https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/device_disabling_browsertest.cc (right): https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:51: return wizard_controller->current_screen() == nit: This will crash if wizard_controller is null, as EXPECT_TRUE doesn't return from the function. So something like adding "wizard_controller &&" into this condition will make it a little bit more robust. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:82: policy::DevicePolicyCrosTestHelper test_helper_; nit: Please make as private all members that aren't used in the inherited class. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:152: // Mark the device as disabled and wait until cros settings update. nit: I'd rather remove these comments here and in the new test - these comments seem to duplicate what is already said in the function names. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:230: class PresetPolicyDeviceDisablingTest : public DeviceDisablingTest { nit: Please add a short comment about what is specific with this class. E.g.: // Sets the device disabled policy before the browser is started. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:250: DisableDuringNormalOperation) { nit: Please find a more relevant name for the test (words "DuringNormalOperation" don't seem to correspond to the fact that the browser is started with the preset policy here).
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... File chrome/browser/chromeos/system/device_disabling_browsertest.cc (right): https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:51: return wizard_controller->current_screen() == On 2017/04/24 16:49:34, emaxx wrote: > nit: This will crash if wizard_controller is null, as EXPECT_TRUE doesn't return > from the function. So something like adding "wizard_controller &&" into this > condition will make it a little bit more robust. Done. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:82: policy::DevicePolicyCrosTestHelper test_helper_; On 2017/04/24 16:49:34, emaxx wrote: > nit: Please make as private all members that aren't used in the inherited class. Done. network_state_change_wait_run_loop_ is used in IN_PROC_BROWSER_TEST_F(DeviceDisablingTest, DisableWithEphemeralUsers) -> gives an error if I make it private https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:152: // Mark the device as disabled and wait until cros settings update. On 2017/04/24 16:49:34, emaxx wrote: > nit: I'd rather remove these comments here and in the new test - these comments > seem to duplicate what is already said in the function names. Done. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:230: class PresetPolicyDeviceDisablingTest : public DeviceDisablingTest { On 2017/04/24 16:49:34, emaxx wrote: > nit: Please add a short comment about what is specific with this class. E.g.: > // Sets the device disabled policy before the browser is started. Done. https://codereview.chromium.org/2843513002/diff/1/chrome/browser/chromeos/sys... chrome/browser/chromeos/system/device_disabling_browsertest.cc:250: DisableDuringNormalOperation) { On 2017/04/24 16:49:34, emaxx wrote: > nit: Please find a more relevant name for the test (words > "DuringNormalOperation" don't seem to correspond to the fact that the browser is > started with the preset policy here). How about DisableBeforeStartup?
LGTM. Thanks for working on this regression test!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
isandrk@chromium.org changed reviewers: + xiyuan@chromium.org - achuith@chromium.org
xiyuan, ptal! -achuith (ooo)
lgtm
The CQ bit was checked by isandrk@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493062398495800,
"parent_rev": "6076ea0e5caca2693cf84db598b1f33e938720d4", "commit_rev":
"29c3639d106664d10b7b42aa4765701dec803b5e"}
Message was sent while issue was closed.
Description was changed from ========== Added a test for crash in DeviceDisabledScreen Follow up CL to crrev.com/2815893002 TEST=Manual. This test was crashing without the fix in cr/2815893002, now it's working fine. BUG=709518 ========== to ========== Added a test for crash in DeviceDisabledScreen Follow up CL to crrev.com/2815893002 TEST=Manual. This test was crashing without the fix in cr/2815893002, now it's working fine. BUG=709518 Review-Url: https://codereview.chromium.org/2843513002 Cr-Commit-Position: refs/heads/master@{#466719} Committed: https://chromium.googlesource.com/chromium/src/+/29c3639d106664d10b7b42aa4765... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/29c3639d106664d10b7b42aa4765... |
