Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2959023003:
MD-Settings: Create tests for edit options in Credit Cards. (Closed)
|
Created:
3 years, 5 months ago by hcarmona Modified:
3 years, 5 months ago Reviewers:
dpapad CC:
arv+watch_chromium.org, chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD-Settings: Create tests for edit options in Credit Cards.
R=dpapad@chromium.org
BUG=735832
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2959023003
Cr-Commit-Position: refs/heads/master@{#483052}
Committed: https://chromium.googlesource.com/chromium/src/+/7b0ea532f1925610e3034b8797c8fef792070877
Patch Set 1 : nit #
Total comments: 8
Patch Set 2 : feedback #
Total comments: 2
Patch Set 3 : semicolon #
Messages
Total messages: 21 (14 generated)
Description was changed from ========== MD-Settings: Create tests for edit options in Credit Cards. R=dpapad@chromium.org BUG=735832 ========== to ========== MD-Settings: Create tests for edit options in Credit Cards. R=dpapad@chromium.org BUG=735832 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:438: var creditCards = [FakeDataMaker.addressEntry()]; Shouldn't this be calling creditCardEntry()? Also, is there a reason to put this in an array here? Why not just wrap it in an array at line 444, before passing it to createAutofillSection_? Same for next test. https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:445: assertTrue(!!section); Is this assertion necessary? Can it ever fail? https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:448: assertEquals(2, creditCardList.children.length); // Template + Row. How about the following? assertEquals(1, creditCardList.querySelectorAll('.list-item').length); This avoids having to account for the <template> child (which is an implementation detail of dom-repeat). https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:459: var ccMenu = section.$.creditCardSharedMenu; Avoid non established abbreviations (per styleguide). Maybe just rename to |menu| here since there is no ambiguity about which menu is referenced.
The CQ bit was checked by hcarmona@chromium.org to run a CQ dry run
https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:438: var creditCards = [FakeDataMaker.addressEntry()]; On 2017/06/27 22:00:36, dpapad wrote: > Shouldn't this be calling creditCardEntry()? Yes, good catch. > Also, is there a reason to put this in an array here? Why not just wrap it in an > array at line 444, before passing it to createAutofillSection_? Same for next > test. Sounds good, updated. https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:445: assertTrue(!!section); On 2017/06/27 22:00:36, dpapad wrote: > Is this assertion necessary? Can it ever fail? Removed here and elsewhere. https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:448: assertEquals(2, creditCardList.children.length); // Template + Row. On 2017/06/27 22:00:36, dpapad wrote: > How about the following? > assertEquals(1, creditCardList.querySelectorAll('.list-item').length); > > This avoids having to account for the <template> child (which is an > implementation detail of dom-repeat). Done. https://codereview.chromium.org/2959023003/diff/20001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:459: var ccMenu = section.$.creditCardSharedMenu; On 2017/06/27 22:00:36, dpapad wrote: > Avoid non established abbreviations (per styleguide). Maybe just rename to > |menu| here since there is no ambiguity about which menu is referenced. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2959023003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2959023003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:449: var menuButton = row.querySelector('#creditCardMenu') Semicolon missing
The CQ bit was checked by hcarmona@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
committing this https://codereview.chromium.org/2959023003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/settings_autofill_section_browsertest.js (right): https://codereview.chromium.org/2959023003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/settings_autofill_section_browsertest.js:449: var menuButton = row.querySelector('#creditCardMenu') On 2017/06/27 23:28:20, dpapad wrote: > Semicolon missing Fixed here and elsewhere.
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2959023003/#ps60001 (title: "semicolon")
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": 60001, "attempt_start_ts": 1498671798560010,
"parent_rev": "23768d32b212f3dbb00965c87a1b892d3ae6f634", "commit_rev":
"7b0ea532f1925610e3034b8797c8fef792070877"}
Message was sent while issue was closed.
Description was changed from ========== MD-Settings: Create tests for edit options in Credit Cards. R=dpapad@chromium.org BUG=735832 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD-Settings: Create tests for edit options in Credit Cards. R=dpapad@chromium.org BUG=735832 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2959023003 Cr-Commit-Position: refs/heads/master@{#483052} Committed: https://chromium.googlesource.com/chromium/src/+/7b0ea532f1925610e3034b8797c8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7b0ea532f1925610e3034b8797c8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
