|
|
DescriptionFix Print Preview failure state and add test
Prevent print preview error state from being unrecoverable by resetting
the state when the user selects a new printer. Add test to check that
the state resets and to check that the response to the INVALID_SETTINGS
event is correct.
BUG=708834
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2849733002
Cr-Commit-Position: refs/heads/master@{#468379}
Committed: https://chromium.googlesource.com/chromium/src/+/a7b31e1ff636ee016885a18953745783c9825616
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix test #
Total comments: 8
Patch Set 3 : Fix nits #
Messages
Total messages: 23 (16 generated)
Description was changed from ========== Fix Print Preview failure state and add test Prevent print preview error state from being unrecoverable by resetting the state when the user selects a new printer. Add test to check that the state resets and to check that the response to the INVALID_SETTINGS event is correct. BUG=708834 ========== to ========== Fix Print Preview failure state and add test Prevent print preview error state from being unrecoverable by resetting the state when the user selects a new printer. Add test to check that the state resets and to check that the response to the INVALID_SETTINGS event is correct. BUG=708834 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== Fix Print Preview failure state and add test Prevent print preview error state from being unrecoverable by resetting the state when the user selects a new printer. Add test to check that the state resets and to check that the response to the INVALID_SETTINGS event is correct. BUG=708834 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix Print Preview failure state and add test Prevent print preview error state from being unrecoverable by resetting the state when the user selects a new printer. Add test to check that the state resets and to check that the response to the INVALID_SETTINGS event is correct. BUG=708834 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2849733002/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2849733002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:157: invalidSettings: function() { Nit: |invalidSettings| sounds more like a data variable name than a function name. How about renaming to "simulateInvalidSettings", or "dispathInvalidSettings". https://codereview.chromium.org/2849733002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1378: expectEquals('ready', printPreview.uiState_); Is there a way to avoid referring to a private variable from this test? Tests that assert on private variables tend to test a specific implementation instead of the observed behavior of the code they are testing. Maybe assert that something that is observable by the user on the DOM (some button being enabled, some message being shown etc).
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2849733002/diff/1/chrome/test/data/webui/prin... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2849733002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:157: invalidSettings: function() { On 2017/04/28 17:13:30, dpapad wrote: > Nit: |invalidSettings| sounds more like a data variable name than a function > name. How about renaming to "simulateInvalidSettings", or > "dispathInvalidSettings". Done. https://codereview.chromium.org/2849733002/diff/1/chrome/test/data/webui/prin... chrome/test/data/webui/print_preview.js:1378: expectEquals('ready', printPreview.uiState_); On 2017/04/28 17:13:30, dpapad wrote: > Is there a way to avoid referring to a private variable from this test? Tests > that assert on private variables tend to test a specific implementation instead > of the observed behavior of the code they are testing. > > Maybe assert that something that is observable by the user on the DOM (some > button being enabled, some message being shown etc). Done.
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.
LGTM with nits. https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:182: **/ Extra asterisk. */ https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1416: expectEquals(-1, customMessageEl.textContent.search(expectedMessageStart)); Nit: Use includes() instead, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1434: var barDestination; var barDestination = printPreview.destinationStore_.destinations().find(function(d) { return d.id == 'BarDevice'; }); https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1445: // loaded Per styleguide, full sentence comments should finish with a period.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/print_preview.js (right): https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:182: **/ On 2017/04/29 01:20:20, dpapad wrote: > Extra asterisk. > */ Done. https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1416: expectEquals(-1, customMessageEl.textContent.search(expectedMessageStart)); On 2017/04/29 01:20:20, dpapad wrote: > Nit: Use includes() instead, > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje.... Done. https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1434: var barDestination; On 2017/04/29 01:20:20, dpapad wrote: > var barDestination = > printPreview.destinationStore_.destinations().find(function(d) { > return d.id == 'BarDevice'; > }); Done. https://codereview.chromium.org/2849733002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/print_preview.js:1445: // loaded On 2017/04/29 01:20:20, dpapad wrote: > Per styleguide, full sentence comments should finish with a period. Done.
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rbpotter@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/2849733002/#ps60001 (title: "Fix nits")
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": 1493661248547140, "parent_rev": "10b769e9ee208a2d2f08dec880126bf4731f5e1e", "commit_rev": "a7b31e1ff636ee016885a18953745783c9825616"}
Message was sent while issue was closed.
Description was changed from ========== Fix Print Preview failure state and add test Prevent print preview error state from being unrecoverable by resetting the state when the user selects a new printer. Add test to check that the state resets and to check that the response to the INVALID_SETTINGS event is correct. BUG=708834 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix Print Preview failure state and add test Prevent print preview error state from being unrecoverable by resetting the state when the user selects a new printer. Add test to check that the state resets and to check that the response to the INVALID_SETTINGS event is correct. BUG=708834 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2849733002 Cr-Commit-Position: refs/heads/master@{#468379} Committed: https://chromium.googlesource.com/chromium/src/+/a7b31e1ff636ee016885a1895374... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a7b31e1ff636ee016885a1895374... |