Closed
Bug 1261785
Opened 8 years ago
Closed 8 years ago
Use shared-head.js in Storage Inspector mochitests
Categories
(DevTools :: Storage Inspector, defect)
DevTools
Storage Inspector
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jsnajdr, Assigned: jsnajdr)
References
Details
Attachments
(4 files, 5 obsolete files)
11.39 KB,
patch
|
jsnajdr
:
review+
|
Details | Diff | Splinter Review |
17.21 KB,
patch
|
jsnajdr
:
review+
|
Details | Diff | Splinter Review |
26.27 KB,
patch
|
jsnajdr
:
review+
|
Details | Diff | Splinter Review |
906 bytes,
patch
|
jdescottes
:
review+
|
Details | Diff | Splinter Review |
Refactor the Storage Inspector mochitests to include shared-head.js. The advantage is that common implementations of functions like addTab or waitForContextMenu can be used. Now, they are often copy&pasted across different head.js's, and there are slight variations among them, making it very confusing to figure out which is the "right" one. I tried to do this already as a drive-by change in bug 1231437, but had to backout because of test failures. The shared-head.js registers a cleanup function that closes the devtools toolbox, which triggers some race conditions that were not happening before.
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b7f489c40d3
Assignee | ||
Comment 3•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad502acb8d24
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=617ea5c23496
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b927c2e6569c
Assignee | ||
Comment 6•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1efba383302
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8740893 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8740894 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8740895 -
Flags: review?(mratcliffe)
Assignee | ||
Comment 10•8 years ago
|
||
Another rather big patch to clean up storage inspector tests: Part 1: Promise rejections Fixed the promise rejection failures in storage inspector tests. They were caused by the test triggering a getEditableFields request to the server and then closing the toolbox and shutting down before the reply arrived. The unresolved promise is then rejected when the reply finally arrives. Fixed this changing the fetchStorageObjects: - not calling makeFieldsEditable on every fetchStorageObjects, but only when the reason is POPULATE. It's needed only here, when the table colums are reset and reconfigured. - in fetchStorageObjects, emitting the "store-objects-updated" event only after both "getStoreObjects" and "getEditableFields" request are complete. This makes the tests cleaner, because openTabAndSetupStorage and selectTreeItem now guarantee that everything is loaded property when they return - data are loaded, columns updated, editor initialized. No need to wait for the FIELDS_EDITABLE event. - rewrote the fetchStorageObjects method into a more elegant Task.async form, together with makeFieldsEditable and resetColumns that are called from it. Part 2: Use shared-head.js in storage inspector tests - moved function waitForContextMenu from webconsole/test/head.js to shared-head.js - added call to Element.scrollIntoView() into this function - removed explicit call to scrollIntoView from places where it was called before waitForContextMenu - included shared-head.js in storage/test/head.js and removed unneeded code from head.js (things already done in shared-head.js) - fixed the browser_layoutHelpers-getBoxQuads.js - removed redefinition of Cu, which is now an error because the original definition in shared-head.js is now "const" instead of "var". Part 3: Solve ESLint errors in all affected files - no comment neccessary
Attachment #8740893 -
Flags: review?(mratcliffe) → review+
Attachment #8740894 -
Flags: review?(mratcliffe) → review+
Comment on attachment 8740895 [details] [diff] [review] Part 3: make affected JS files eslint-clean. Review of attachment 8740895 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/test/shared-head.js @@ +169,5 @@ > * @param {Object} target An observable object that either supports on/off or > * addEventListener/removeEventListener > * @param {String} eventName > + * @param {Boolean} useCapture Optional, for > + * addventListener/removeEventListener Nit: addventListener/removeEventListener should be addEventListener/removeEventListener
Attachment #8740895 -
Flags: review?(mratcliffe) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8740895 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/ff8aa19ba327 https://hg.mozilla.org/integration/fx-team/rev/5d81dab28e18 https://hg.mozilla.org/integration/fx-team/rev/735739a3a13a
Keywords: checkin-needed
Comment 14•8 years ago
|
||
Backed this out for frequent failure in devtools test devtools/client/shared/test/browser_telemetry_toolboxtabs_storage.js Push with failure: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=63f11876b0b8ed6f49c697e5072da6eaeddbf2e0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=8743406 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8743406&repo=fx-team 17:00:01 INFO - 237 INFO TEST-UNEXPECTED-FAIL | devtools/client/shared/test/browser_telemetry_toolboxtabs_storage.js | A promise chain failed to handle a rejection: - at resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1125 - Error: Connection closed, pending request to server1.conn24.cookies28, type getStoreObjects failed 17:00:01 INFO - Request stack: 17:00:01 INFO - Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7 17:00:01 INFO - frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14 17:00:01 INFO - StorageUI.prototype.fetchStorageObjects<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:415:24 17:00:01 INFO - TaskImpl_run@resource://gre/modules/Task.jsm:319:40 17:00:01 INFO - TaskImpl@resource://gre/modules/Task.jsm:280:3 17:00:01 INFO - createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:254:14 17:00:01 INFO - StorageUI.prototype.onHostSelect@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:646:5 17:00:01 INFO - EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:163:11 17:00:01 INFO - TreeWidget.prototype.selectedItem@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TreeWidget.js:82:7 17:00:01 INFO - StorageUI.prototype.populateStorageTree@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:466:43 17:00:01 INFO - StorageUI/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:105:5 17:00:01 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 17:00:01 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 17:00:01 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 17:00:01 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 17:00:01 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7 17:00:01 INFO - Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9 17:00:01 INFO - DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7 17:00:01 INFO - frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14 17:00:01 INFO - StorageUI@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:104:3 17:00:01 INFO - StoragePanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:54:17 17:00:01 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 17:00:01 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 17:00:01 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 17:00:01 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 17:00:01 INFO - Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5 17:00:01 INFO - StoragePanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:50:12 17:00:01 INFO - Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1310:19 17:00:01 INFO - Stack trace: 17:00:01 INFO - Front<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1125:23 17:00:01 INFO - Pool<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:820:9 17:00:01 INFO - Front<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1127:5 17:00:01 INFO - StoragePanel.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:74:7 17:00:01 INFO - Toolbox.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:2057:26 17:00:01 INFO - DT_closeToolbox@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/devtools.js:470:12 17:00:01 INFO - openAndCloseToolbox@chrome://mochitests/content/browser/devtools/client/shared/test/head.js:210:11 17:00:01 INFO - StorageUI.prototype.onHostSelect@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:646:5 17:00:01 INFO - EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:163:11 17:00:01 INFO - TreeWidget.prototype.selectedItem@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/TreeWidget.js:82:7 17:00:01 INFO - StorageUI.prototype.populateStorageTree@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:466:43 17:00:01 INFO - StorageUI/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:105:5 17:00:01 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 17:00:01 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 17:00:01 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 17:00:01 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 17:00:01 INFO - this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:714:7 17:00:01 INFO - Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9 17:00:01 INFO - DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7 17:00:01 INFO - frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14 17:00:01 INFO - StorageUI@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/ui.js:104:3 17:00:01 INFO - StoragePanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:54:17 17:00:01 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23 17:00:01 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:816:7 17:00:01 INFO - Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:11 17:00:01 INFO - this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:779:7 17:00:01 INFO - Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:454:5 17:00:01 INFO - StoragePanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/storage/panel.js:50:12 17:00:01 INFO - Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1310:19
Flags: needinfo?(jsnajdr)
Assignee | ||
Comment 15•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=903d0ac7b3cb
Assignee | ||
Comment 16•8 years ago
|
||
The failing tests opens repeatedly the devtools toolbox with Storage Inspector for 200ms and then closes it. If the getStoreObjects server call is not finished in that time, it will fail with a promise rejection when it returns a response later. I fixed this by adding a catch block that catches and reports the error - see the "Part 4" patch. This error handler was there previously, and I removed it during the refactoring. That was not so wise, it seems.
Flags: needinfo?(jsnajdr)
Assignee | ||
Comment 17•8 years ago
|
||
Added a catch block to prevent test failure.
Attachment #8742224 -
Flags: review?(mratcliffe)
Comment on attachment 8742224 [details] [diff] [review] Part 4: add error handling to fetchStorageObjects. Review of attachment 8742224 [details] [diff] [review]: ----------------------------------------------------------------- I thought this might find it's way back.
Attachment #8742224 -
Flags: review?(mratcliffe) → review+
Comment 19•8 years ago
|
||
Backout: https://hg.mozilla.org/mozilla-central/rev/20eec5d1b590 https://hg.mozilla.org/mozilla-central/rev/ba3c8cb72bda https://hg.mozilla.org/mozilla-central/rev/f8c0dd78206c
Assignee | ||
Comment 20•8 years ago
|
||
Try run looks good, there are many intermittents in debugger that are not related. Requesting checkin again.
Keywords: checkin-needed
Assignee | ||
Comment 21•8 years ago
|
||
Cancelling checkin-needed until I rebase the patches against latest mozilla-central, and do one more try run, to be sure.
Keywords: checkin-needed
Assignee | ||
Comment 22•8 years ago
|
||
Attachment #8742693 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8740893 -
Attachment is obsolete: true
Attachment #8742224 -
Attachment is obsolete: true
Assignee | ||
Comment 23•8 years ago
|
||
Attachment #8742694 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8740894 -
Attachment is obsolete: true
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8742695 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8741792 -
Attachment is obsolete: true
Assignee | ||
Comment 25•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c32c74e1de43
Assignee | ||
Comment 26•8 years ago
|
||
Apply the changes to waitForContextMenu from bug 1258114 to the shared-head.js incarnation of the function. Changes the click offset from [2,2] to [5,2], because there can be an overlapping splitter if we're too close to the element's border.
Attachment #8742746 -
Flags: review?(jdescottes)
Comment 27•8 years ago
|
||
Comment on attachment 8742746 [details] [diff] [review] Part 4: fix the click offset in waitForContextMenu. Looks good to me, sorry about the conflicts!
Attachment #8742746 -
Flags: review?(jdescottes) → review+
Assignee | ||
Comment 28•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a132c6aa5f1
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/681e7c3bd5e4 https://hg.mozilla.org/integration/fx-team/rev/acb76d90e671 https://hg.mozilla.org/integration/fx-team/rev/15763e59a810 https://hg.mozilla.org/integration/fx-team/rev/f4dd4c0aaadc
Keywords: checkin-needed
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/681e7c3bd5e4 https://hg.mozilla.org/mozilla-central/rev/acb76d90e671 https://hg.mozilla.org/mozilla-central/rev/15763e59a810 https://hg.mozilla.org/mozilla-central/rev/f4dd4c0aaadc
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•