Use shared-head.js in Storage Inspector mochitests

RESOLVED FIXED in Firefox 48

Status

DevTools
Storage Inspector
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

Trunk
Firefox 48

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
Assignee: nobody → jsnajdr
See Also: → bug 1231437
(Assignee)

Comment 7

2 years ago
Created attachment 8740893 [details] [diff] [review]
Part 1: fix promise rejections in storage inspector tests.
Attachment #8740893 - Flags: review?(mratcliffe)
(Assignee)

Comment 8

2 years ago
Created attachment 8740894 [details] [diff] [review]
Part 2: use shared-head.js for storage inspector tests.
Attachment #8740894 - Flags: review?(mratcliffe)
(Assignee)

Comment 9

2 years ago
Created attachment 8740895 [details] [diff] [review]
Part 3: make affected JS files eslint-clean.
Attachment #8740895 - Flags: review?(mratcliffe)
(Assignee)

Comment 10

2 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)

Comment 12

2 years ago
Created attachment 8741792 [details] [diff] [review]
Part 3: make affected JS files eslint-clean.

Fixed a typo in comment
Attachment #8741792 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8740895 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
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)
(Assignee)

Comment 16

2 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

2 years ago
Created attachment 8742224 [details] [diff] [review]
Part 4: add error handling to fetchStorageObjects.

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+
(Assignee)

Comment 20

2 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

2 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

2 years ago
Created attachment 8742693 [details] [diff] [review]
Part 1: fix promise rejections in storage inspector tests.
Attachment #8742693 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8740893 - Attachment is obsolete: true
Attachment #8742224 - Attachment is obsolete: true
(Assignee)

Comment 23

2 years ago
Created attachment 8742694 [details] [diff] [review]
Part 2: use shared-head.js for storage inspector tests.
Attachment #8742694 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8740894 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
Created attachment 8742695 [details] [diff] [review]
Part 3: make affected JS files eslint-clean.
Attachment #8742695 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8741792 - Attachment is obsolete: true
(Assignee)

Comment 26

2 years ago
Created attachment 8742746 [details] [diff] [review]
Part 4: fix the click offset in waitForContextMenu.

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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 30

2 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
Last Resolved: 2 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.