Add unit tests to prevent regressions like crbug.com/807856 |
||||||
Issue descriptionI mean, the tests that checks the sanity of parameters passed to cryptohome methods from encryption_migration_screen. I haven't looked into the details yet, but recording the passed arguments FakeCryptohomeClient and checking them in looks doable to me. https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler_unittest.cc Any nicer suggestions are welcomed.
,
Feb 1 2018
+xiyuan@ and +hidehiko@ in case they have any opinions
,
Feb 2 2018
Putting this on my plate for now so that it won't be forgotten (but feel free to take over if anyone got interested.)
,
Feb 2 2018
Grabbing as adding a unit test is pretty easy. If we think adding other types of tests (integration/e2e etc) are worth it maybe we can split this bug out.
,
Feb 2 2018
,
Feb 6 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0127a3dcc32957dc54285c7925bc28144c6d659e commit 0127a3dcc32957dc54285c7925bc28144c6d659e Author: Maajid <maajid@chromium.org> Date: Tue Feb 06 03:39:59 2018 Add unit test checks for authorization request regression. Bug: 807876 Test: Confirmed locally that this would have caught the regression. Change-Id: I13880896420471a8e0d5325d2453306b11f37fe8 Reviewed-on: https://chromium-review.googlesource.com/897289 Reviewed-by: Kazuhiro Inaba <kinaba@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Commit-Queue: Maajid <maajid@chromium.org> Cr-Commit-Position: refs/heads/master@{#534614} [modify] https://crrev.com/0127a3dcc32957dc54285c7925bc28144c6d659e/chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler_unittest.cc [modify] https://crrev.com/0127a3dcc32957dc54285c7925bc28144c6d659e/chromeos/dbus/fake_cryptohome_client.cc [modify] https://crrev.com/0127a3dcc32957dc54285c7925bc28144c6d659e/chromeos/dbus/fake_cryptohome_client.h
,
Feb 6 2018
Marking this as fixed, let's file a new bug if we want to add an autotest or something heavier than a unit test. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by maajid@google.com
, Feb 1 2018