The test should just suppress the prompts and check that they are invoked in response to observer notifications when public downloads are present.

Comment on attachment 793848 [details] [diff] [review] Patch v1 Review of attachment 793848 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm @@ +109,5 @@ > */ > this.DownloadIntegration = { > // For testing only > _testMode: false, > + _testPromptDownloads: 0, Just declare a public variable, no need for a setter and getter. @@ +740,5 @@ > + // Handle test mode > + if (DownloadIntegration.testMode) { > + DownloadIntegration.testPromptDownloads = aDownloadsCount; > + return; > + } Move this before the previous "if", as we should also test cases where the prompt is not shown. ::: toolkit/components/jsdownloads/test/unit/test_DownloadObserver.js @@ +1,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +/** > + * Tests the DownloadIntegration object. Since DownloadObserver is just a helper in DownloadIntegration, the tests should be defined in test_DownloadIntegration.js. @@ +19,5 @@ > + * or already finished. The promiseDownloadStopped function can be > + * used to wait for completion. > + * @rejects JavaScript exception. > + */ > +function promiseStartDownload(aSourceUrl) { You can use promiseNewDownload and start the download yourself instead: mustInterruptResponses(); let download = promiseNewDownload(...); let promiseAttempt = download.start(); // ...test... continueResponses(); yield promiseAttempt; @@ +37,5 @@ > + * @return {Promise} > + * @resolves When the download has reached half of its progress. > + * @rejects Never. > + */ > +function promiseDownloadMidway(aDownload) { You don't need to wait for the download to reach half of its progress. @@ +67,5 @@ > +add_task(function test_publicDownloadNotifications() > +{ > + // Enable test mode for the _confirmCancelDownloads method to return > + // the number of downloads instead of showing the prompt to cancel or not. > + DownloadIntegration.dontLoad = false; I think we should find a way to register the observers while testing (even if dontLoad is true), without executing all the other initialization there. @@ +98,5 @@ > + do_check_eq(DownloadIntegration.testPromptDownloads, 1); > + > + DownloadIntegration.testPromptDownloads = 0; > + Services.obs.notifyObservers(cancelQuit, "offline-requested", null); > + do_check_eq(DownloadIntegration.testPromptDownloads, 1); These test cases are good, but we should also test the negative cases, where we don't show the prompt. We should also test with more than one download in the list (like one stopped and two in progress) and see that we still display the right count. @@ +114,5 @@ > + * offline request and exit private browsing observers are notified for a > + * private download. > + */ > +add_task(function test_privateDownloadNotifications() > +{ To avoid this test becoming mostly a cut-and-paste of the other, you can iterate in the previous one: add_task(function test_notifications() { for (let isPrivate of [false, true]) { //...test... } } And only handle the differences. We also need a test to see that we combine the count when we quit the application with both private and public downloads active at the same time.

Applied suggested changes and improved test coverage.

Comment on attachment 794424 [details] [diff] [review] Patch v2 Review of attachment 794424 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! I have just a few suggestions to simplify the tests. ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js @@ +38,5 @@ > + let cancelQuit = Cc["@mozilla.org/supports-PRBool;1"]. > + createInstance(Ci.nsISupportsPRBool); > + > + // Notify quit application requested observer. > + DownloadIntegration.testPromptDownloads = 0; Set to -1, to distinguish the not notified at all case from the 0 downloads case. Same below. @@ +209,5 @@ > + mustInterruptResponses(); > + > + for (let isPrivate of [false, true]) { > + let list = isPrivate ? yield promiseNewPrivateDownloadList(): > + yield promiseNewDownloadList(); nit: colon under question mark @@ +218,5 @@ > + let promiseAttempt2 = download2.start(); > + download3.start(); > + do_check_false(download1.stopped); > + do_check_false(download2.stopped); > + do_check_false(download3.stopped); nit: the "stopped" checks may be redundant, we already have other tests that verify that things work as expected here. @@ +225,5 @@ > + list.add(download1); > + list.add(download2); > + list.add(download3); > + let items = yield list.getAll(); > + do_check_eq(items.length, 3); nit: the same for the list items checks, also below.

Applied changes. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=90349083b431

Try is good https://tbpl.mozilla.org/?tree=Try&rev=90349083b431