Implement chrome.experimental.downloads.onChanged

chrome/test/data/extensions/api_test/downloads/test.js

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // downloads api test
 // browser_tests.exe --gtest_filter=DownloadsApiTest.Downloads
 
 var downloads = chrome.experimental.downloads;
 
 chrome.test.getConfig(function(testConfig) {
@@ skipping 30 matching lines @@
   }
 
   // The "/slow" handler waits a specified amount of time before returning a
   // safe file. Specify zero seconds to return quickly.
   var SAFE_FAST_URL = getURL('slow?0');
   var NEVER_FINISH_URL = getURL('download-known-size');
   var ERROR_GENERIC = downloads.ERROR_GENERIC;
   var ERROR_INVALID_URL = downloads.ERROR_INVALID_URL;
   var ERROR_INVALID_OPERATION = downloads.ERROR_INVALID_OPERATION;
 
+  console.log("I know all these console.logs stink up the place, but please " +
+              "leave them in: they will help you check that all the " +
+              "callbacks for a particular test run before the test harness " +
+              "thinks that that test is finished and that that test doesn't " +
+              "'finish' twice.");
+  console.log("All log lines that contain a download_id should appear " +
+              "before that test finishes. If a callback for test N runs " +
+              "after test N+1 begins, then that indicates a race condition " +
+              "in test N.");

[Randy Smith (Not in Mondays), 2012/02/02 18:10:07]

Is there really no way for us to check for this programmatically?  That's worlds
better than relying on people to look at the test output--after you check this
in, that's not going to happen unless one of us suspects a problem.

(And if we can check for it programmatically, then we can pull the console
logs.)

[benjhayden, 2012/02/02 20:40:38]

chrome.test already tests it programmatically by asserting that the pending
callback counter doesn't jump around.
However, the race conditions mean that, if you forget to surround a callback
with callbackAdded()/callbackCompleted(), then sometimes those assertions will
pass and sometimes they'll fail with a less-than-helpful error message. If every
callback log()s, then it's much easier to find the untracked callback.

[Randy Smith (Not in Mondays), 2012/02/03 20:02:00]

Would it satisfy your goals to change the above two console.log's to in-file
comments and leave the logging below?

[benjhayden, 2012/02/10 18:53:07]

I think that if I saw test output as verbose as this without knowing why, then
I'd probably delete all the print statements before reading all the comments in
the file in case any of them explain why I shouldn't. I can turn these logs into
comments if you think I'm the only one who would do that.

[Randy Smith (Not in Mondays), 2012/02/13 16:13:08]

It looks like you've nuked the logging; was that in response to this comment
thread?  I didn't see other comment threads on this topic.

(Note that I'm not objecting--I'm happier without the logging.  Just confused.)

[benjhayden, 2012/02/13 19:01:31]

Chris said that it was generally not acceptable.

+
   chrome.test.runTests([

[Randy Smith (Not in Mondays), 2012/02/02 18:10:07]

Could you put in a short comment about what these tests are testing and why? 
They end up looking pretty sparse reading through them, which I presume is
because we're testing the real functionality using the C++ tests, but a comment
explaining that would be a win.

[benjhayden, 2012/02/02 20:40:38]

Um, you might want to have a look at download_extension_test.cc and see what
features you think should be tested somewhere and aren't.
Comments done.

[Randy Smith (Not in Mondays), 2012/02/03 20:02:00]

I think I didn't read as closely as I should have.  It looks ok modulo my
questions about method and headers elsewhere.

[benjhayden, 2012/02/10 18:53:07]

Done.

+    function downloadSimple() {
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+      downloads.download(
+          {'url': SAFE_FAST_URL},
+          chrome.test.callbackPass(function(id) {
+            console.log("id: " + download_id);
+            chrome.test.assertEq(download_id, id);
+          }));
+    },
+    function downloadPost() {
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+      downloads.download(
+          {'url': getURL('files/post/downloads/a_zip_file.js'),
+           'method': 'POST',
+           'body': 'WOOHOO'},
+          chrome.test.callbackPass(function(id) {
+            console.log("id: " + download_id);
+            chrome.test.assertEq(download_id, id);
+          }));
+    },
+    function downloadHeader() {
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+      downloads.download(
+          {'url': SAFE_FAST_URL,
+           'headers': [{'name': 'Foo', 'value': 'bar'}]
+          },
+          chrome.test.callbackPass(function(id) {
+            console.log("id: " + download_id);
+            chrome.test.assertEq(download_id, id);
+          }));
+    },
+    function downloadInterrupted() {
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+
+      var createdCompleted = chrome.test.callbackAdded();
+      function createdListener(created_item) {
+        console.log("created_id: " + created_item.id);
+        if (created_item.id != download_id)
+          return;
+        // TODO(benjhayden) Move this cancel() into the download() callback
+        // after ensuring that DownloadItems are created before that callback
+        // is fired.
+        downloads.cancel(download_id, chrome.test.callback(function() {
+          console.log("id: " + download_id);
+        }));
+        downloads.onCreated.removeListener(createdListener);
+        createdCompleted();
+      }
+      downloads.onCreated.addListener(createdListener);
+
+      var changedCompleted = chrome.test.callbackAdded();
+      function changedListener(delta) {
+        console.log("id: " + delta.id);
+        if ((delta.id == download_id) &&
+            delta.state &&
+            (delta.state.new == 'interrupted')) {
+          console.log("id: " + delta.id);
+          downloads.onChanged.removeListener(changedListener);
+          changedCompleted();
+        }
+      }
+      downloads.onChanged.addListener(changedListener);
+
+      downloads.download(
+          {'url': NEVER_FINISH_URL},
+          chrome.test.callback(function(id) {
+            console.log("id: " + download_id);
+            chrome.test.assertEq(download_id, id);
+          }));
+    },
     // TODO(benjhayden): Test onErased using remove().
+    function downloadOnChanged() {
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+      var callbackCompleted = chrome.test.callbackAdded();
+      console.log("id: " + download_id);
+      function myListener(delta) {
+        console.log("id: " + delta.id);
+        if (delta.state) console.log("state: " + delta.state.new);
+        if (delta.state && delta.state.new == 'complete') {
+          downloads.onChanged.removeListener(myListener);
+          callbackCompleted();
+        }
+      }
+      downloads.onChanged.addListener(myListener);
+      downloads.download(
+        {"url": getURL("slow?0")},
+        chrome.test.callback(function(id) {
+          console.log("id: " + download_id);
+          chrome.test.assertEq(download_id, id);
+      }));
+    },
     function downloadFilename() {
+      const kFilename = 'owiejtoiwjrfoiwjroiwjroiwjroiwjrfi';
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+      var callbackCompleted = chrome.test.callbackAdded();
+      function myListener(delta) {
+        console.log("id: " + download_id);
+        if (delta.filename) console.log("filename: " + delta.filename.new);
+        if (delta.filename && delta.filename.new.indexOf(kFilename) !== -1) {
+          downloads.onChanged.removeListener(myListener);
+          callbackCompleted();
+        }
+      }
+      downloads.onChanged.addListener(myListener);
       downloads.download(
-          {'url': SAFE_FAST_URL, 'filename': 'foo'},
-          chrome.test.callbackPass(function(id) {
-            chrome.test.assertEq(getNextId(), id);
+          {'url': SAFE_FAST_URL, 'filename': kFilename},
+          chrome.test.callback(function(id) {
+            console.log("id: " + download_id);
+            chrome.test.assertEq(download_id, id);
           }));
-      // TODO(benjhayden): Test the filename using onChanged.
     },
     function downloadOnCreated() {
-      chrome.test.listenOnce(downloads.onCreated,
-                             chrome.test.callbackPass(function(item) {}));
+      var download_id = getNextId();
+      console.log("id: " + download_id);
+      chrome.test.listenOnce(downloads.onCreated, function(item) {
+        // This callback is implicitly wrapped by chrome.test.callbackPass(),
+        // so if onCreated never fires, then this test will timeout.
+      });
       downloads.download(
           {'url': SAFE_FAST_URL},
-          function(id) {
-            chrome.test.assertEq(getNextId(), id);
-          });
+          chrome.test.callback(function(id) {
+            chrome.test.assertEq(download_id, id);
+          }));
     },
-    function downloadSubDirectoryFilename() {
-      downloads.download(
-          {'url': SAFE_FAST_URL, 'filename': 'foo/slow'},
-          chrome.test.callbackPass(function(id) {
-            chrome.test.assertEq(getNextId(), id);
-          }));
-      // TODO(benjhayden): Test the filename using onChanged.
-    },
+    // TODO(benjhayden): Talk to Asanka about a way to make the filename
+    // sanitizer respect filenames from the downloads api (instead of replace
+    // slashes with underscores).

[Randy Smith (Not in Mondays), 2012/02/02 18:10:07]

I'd suggest you have a plan sketch (and maybe a bug id) for this before you
commit.

[benjhayden, 2012/02/02 20:40:38]

Added the bug id, and Asanka has a vague plan if I understand him correctly. I'd
rather not write the plan of attack here because it might change, and that kind
of discussion seems to belong in the bug thread.

[Randy Smith (Not in Mondays), 2012/02/03 20:02:00]

Yep; if there's a bug id I agree the discussion should go in the bug.  Thanks.

[benjhayden, 2012/02/10 18:53:07]

Done.

+    // function downloadSubDirectoryFilename() {
+    //   var download_id = getNextId();
+    //   console.log("id: " + download_id);
+    //   var callbackCompleted = chrome.test.callbackAdded();
+    //   function myListener(delta) {
+    //     console.log("id: " + download_id);
+    //     if (delta.filename) console.log("filename: " + delta.filename.new);
+    //     if (delta.filename &&
+    //         delta.filename.new.indexOf('/foo/slow') !== -1) {
+    //       downloads.onChanged.removeListener(myListener);
+    //       callbackCompleted();
+    //     }
+    //   }
+    //   downloads.onChanged.addListener(myListener);
+    //   downloads.download(
+    //       {'url': SAFE_FAST_URL, 'filename': 'foo/slow'},
+    //       chrome.test.callback(function(id) {
+    //         console.log("id: " + download_id);
+    //         chrome.test.assertEq(download_id, id);
+    //       }));
+    // },
     function downloadInvalidFilename() {
       downloads.download(
           {'url': SAFE_FAST_URL, 'filename': '../../../../../etc/passwd'},
           chrome.test.callbackFail(ERROR_GENERIC));
-      // TODO(benjhayden): Give a better error message.
     },
     function downloadEmpty() {
       assertThrows(('Invalid value for argument 1. Property \'url\': ' +
                     'Property is required.'),
                    downloads.download, {});
     },
     function downloadInvalidSaveAs() {
       assertThrows(('Invalid value for argument 1. Property \'saveAs\': ' +
                     'Expected \'boolean\' but got \'string\'.'),
                    downloads.download,
                    {'url': SAFE_FAST_URL, 'saveAs': 'GOAT'});
     },
     function downloadInvalidHeadersOption() {
       assertThrows(('Invalid value for argument 1. Property \'headers\': ' +
                     'Expected \'array\' but got \'string\'.'),
                    downloads.download,
                    {'url': SAFE_FAST_URL, 'headers': 'GOAT'});
     },
     function downloadInvalidURL() {
       downloads.download(
           {'url': 'foo bar'},
           chrome.test.callbackFail(ERROR_INVALID_URL));
     },
     function downloadInvalidMethod() {
       assertThrows(('Invalid value for argument 1. Property \'method\': ' +
                     'Value must be one of: [GET, POST].'),
                    downloads.download,
                    {'url': SAFE_FAST_URL, 'method': 'GOAT'});
     },
-    function downloadSimple() {
-      downloads.download(
-          {'url': SAFE_FAST_URL},
-          chrome.test.callbackPass(function(id) {
-            chrome.test.assertEq(getNextId(), id);
-          }));
-    },
-    function downloadPost() {
-      downloads.download(
-          {'url': getURL('files/post/downloads/a_zip_file.js'),
-           'method': 'POST',
-           'body': 'WOOHOO'},
-          chrome.test.callbackPass(function(id) {
-              chrome.test.assertEq(getNextId(), id);
-          }));
-    },
-    function downloadHeader() {
-      downloads.download(
-          {'url': SAFE_FAST_URL,
-           'headers': [{'name': 'Foo', 'value': 'bar'}]
-          },
-          chrome.test.callbackPass(function(id) {
-            chrome.test.assertEq(getNextId(), id);
-          }));
-    },
-    function downloadInterrupted() {
-      // TODO(benjhayden): Find a suitable URL and test that this id is
-      // eventually interrupted using onChanged.
-      downloads.download(
-          {'url': SAFE_FAST_URL},
-          chrome.test.callbackPass(function(id) {
-            chrome.test.assertEq(getNextId(), id);
-          }));
-    },
     function downloadInvalidHeader() {
       downloads.download(
           {'url': SAFE_FAST_URL,
            'headers': [{ 'name': 'Cookie', 'value': 'fake'}]
         },
         chrome.test.callbackFail(ERROR_GENERIC));
-      // TODO(benjhayden): Give a better error message.
     },
     function downloadGetFileIconInvalidOptions() {
       assertThrows(('Invalid value for argument 2. Property \'cat\': ' +
                     'Unexpected property.'),
                    downloads.getFileIcon,
                    -1, {cat: 'mouse'});
     },
     function downloadGetFileIconInvalidSize() {
       assertThrows(('Invalid value for argument 2. Property \'size\': ' +
                     'Value must be one of: [16, 32].'),
                    downloads.getFileIcon, -1, {size: 31});
     },
     function downloadGetFileIconInvalidId() {
       downloads.getFileIcon(-42, {size: 32},
         chrome.test.callbackFail(ERROR_INVALID_OPERATION));
     },
     function downloadNoComplete() {
       // This is used partly to test cleanUp.
+      var download_id = getNextId();
+      console.log("id: " + download_id);
       downloads.download(
           {'url': NEVER_FINISH_URL},
           chrome.test.callbackPass(function(id) {
-            chrome.test.assertEq(getNextId(), id);
+            console.log("id: " + download_id);
+            chrome.test.assertEq(download_id, id);
           }));
     },
     function downloadPauseInvalidId() {
       downloads.pause(-42, chrome.test.callbackFail(ERROR_INVALID_OPERATION));
     },
     function downloadPauseInvalidType() {
       assertThrows(('Invalid value for argument 1. Expected \'integer\' ' +
                     'but got \'string\'.'),
                    downloads.pause,
                    'foo');
@@ skipping 18 matching lines @@
     },
     function cleanUp() {
       // cleanUp must come last. It clears out all in-progress downloads
       // so the browser can shutdown cleanly.
       for (var id = 0; id < nextId; ++id) {
         downloads.cancel(id, chrome.test.callbackPass(function() {}));
       }
     }
   ]);
 });