436553 - Automate litmus test for closing the download manager.

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Shawn Wilsher :sdwilsh]

This should be a chrome test that lives in toolkit/mozapps/downloads/tests/chrome/ to replace this litmus test:
https://litmus.mozilla.org/show_test.cgi?id=3979

Comment 1

[Anoop Saldanha (Poona)]

Attachment: v1.0

I have the Makefile.in in the patch too.  Do have a look at the Makefile.in.  The issue is, I think a Makefile.in has already been written for this directory by edward.  A file that you supplied yesterday, was actually a chrome test for dm, written by edward.  That file isn't in my src, maybe because he wrote it recently.  If that is the case I can integrate my Makefile.in with the already existant Makefile.in once I update the trunk on my machine

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 327071 [details] [diff] [review]
v1.0

Please update this patch to latest in mozilla-central

here are some comments

>+/*
nit: extra * here please

>+ * Litmus Testcase ID #3979 - Various methods to close the Downloads dialog
>+ * Verify the various methods to close the download manager
>+ *	- Press "Esc"
>+ *	- Ctrl+W (Mac: Cmd+W)
>+ *	- Click "X" close window button in the titlebar.<-- Can't be implemented since "X"
>+ *                                                      button is a part of the native widget.
>+ * Each of the actions should close the Downloads dialog if it is open and in focus.	
I'm actually not interested in the litmus testcase id, because the tests are either going to be disabled or modified to cover what we couldn't automate.  With that said, please only include what we are actually testing in this block.

>+<window title="Download Manager Test"
>+  onload="setTimeout(runTest, 0);"
>+  xmlns:html="http://www.w3.org/1999/xhtml"
>+  xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
nit: attributes should line up

>+  <title>Panel Focus Tests</title>
huh?

>+  <script>
script needs the type set

nit: let's not indent all this JS.  It doesn't really buy us anything

>+    function testCloseDMEscKey()
>+    {
>+      let dm_win = wm.getMostRecentWindow("Download:Manager");
>+      let [key, event] = getCloseKeys();
>+      
>+      EventUtils.synthesizeKey(key, event, dm_win);
>+      ok(!dmui.visible, "DMUI fails to close with Esc");
>+      return;
just use the ESC key

>+    function testCloseDMMetaKey()
nit: with Accel-key?

>+    function getCloseKeys()
>+    {
>+      switch (testPhase - 1) {
>+        case 0 :
>+          return ["VK_ESC", { }];
>+          break;
>+        case 1 :
>+          if (navigator.platform.search("Mac") == 0) return ["w", { metaKey: true }];
>+          else return ["w", { ctrlKey: true }];
>+          break;
>+      }
>+    }
The whole global state thing is just confusing.  Just use this function for the close key.

Comment 3

[Anoop Saldanha (Poona)]

Attachment: v2.0

Comment 4

[Anoop Saldanha (Poona)]

Attachment: v3.0

Comment 5

[Anoop Saldanha (Poona)]

Attachment: v4.0

Comment 6

[Anoop Saldanha (Poona)]

Attachment: v5.0

Comment 7

[Anoop Saldanha (Poona)]

Attachment: v6.0

Comment 8

[Anoop Saldanha (Poona)]

Attachment: v7.0

Have run the test.  It works.  I checked if dmui.visible before I close it and I get true.

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 329694 [details] [diff] [review]
v7.0

>+++ b/toolkit/mozapps/downloads/tests/chrome/Makefile.in
>@@ -61,6 +61,7 @@ _CHROME_FILES = \
>   test_select_all.xul \
>   test_space_key_pauses_resumes.xul \
>   test_ui_stays_open_on_alert_clickback.xul \
>+  test_close_download_manager.xul \
>   test_bug_429247.xul \
>   utils.js \
>   $(NULL)
Keep this list alphabetized please

>+++ b/toolkit/mozapps/downloads/tests/chrome/test_close_download_manager.xul
>+/**
>+ * This tests bug 436553.  This test basically checks the various methods to 
>+ * close the download manager.- the ESC key and and the accel-key.
>+ */
Doesn't quite seem right...

>+// Variable to indicate the test that is running.  Make it global since we
>+// need it in more than 2 functions
don't worry about the comment - not needed

>+var testPhase = 0;
s/var/let/

>+function getCloseKeys()
>+{
>+  if (navigator.platform.search("Mac") == 0)
>+    return ["w", { metaKey: true }];
>+  else 
>+    return ["w", { ctrlKey: true }];
>+}
Don't need this

>+function testCloseDMWithEscKey(aWin)
>+{
>+  function dmWindowClosedListener() {
>+    ok(!dmui.visible, "DMUI closes with ESC key");
>+    dmui.show();
>+  }
>+  aWin.addEventListener("unload", dmWindowClosedListener, false);
I think you need to remove the event listener in your function

>+function testCloseDMWithAccelKey(aWin)
>+{
>+  let [key, event] = getCloseKeys();
>+
>+  function dmWindowClosedListener() {
>+    ok(!dmui.visible, "DMUI closes with [ctrl/meta]-w");
just say accel

>+  synthesizeKey(key, event, aWin);
just do synthesizeKey("w", {accelKey:true}, aWin);

>+  let testObs = {
>+    observe: function(aSubject, aTopic, aData) {
>+      let win = aSubject.QueryInterface(Ci.nsIDOMWindow);
>+      if(testPhase == 0) {
>+        testPhase++;
>+        testCloseDMWithEscKey(win);
>+      }
>+      else {
>+        obs.removeObserver(testObs, DLMGR_UI_DONE);
>+        testCloseDMWithAccelKey(win);
use a switch please

>+  <body xmlns="http://www.w3.org/1999/xhtml">
>+    <p id="display">
>+    </p>
>+    <div id="content" style="display: none">
>+    </div>
>+    <pre id="test">
>+    </pre>
>+  </body>
Don't change all this formatting - keep it how all the other test files are please

Comment 10

[Anoop Saldanha (Poona)]

Attachment: v8.0

Patch updated with the corrections requested.  Tested - Passes

Comment 11

[Anoop Saldanha (Poona)]

Attachment: v9.0

Solved the issue I have with utils.js :)
I have updated the patch now to use the functions from utils.js

Test runs

Comment 12

[Anoop Saldanha (Poona)]

Attachment: v10.0

Test updated to accomodate the new utils.js.  The test passes.

Comment 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 331506 [details] [diff] [review]
v10.0

>+/**
>+ * This tests bug 436553.  This test basically checks if the download manager
>+ * closes when you press the esy key and accelKey-w.
>+ */
lose the bug number (per previous comments)
s/esy/esc/
just say accel + w

>+function testCloseDMWithAccelKey(aWin)
>+{
>+  function dmWindowClosedListener() {
>+    aWin.removeEventListener("unload", dmWindowClosedListener, false);
>+    ok(!dmui.visible, "DMUI closes with [ctrl/meta]-w");
accel + w instead of the ctrl...

r=sdwilsh with those changes

Comment 14

[Anoop Saldanha (Poona)]

Attachment: v11.0 [Checkin: Comment 17]

Test updated to accommodate the reviews :)

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 334282 [details] [diff] [review]
v11.0
[Checkin: Comment 17]

r=sdwilsh

Comment 16

[Shawn Wilsher :sdwilsh]

The litmus test for this can be disabled once this lands.

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 334282 [details] [diff] [review]
v11.0
[Checkin: Comment 17]

I fixed
[
patching file toolkit/mozapps/downloads/tests/chrome/Makefile.in
Hunk #1 FAILED at 48
1 out of 1 hunk FAILED -- saving rejects to file [...]
]

http://hg.mozilla.org/mozilla-central/rev/c215871b5f6d

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #16)
> The litmus test for this can be disabled once this lands.

Can I disable it, even with the failing hunk?  The HG log looks like it all landed...

Comment 19

[Shawn Wilsher :sdwilsh]

Yeah - it was just conflicting stuff in the Makefile that Serge fixed at checkin.

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

https://litmus.mozilla.org/show_test.cgi?id=3979 is around, but disabled; thanks!

These tests rock -- not sure how to verify, or I can, yet...