Closed Bug 436553 Opened 13 years ago Closed 13 years ago

Automate litmus test for closing the download manager.

Categories

(Toolkit :: Downloads API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: sdwilsh, Assigned: poonaatsoc)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 10 obsolete files)

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
Attached patch v1.0 (obsolete) — Splinter Review
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
Attachment #327071 - Flags: review?(sdwilsh)
Blocks: 442327
No longer blocks: gsoc
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.
Attachment #327071 - Flags: review?(sdwilsh)
Attached patch v2.0 (obsolete) — Splinter Review
Attachment #327071 - Attachment is obsolete: true
Attachment #329252 - Flags: review?(sdwilsh)
Attached patch v3.0 (obsolete) — Splinter Review
Attachment #329252 - Attachment is obsolete: true
Attachment #329296 - Flags: review?(sdwilsh)
Attachment #329252 - Flags: review?(sdwilsh)
Attached patch v4.0 (obsolete) — Splinter Review
Attachment #329296 - Attachment is obsolete: true
Attachment #329320 - Flags: review?(sdwilsh)
Attachment #329296 - Flags: review?(sdwilsh)
Attached patch v5.0 (obsolete) — Splinter Review
Attachment #329320 - Attachment is obsolete: true
Attachment #329325 - Flags: review?(sdwilsh)
Attachment #329320 - Flags: review?(sdwilsh)
Attached patch v6.0 (obsolete) — Splinter Review
Attachment #329325 - Attachment is obsolete: true
Attachment #329441 - Flags: review?(sdwilsh)
Attachment #329325 - Flags: review?(sdwilsh)
Attached patch v7.0 (obsolete) — Splinter Review
Have run the test.  It works.  I checked if dmui.visible before I close it and I get true.
Attachment #329441 - Attachment is obsolete: true
Attachment #329694 - Flags: review?(sdwilsh)
Attachment #329441 - Flags: review?(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
Attachment #329694 - Flags: review?(sdwilsh) → review-
Attached patch v8.0 (obsolete) — Splinter Review
Patch updated with the corrections requested.  Tested - Passes
Attachment #329694 - Attachment is obsolete: true
Attachment #329994 - Flags: review?(sdwilsh)
Attached patch v9.0 (obsolete) — Splinter Review
Solved the issue I have with utils.js :)
I have updated the patch now to use the functions from utils.js

Test runs
Attachment #329994 - Attachment is obsolete: true
Attachment #330341 - Flags: review?(sdwilsh)
Attachment #329994 - Flags: review?(sdwilsh)
Attached patch v10.0 (obsolete) — Splinter Review
Test updated to accomodate the new utils.js.  The test passes.
Attachment #330341 - Attachment is obsolete: true
Attachment #331506 - Flags: review?(sdwilsh)
Attachment #330341 - Flags: review?(sdwilsh)
Product: Firefox → Toolkit
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
Attachment #331506 - Flags: review?(sdwilsh) → review+
Test updated to accommodate the reviews :)
Attachment #331506 - Attachment is obsolete: true
Attachment #334282 - Flags: review?(sdwilsh)
Comment on attachment 334282 [details] [diff] [review]
v11.0
[Checkin: Comment 17]

r=sdwilsh
Attachment #334282 - Flags: review?(sdwilsh) → review+
The litmus test for this can be disabled once this lands.
Flags: in-litmus?
Keywords: checkin-needed
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
Attachment #334282 - Attachment description: v11.0 → v11.0 [Checkin: Comment 17]
Keywords: checkin-needed
Whiteboard: [ToDo: comment 16]
Target Milestone: --- → mozilla1.9.1b1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [ToDo: comment 16]
(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...
Yeah - it was just conflicting stuff in the Makefile that Serge fixed at checkin.
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...
Flags: in-litmus? → in-litmus-
You need to log in before you can comment on or make changes to this bug.