Closed Bug 1261785 Opened 8 years ago Closed 8 years ago

Use shared-head.js in Storage Inspector mochitests

Categories

(DevTools :: Storage Inspector, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

Attachments

(4 files, 5 obsolete files)

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: nobody → jsnajdr
See Also: → 1231437
Attachment #8740893 - Flags: review?(mratcliffe)
Attachment #8740894 - Flags: review?(mratcliffe)
Attachment #8740895 - Flags: review?(mratcliffe)
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+
Fixed a typo in comment
Attachment #8741792 - Flags: review+
Attachment #8740895 - Attachment is obsolete: true
Keywords: checkin-needed
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)
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)
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+
Try run looks good, there are many intermittents in debugger that are not related. Requesting checkin again.
Keywords: checkin-needed
Cancelling checkin-needed until I rebase the patches against latest mozilla-central, and do one more try run, to be sure.
Keywords: checkin-needed
Attachment #8740893 - Attachment is obsolete: true
Attachment #8742224 - Attachment is obsolete: true
Attachment #8740894 - Attachment is obsolete: true
Attachment #8741792 - Attachment is obsolete: true
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 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+
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.