Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2602503002:
bluetooth: Fix flaky bluetooth internals browser test. (Closed)
Descriptionbluetooth: Fix flaky bluetooth internals browser test.
Changes Snackbar#dismiss to return Promise that resolves when snackbar is fully
dismissed.
Add check for snackbar that's already dismissed to fix test issue where Snackbar
is dismissed repeatedly.
BUG=676227
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2602503002
Cr-Commit-Position: refs/heads/master@{#441712}
Committed: https://chromium.googlesource.com/chromium/src/+/8f983db3a8ae9058070528ed2ec2034bedc6976e
Patch Set 1 #Patch Set 2 : Add more checks to Snackbar_QueueThreeDismissAll test #
Total comments: 2
Patch Set 3 : Add transition duration constant #
Total comments: 8
Patch Set 4 : Fix up tests, clean up formatting #
Total comments: 6
Patch Set 5 : Simplify snackbar and test #
Messages
Total messages: 34 (24 generated)
Description was changed from ========== bluetooth: Fix flaky bluetooth internals browser test. Changes Snackbar#dismiss to return Promise that resolves when snackbar is fully dismissed. Add check for snackbar that's already dismissed to fix test issue where Snackbar is dismissed repeatedly. BUG=676227 ========== to ========== bluetooth: Fix flaky bluetooth internals browser test. Changes Snackbar#dismiss to return Promise that resolves when snackbar is fully dismissed. Add check for snackbar that's already dismissed to fix test issue where Snackbar is dismissed repeatedly. BUG=676227 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by mbrunson@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 checked by mbrunson@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by mbrunson@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.
mbrunson@chromium.org changed reviewers: + dbeam@chromium.org, scheib@chromium.org
LGTM, but a Q: https://codereview.chromium.org/2602503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/snackbar.js (right): https://codereview.chromium.org/2602503002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:110: ensureTransitionEndEvent(this, 225); Wny dropping 'SHOW_DURATION'?
https://codereview.chromium.org/2602503002/diff/20001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/snackbar.js (right): https://codereview.chromium.org/2602503002/diff/20001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:110: ensureTransitionEndEvent(this, 225); On 2017/01/03 22:50:58, scheib wrote: > Wny dropping 'SHOW_DURATION'? The SHOW_DURATION value was actually the wrong number here. I've made this 225 value a constant in the next patchset. ensureTransitionEndEvent should fire the webkitTransitionEnd event if the hide transition doesn't finish 225ms after dismiss() is called, not 5000ms.
https://codereview.chromium.org/2602503002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/snackbar.js (right): https://codereview.chromium.org/2602503002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:104: } no curlies https://codereview.chromium.org/2602503002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:107: this.boundOnTransitionEnd_ = this.onTransitionEnd_(resolve).bind(this); nit: can we use listenOnce from util.js instead?
also this https://codereview.chromium.org/2602503002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2602503002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:485: } no curlies https://codereview.chromium.org/2602503002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:521: whenSnackbarShows(snackbar1).then(finish); if you drop the |done| argument and just return a Promise, the test will wait for the returned Promise (and any chained promise). I think something like this might work: return whenSnackbarShows(snackbar1).then(function() { return snackbar.Snackbar.dismiss(); }).then(function() { return finishSnackbarTest(); });
The CQ bit was checked by mbrunson@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/2602503002/diff/40001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/snackbar.js (right): https://codereview.chromium.org/2602503002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:104: } On 2017/01/04 01:44:09, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/2602503002/diff/40001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:107: this.boundOnTransitionEnd_ = this.onTransitionEnd_(resolve).bind(this); On 2017/01/04 01:44:09, Dan Beam wrote: > nit: can we use listenOnce from util.js instead? Done. https://codereview.chromium.org/2602503002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2602503002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:485: } On 2017/01/04 01:48:33, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/2602503002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:521: whenSnackbarShows(snackbar1).then(finish); On 2017/01/04 01:48:33, Dan Beam wrote: > if you drop the |done| argument and just return a Promise, the test will wait > for the returned Promise (and any chained promise). > > I think something like this might work: > > return whenSnackbarShows(snackbar1).then(function() { > return snackbar.Snackbar.dismiss(); > }).then(function() { > return finishSnackbarTest(); > }); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/nits https://codereview.chromium.org/2602503002/diff/60001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/snackbar.js (right): https://codereview.chromium.org/2602503002/diff/60001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:102: if (!this.classList.contains('open')) return Promise.resolve(); this differs from our clang-format rules and will eventually need to be split into 2 lines https://codereview.chromium.org/2602503002/diff/60001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:106: this.onTransitionEnd_(resolve).bind(this)); why not just listenOnce(this, 'webkitTransitionEnd', function() { this.dispatchEvent(new CustomEvent('dismissed')); resolve(); }); ? why do you need onTransitionEnd_? https://codereview.chromium.org/2602503002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2602503002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:477: pendingSnackbar.addEventListener('showed', function() { resolve(); }); if you don't care about the argument you're resolving with, pendingSnackbar.addEventListener('showed', resolve);
https://codereview.chromium.org/2602503002/diff/60001/chrome/browser/resource... File chrome/browser/resources/bluetooth_internals/snackbar.js (right): https://codereview.chromium.org/2602503002/diff/60001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:102: if (!this.classList.contains('open')) return Promise.resolve(); On 2017/01/04 22:28:59, Dan Beam wrote: > this differs from our clang-format rules and will eventually need to be split > into 2 lines Done. https://codereview.chromium.org/2602503002/diff/60001/chrome/browser/resource... chrome/browser/resources/bluetooth_internals/snackbar.js:106: this.onTransitionEnd_(resolve).bind(this)); On 2017/01/04 22:28:59, Dan Beam wrote: > why not just > > listenOnce(this, 'webkitTransitionEnd', function() { > this.dispatchEvent(new CustomEvent('dismissed')); > resolve(); > }); > > ? why do you need onTransitionEnd_? Done. https://codereview.chromium.org/2602503002/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/bluetooth_internals_browsertest.js (right): https://codereview.chromium.org/2602503002/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/bluetooth_internals_browsertest.js:477: pendingSnackbar.addEventListener('showed', function() { resolve(); }); On 2017/01/04 22:28:59, Dan Beam wrote: > if you don't care about the argument you're resolving with, > > pendingSnackbar.addEventListener('showed', resolve); Done.
The CQ bit was checked by mbrunson@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.
The CQ bit was checked by mbrunson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2602503002/#ps80001 (title: "Simplify snackbar and test")
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": 80001, "attempt_start_ts": 1483641602600100,
"parent_rev": "d59fe166058d3a4c3f2d44ef5c76b0076315f16e", "commit_rev":
"8f983db3a8ae9058070528ed2ec2034bedc6976e"}
Message was sent while issue was closed.
Description was changed from ========== bluetooth: Fix flaky bluetooth internals browser test. Changes Snackbar#dismiss to return Promise that resolves when snackbar is fully dismissed. Add check for snackbar that's already dismissed to fix test issue where Snackbar is dismissed repeatedly. BUG=676227 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== bluetooth: Fix flaky bluetooth internals browser test. Changes Snackbar#dismiss to return Promise that resolves when snackbar is fully dismissed. Add check for snackbar that's already dismissed to fix test issue where Snackbar is dismissed repeatedly. BUG=676227 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2602503002 Cr-Commit-Position: refs/heads/master@{#441712} Committed: https://chromium.googlesource.com/chromium/src/+/8f983db3a8ae9058070528ed2ec2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8f983db3a8ae9058070528ed2ec2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
