1225832 - Re-enable browser_UITour_loop.js

Status: RESOLVED WONTFIX

(Hello (Loop) :: Client, defect, P2)

Description

[Mike de Boer [:mikedeboer]]

In bug 1048850 we had to disable the UITour unit tests, because it relies too much on synchronous behavior and immediate availability of DOM elements.

We can bring that behavior back if we pre-fetch all the required data from the Loop API _before_ we start rendering anything with React.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: WIP patch

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: fix UITour unit tests for Loop without navigator.mozLoop present

Comment 3

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa762e1c9f35

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1.1: fix UITour unit tests for Loop without navigator.mozLoop present

Forgot to have build-jsx running...

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8689512 [details] [diff] [review]
Patch v1.1: fix UITour unit tests for Loop without navigator.mozLoop present

Review of attachment 8689512 [details] [diff] [review]:
-----------------------------------------------------------------

I like this, because it means we're not getting these async in the views :-)

However, unfortunately for me its failing when I run the test locally:

176 INFO Starting 
177 INFO 	test_setConfiguration
178 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | pref should be false but exist - 
179 INFO == Done test, doing shared checks before teardown ==
180 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Element should not be null, when checking visibility - 
181 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Highlight should be closed/hidden after UITour tab is closed - 
182 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Element should not be null, when checking visibility - 
183 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Tooltip should be closed/hidden after UITour tab is closed - 
184 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | @noautohide on the menu panel should have been cleaned up - 
185 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | The panel shouldn't have @panelopen - 
186 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | The panel shouldn't be open - 
187 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Menu button should know that the menu is closed - 
188 INFO Done shared checks
189 INFO Starting 
190 INFO 	test_resumeViaMenuPanel_roomClosedTabOpen
JavaScript error: chrome://browser/content/loop/shared/js/validate.js, line 101: TypeError: missing required roomList
JavaScript error: chrome://browser/content/loop/shared/js/validate.js, line 101: TypeError: missing required roomList
191 INFO Console message: [JavaScript Error: "TypeError: missing required roomList" {file: "chrome://browser/content/loop/shared/js/validate.js" line: 101}]
192 INFO Console message: [JavaScript Error: "TypeError: missing required roomList" {file: "chrome://browser/content/loop/shared/js/validate.js" line: 101}]

::: browser/components/loop/content/js/conversation.jsx
@@ +104,1 @@
>        ["GetAllConstants"],

I think we should add a note here about the fact that requestIdx is keyed off the order of the requests *and* prefetch, and changes should be careful to update both.

@@ +111,5 @@
> +    ];
> +    var prefetch = [
> +      ["GetConversationWindowData", windowId]
> +    ];
> +    return loop.requestMulti.apply(null, requests.concat(prefetch)).then(function(results) {

nit: blank line before the return please.

::: browser/components/loop/content/js/roomViews.jsx
@@ +284,5 @@
>      handleCopyButtonClick: function(event) {
>        event.preventDefault();
>  
>        this.props.dispatcher.dispatch(new sharedActions.CopyRoomUrl({
> +        roomUrl: this.props.roomData.roomUrl || "dummy",

This doesn't seem to belong to this patch, or if it does, then "dummy" doesn't seem right.

::: browser/components/loop/ui/fake-mozLoop.js
@@ +150,5 @@
>      EnsureRegistered: function() {},
>      GetAudioBlob: function() {
>        return new Blob([new ArrayBuffer(10)], { type: "audio/ogg" });
>      },
> +    GetDoNotDisturb: function() { return true; }, 

nit: eslint says trailing space.

::: browser/components/uitour/test/browser_UITour_loop.js
@@ +224,5 @@
>      });
>      setupFakeRoom();
>      LoopRooms.open("fakeTourRoom");
>    }),
> +  /*runOffline(function test_notifyLoopRoomURLEmailed(done) {

This either needs uncommenting, or a XXX statement with bug number for getting it fixed up.

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #5)
> I like this, because it means we're not getting these async in the views :-)
> 
> However, unfortunately for me its failing when I run the test locally:

Prolly due to bitrot.

Comment 7

[Mike de Boer [:mikedeboer]]

Attachment: Patch v2: fix UITour unit tests for Loop without navigator.mozLoop present

Comment 8

[Mark Banner (:standard8)]

Comment on attachment 8691880 [details] [diff] [review]
Patch v2: fix UITour unit tests for Loop without navigator.mozLoop present

Review of attachment 8691880 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the console.log removed, and the skip = true changed to something more reasonable in browser/components/uitour/test/browser.ini

::: browser/components/loop/content/js/roomViews.jsx
@@ +774,5 @@
>  
>        var shouldRenderInvitationOverlay = this._shouldRenderInvitationOverlay();
>        var shouldRenderEditContextView = this.state.showEditContext;
>        var roomData = this.props.roomStore.getStoreState("activeRoom");
> +      console.log("ROOMDATA::", roomData);

I think you left that in by mistake.

Comment 9

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/958669938c10f37112896f1f47e8ed8074c1495d
Bug 1225832: fix UITour unit tests for Loop without navigator.mozLoop present. r=Standard8

Comment 10

[Mark Banner (:standard8)]

Sorry Mike, I had to back this out for almost perma mochitest failures (mainly on Mac) such as shown below:

Backout
https://hg.mozilla.org/integration/fx-team/rev/f48352b311eb

https://treeherder.mozilla.org/logviewer.html#?job_id=5942896&repo=fx-team
 10:08:31     INFO -  186 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Loop chat window should've opened -
 10:08:31     INFO -  187 INFO TEST-PASS | browser/components/uitour/test/browser_UITour_loop.js | Check Loop:ChatWindowShown notification -
 10:08:31     INFO -  188 INFO Console message: [JavaScript Error: "Content Security Policy: The page's settings blocked the loading of a resource at blob:null/b20939b2-2a6d-4b4b-b60d-41e91540cd38 ("default-src about://:1 about: file: chrome: data: wss://* http://* https://*")."]
 10:08:31     INFO -  189 INFO Console message: [JavaScript Error: "Content Security Policy: The page's settings blocked the loading of a resource at blob:null/81d4ba0c-3247-b44e-901b-65e613af83a0 ("default-src about://:1 about: file: chrome: data: wss://* http://* https://*")."]
 10:08:31     INFO -  190 INFO Console message: [JavaScript Error: "Content Security Policy: The page's settings blocked the loading of a resource at blob:null/6b42eaf0-8092-d546-ac85-0237b6e2381b ("default-src about://:1 about: file: chrome: data: wss://* http://* https://*")."]
 10:08:31     INFO -  191 INFO Console message: [JavaScript Error: "Content Security Policy: The page's settings blocked the loading of a resource at blob:null/1481ee53-5487-a84a-8c9a-b30b19fe28d0 ("default-src about://:1 about: file: chrome: data: wss://* http://* https://*")."]
 10:08:31     INFO -  192 INFO TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_loop.js | A promise chain failed to handle a rejection:  - Email button should be there
 10:08:31     INFO -  Stack trace:
 10:08:31     INFO -  JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: PendingErrors.register :: line 191
 10:08:31     INFO -  JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.completePromise :: line 715
 10:08:31     INFO -  JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: Handler.prototype.process :: line 970
 10:08:31     INFO -  JS frame :: resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js :: this.PromiseWalker.walkerLoop :: line 813
 10:08:42     INFO -  193 INFO Console message: 1448647722113	Toolkit.Telemetry	WARN	TelemetryStorage::_scanArchive - have seen this id before: 18bb372c-ef1e-2a48-8af7-4d756b1e37f4, overwrite: false
 10:08:43     INFO -  194 INFO Console message: [JavaScript Error: "1448647723623	Toolkit.GMP	ERROR	GMPInstallManager.simpleCheckAndInstall Could not check for addons: Error: got node name: html, expected: updates (resource://gre/modules/addons/ProductAddonChecker.jsm:145:1) JS Stack trace: [e-mail]:145:1 < promise callback*[e-mail]:299:12 < [e-mail]:107:5 < GMPInstallManager.prototype.simpleCheckAndInstall<@GMPInstallManager.jsm:213:29 < gBrowserInit._delayedStartup/<@browser.js:12323:7 < setTimeout handler*[e-mail]:12319:1 < EventListener.handleEvent*[e-mail]:12040:5 < [e-mail]:1:1" {file: "resource://gre/modules/Log.jsm" line: 751}]
 10:09:06     INFO -  Not taking screenshot here: see the one that was previously logged
 10:09:06     INFO -  195 INFO TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour_loop.js | Test timed out -
 10:09:06     INFO -  MEMORY STAT | vsize 3456MB | residentFast 458MB | heapAllocated 96MB
 10:09:06     INFO -  196 INFO TEST-OK | browser/components/uitour/test/browser_UITour_loop.js | took 45068ms
 10:09:06     INFO -  197 INFO TEST-START | browser/components/uitour/test/browser_UITour_modalDialog.js

Comment 11

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=042035bfc3bd

Comment 12

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea249dde1d6

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch v3: WITHOUT enabling browser_UITour_loop.js on infra

Carrying over r=Standard8

Comment 14

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/da1191bc1a8e92ffaa75f7d39462ce71b5f480df
Bug 1225832: partially fix UITour for Loop without navigator.mozLoop present. r=Standard8

Comment 15

[Mike de Boer [:mikedeboer]]

What I did in comment 14 was land all the changes to make the UITour work _at least_ for the panel and partially the conversation window.

The test failure on OSX 10.10 - which blocks re-enabling the test on infra through browser.ini - is intriguing and I can not reproduce it locally. This means that I will need several try pushes with added logging and possibly screen dumps to figure out what's going on exactly on those boxes.
The time I will spend on this is unknown, because I need to 1) push to try 2) wait 'till build and tests finish 3) examine logs 4) implement fix or extend logging 5) rinse, repeat. Therefore I chose to land the LoopAPI client prefetching mechanism to make things work better in the meanwhile.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/da1191bc1a8e

Comment 17

[Romain Testard [:RT]]

Closing as WON'T FIX since we will not use the FTU door hangers anymore in the new FTU.

Comment 18

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.