Password manager code should be better modularised to make testing easier |
|||||
Issue descriptionThis is a high-level bug, feel free to link more concrete ones into it. The structure of the classes in the password_manager component seems a bit arbitrary, e.g., the PasswordManager class is a mix of methods for different functions (organising PasswordFormManagers, understanding a successful sign-in, dispatching autofill requests from the form managers, handling HTTP auth, etc; the PasswordFormManager class has the same issue, and there are likely others. This makes unit-testing in particular harder than necessary. It is difficult to isolate the tested code from its dependencies, leading to, e.g., integration tests masked as unit tests in the (downstream) iOS part of password manager, or mocking parts of the tested class (bug 474577). It is a low-priority refactoring so far, because it is not blocking anything. But fixing it would help us improve our test coverage and decrease flakiness by moving more testing from integration to unit tests.
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86b472697e7f5871d75963d32266332b1a95a242 commit 86b472697e7f5871d75963d32266332b1a95a242 Author: vabr <vabr@chromium.org> Date: Wed Mar 30 09:49:02 2016 Add a unittest for PasswordController While PasswordController already has a unittest downstream, that one is rather an integration test (the heaviness of which is the reason it is still downstream, stuck on some downstream-only testing framework). When a test was needed for https://codereview.chromium.org/1806333005/, it turned out a simple unittest would be better. This CL adds that test and places it upstream. (Ultimately we should convert the downstream tests to a lighter version and add them here as well, but not in this CL.) BUG= 595717 , 598672 Review URL: https://codereview.chromium.org/1808853005 Cr-Commit-Position: refs/heads/master@{#383941} [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/BUILD.gn [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.h [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.mm [add] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller_unittest.mm [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/ios_chrome_tests.gyp
,
Mar 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/86b472697e7f5871d75963d32266332b1a95a242 commit 86b472697e7f5871d75963d32266332b1a95a242 Author: vabr <vabr@chromium.org> Date: Wed Mar 30 09:49:02 2016 Add a unittest for PasswordController While PasswordController already has a unittest downstream, that one is rather an integration test (the heaviness of which is the reason it is still downstream, stuck on some downstream-only testing framework). When a test was needed for https://codereview.chromium.org/1806333005/, it turned out a simple unittest would be better. This CL adds that test and places it upstream. (Ultimately we should convert the downstream tests to a lighter version and add them here as well, but not in this CL.) BUG= 595717 , 598672 Review URL: https://codereview.chromium.org/1808853005 Cr-Commit-Position: refs/heads/master@{#383941} [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/BUILD.gn [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.h [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller.mm [add] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/browser/passwords/password_controller_unittest.mm [modify] https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242/ios/chrome/ios_chrome_tests.gyp
,
Mar 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aaeb70a8b015d3b88fd34731efa8ad499e760c05 commit aaeb70a8b015d3b88fd34731efa8ad499e760c05 Author: vabr <vabr@chromium.org> Date: Thu Mar 31 08:50:06 2016 Revert of Add a unittest for PasswordController (patchset #5 id:80001 of https://codereview.chromium.org/1808853005/ ) Reason for revert: Reporter breakage, vabr@ will investigate. Original issue's description: > Add a unittest for PasswordController > > While PasswordController already has a unittest downstream, that one is rather > an integration test (the heaviness of which is the reason it is still > downstream, stuck on some downstream-only testing framework). > > When a test was needed for https://codereview.chromium.org/1806333005/, it > turned out a simple unittest would be better. This CL adds that test and > places it upstream. (Ultimately we should convert the downstream tests to a > lighter version and add them here as well, but not in this CL.) > > BUG= 595717 , 598672 > > Committed: https://crrev.com/86b472697e7f5871d75963d32266332b1a95a242 > Cr-Commit-Position: refs/heads/master@{#383941} TBR=droger@chromium.org,dvadym@chromium.org,eugenebut@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 595717 , 598672 Review URL: https://codereview.chromium.org/1842313002 Cr-Commit-Position: refs/heads/master@{#384225} [modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/BUILD.gn [modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/browser/passwords/password_controller.h [modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/browser/passwords/password_controller.mm [delete] https://crrev.com/04143c634f21b22da89a698be7744acdd22306ae/ios/chrome/browser/passwords/password_controller_unittest.mm [modify] https://crrev.com/aaeb70a8b015d3b88fd34731efa8ad499e760c05/ios/chrome/ios_chrome_tests.gyp
,
Mar 31 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/40a58addf1b656beaac8b7a2f50cca718a0556e0 commit 40a58addf1b656beaac8b7a2f50cca718a0556e0 Author: vabr <vabr@chromium.org> Date: Thu Mar 31 15:54:06 2016 Reland of Add a unittest for PasswordController Original CL: https://codereview.chromium.org/1808853005/ CL with revert: https://codereview.chromium.org/1842313002/ Patch set 1 here == what landed in the original CL Subsequent patches constitute the fix of the breakage. TBR-ing droger@ for the GYP and GN changes, which he already approved in the original CL, and which are not different here. Original description: ------------- While PasswordController already has a unittest downstream, that one is rather an integration test (the heaviness of which is the reason it is still downstream, stuck on some downstream-only testing framework). When a test was needed for https://codereview.chromium.org/1806333005/, it turned out a simple unittest would be better. This CL adds that test and places it upstream. (Ultimately we should convert the downstream tests to a lighter version and add them here as well, but not in this CL.) ------------- BUG= 595717 , 598672, 599231 TBR=droger@chromium.org Review URL: https://codereview.chromium.org/1848433004 Cr-Commit-Position: refs/heads/master@{#384291} [modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/BUILD.gn [modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/browser/passwords/password_controller.h [modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/browser/passwords/password_controller.mm [add] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/browser/passwords/password_controller_unittest.mm [modify] https://crrev.com/40a58addf1b656beaac8b7a2f50cca718a0556e0/ios/chrome/ios_chrome_tests.gyp
,
Jun 6 2016
,
Jun 6 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 23 2017
,
Jan 26 2018
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by vabr@chromium.org
, Mar 29 2016