668154 - Use the same mock file picker for all mochitests and xpcshell tests

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Geoff Lankow (:darktrojan)]

There's currently at least 7 mock file pickers on the tree, and that's probably 6 too many. (Okay, one of them is /toolkit/content/tests/browser/common/mockFilePicker.js, which as at least reused once or twice.)

I propose creating a JSM in the mochikit extension, for ease of use in any of the mochitests. Thus, using a mock file picker will be a case of importing the module and resetting it, 2 lines of code instead of a few dozen.

Comment 1

[Geoff Lankow (:darktrojan)]

Looks like the same can be done for xpcshell tests. When I figure out how.

Comment 2

[(not currently active) Ted Mielczarek]

If you're going to do this for Mochitest, please make it work with SpecialPowers, so that we can continue removing enablePrivilege (bug 462483).

https://developer.mozilla.org/en/SpecialPowers

Feel free to ask here or on IRC if you need any help.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: patch

This cleans up all the mock file pickers I could find.

Comment 4

[Geoff Lankow (:darktrojan)]

Bah, forgot to qrefresh before exporting the patch, but I'm sure it won't be the final version.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 543330 [details] [diff] [review]
patch

I don't have time to give this a thorough review today, and I'm going to be on vacation for the next week, so I'm going to shift review over to jmaher. I'd be happy to provide feedback when I get back if you'd like, but I don't want to block your work.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 543330 [details] [diff] [review]
patch

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

this looks good.  Can you verify this passes on all platforms on try server?  I am not an expert in .jsm but nothing threw up a red flag for me.  The tests look great!

::: content/html/content/test/test_bug500885.html
@@ +29,5 @@
>  
>  function test() {
>    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>    var wu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                 .getInterface(Ci.nsIDOMWindowUtils);

SpecialPowers.DOMWindowUtils should do the trick here, but other parts of the file might need updating, we could save it for later if it isn't that simple.

::: layout/forms/test/test_bug36619.html
@@ +37,5 @@
>  var oldAllow = pbi.getBoolPref("dom.disable_open_during_load");
>  var oldShowBar = pbi.getBoolPref("privacy.popups.showBrowserMessage");
>  pbi.setBoolPref("dom.disable_open_during_load", true);
>  pbi.setBoolPref("privacy.popups.showBrowserMessage", false);
>  

can we use SpecialPowers here for the prefs?

@@ +42,4 @@
>  function checkFirst()
>  {
>    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +  ok(MockFilePicker.shown, "File picker show method should have been called");

do we still need the enablePrivilege call here?

@@ +53,2 @@
>    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +  ok(!MockFilePicker.shown, "File picker show method should not have been called");

do we still need the enablePrivilege call here?

@@ +55,3 @@
>  
>    pbi.setBoolPref("dom.disable_open_during_load", oldAllow);
>    pbi.setBoolPref("privacy.popups.showBrowserMessage", oldShowBar);

more prefs work that can be done with SpecialPowers

::: layout/forms/test/test_bug536567.html
@@ +18,5 @@
>  <script type="application/javascript">
>  
>  /** Test for Bug 536567 **/
>  
>  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");

do we need this enablePrivilege call

@@ +26,5 @@
>  const Cm = Components.manager;
>  
> +Cu.import("resource://mochikit/MockFilePicker.jsm");
> +MockFilePicker.reset();
> +

I have a patch which should land tonight or tomorrow that adds SpecialPowers to chrome tests in bug 666636.

::: toolkit/content/tests/browser/browser_save_resend_postdata.js
@@ +35,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
> +Components.utils.import("resource://mochikit/MockFilePicker.jsm");
> +MockFilePicker.reset();
> +

we should have specialpowers in browser-chrome after bug 666636 lands

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: patch

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: interdiff

Comment 9

[Geoff Lankow (:darktrojan)]

(In reply to comment #6)
> Comment on attachment 543330 [details] [diff] [review] [review]
> patch
> 
> Review of attachment 543330 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> this looks good.  Can you verify this passes on all platforms on try server?
> I am not an expert in .jsm but nothing threw up a red flag for me.  The
> tests look great!

Yes, everything passed on try.

> we should have specialpowers in browser-chrome after bug 666636 lands

Ack, forgot to do anything about that. I've got further work on  browser_save_resend_postdata.js in bug 668710 and test_bug536567.html will get looked at again anyway if someone goes through and removes Cc, Ci, etc.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 544964 [details] [diff] [review]
interdiff

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

thanks for the cleanup and the license header!

Comment 11

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f2390732b6a4

Comment 12

[Daniel Holbert [:dholbert]]

Backed out, as this appears to have turned mochitest-4 & mochitest-browser-chrome perma-orange with leaks:
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec45982514f4

Comment 13

[Geoff Lankow (:darktrojan)]

Oh, just realised I didn't do debug builds on try. Oops.

No idea what's causing the orange though. Help, anyone?

Comment 14

[Daniel Holbert [:dholbert]]

Leaks are harder to debug, so I'd suggest starting by debugging the actual test-failures (there are 2 unexpected failures in mochitest-oth, and they both say "Move DOWN to [something]" in the failure message).  If you're lucky, the fix for that may fix your leaks, too. :)

Sample test log with those failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1310421006.1310425263.18543.gz

Comment 15

[:Paolo Amadini]

(In reply to comment #13)
> Oh, just realised I didn't do debug builds on try. Oops.
> 
> No idea what's causing the orange though. Help, anyone?

I've not investigated this in detail, but that's probably due to the fact
that you're not unregistering the mock object factories after you register
them in the "reset" function of the JSM.

What you probably need here is to change the interface so that you have two
functions exposed by the JSM, "register" and "unregister", and the tests
should call them at the appropriate time. This also ensures that each test
properly cleans up the environment right after it's executed.

You can then use the functions in "mockObjects.js" to properly register and
unregister the mock object factory without leaks.

Comment 16

[:Paolo Amadini]

Side-notes: there's a patch in the works (bug 564387) where we don't want to
choose the file name, just ensure that the default file name is set correctly
on the file picker by the "Save Video As" code. Is it possible with the
current patch? We could add this feature in bug 564387 but it would be
nice if that's already supported by this implementation.

See line 91 here for what we want to do:

https://bugzilla.mozilla.org/attachment.cgi?id=545299&action=diff#a/toolkit/content/tests/browser/browser_save_video.js_sec1

In the download code, we also need to ensure that the value of the
"filterIndex" property is set to a specific value after "show" has been
called, regardless of what the code being tested set previously on the
same property:

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/tests/browser/browser_save_resend_postdata.js#116

If we don't do that, we save a complete web page instead of a single HTML
file. The test still succeeds but that's not what we want.

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

I've plugged the leak with extra calls to MockFilePicker.reset() at the end of tests that pass nsIFiles or callbacks to the JSM.

(In reply to comment #16)
> Side-notes: there's a patch in the works (bug 564387) where we don't want to
> choose the file name, just ensure that the default file name is set correctly
> on the file picker by the "Save Video As" code. Is it possible with the
> current patch? We could add this feature in bug 564387 but it would be
> nice if that's already supported by this implementation.
> 
> See line 91 here for what we want to do:
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=545299&action=diff#a/toolkit/
> content/tests/browser/browser_save_video.js_sec1

Do this:
MockFilePicker.showCallback = function(fp) {
  is(fp.file.leafName, .....
};

> In the download code, we also need to ensure that the value of the
> "filterIndex" property is set to a specific value after "show" has been
> called, regardless of what the code being tested set previously on the
> same property:
> 
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/tests/
> browser/browser_save_resend_postdata.js#116
> 
> If we don't do that, we save a complete web page instead of a single HTML
> file. The test still succeeds but that's not what we want.

Sorry, I missed that. I've changed the file picker's filterIndex property to a getter so this can be done.

Comment 18

[Daniel Holbert [:dholbert]]

Pushed newest patch to try-server as a sanity-check before re-landing:
 http://tbpl.mozilla.org/?tree=Try&rev=f932ff35de8f

Comment 19

[Daniel Holbert [:dholbert]]

Latest patch still fails tests in mochitest-4. (The oranges aren't visible on the tbpl link above yet, but I got tryserver emails with 3 instances of mochitest-4 failures so far)

Logs are:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f932ff35de8f/try-macosx64/try_leopard_test-mochitests-4-build452.txt.gz
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f932ff35de8f/try-macosx-debug/try_leopard-o-debug_test-mochitests-4-build191.txt.gz
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f932ff35de8f/try-linux-debug/try_fedora-debug_test-mochitests-4-build536.txt.gz

Test failures look like this:
1837 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | File picker should show the correct filter index - got 0, expected 1
1842 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | File picker should show the correct filter index - got 0, expected 1
1847 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_bug377624.html | File picker should show the correct filter index - got 0, expected 1

Comment 20

[Geoff Lankow (:darktrojan)]

(In reply to comment #19)
> Latest patch still fails tests in mochitest-4. (The oranges aren't visible
> on the tbpl link above yet, but I got tryserver emails with 3 instances of
> mochitest-4 failures so far)

Yes, I know what's going on there. I was going to say I'm glad it's not a test of MY sanity.

Comment 21

[Geoff Lankow (:darktrojan)]

Attachment: patch v4

Yay, fixed it again. No last-minute changes this time!

http://tbpl.mozilla.org/?tree=Try&rev=ee2f4266e04c (okay so I only tested linux64 but there's nothing even slightly platform related)

Comment 22

[:Paolo Amadini]

(In reply to comment #17)
> I've plugged the leak with extra calls to MockFilePicker.reset() at the end
> of tests that pass nsIFiles or callbacks to the JSM.

This still doesn't make sure that each test restores the same environment it
had before it started. This is an important property because it ensures that
tests don't depend on the order of execution.

If we want to have the mock file picker registered during all tests, then we
should probably do that in the testing framework, before any test runs.

> Do this:
> MockFilePicker.showCallback = function(fp) {
>   is(fp.file.leafName, .....
> };

Great, thanks!

>+function MockFilePickerInstance() { };
>+MockFilePickerInstance.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFilePicker]),
>+  init: function(aParent, aTitle, aMode) {
>+    MockFilePicker.mode = aMode;
>+    this.filterIndex = MockFilePicker.filterIndex;

This is too early: the code being tested sets the filterIndex property after
it calls "init". Given that we have "showCallback" available, I think we
could use it when the test code requires to change the value.

Comment 23

[:Paolo Amadini]

Bug 564387 is landing shortly, sorry for the race condition. It adds a new
browser test that calls the toolkit's mock file picker implementation. It's
the same call as the toolkit test so it should be trivial to unify.

Comment 24

[Geoff Lankow (:darktrojan)]

Attachment: patch v5

Hopefully everyone's stopped changing the files I'm working on (why do they suddenly change as soon as I want to patch them, after years of nothing?!!) and this will actually land.

Joel, this is practically identical to earlier patches, I'm just asking for review again to pick up any mistakes I made removing large amounts of bitrot.

Comment 25

[:Paolo Amadini]

Comment on attachment 551005 [details] [diff] [review]
patch v5

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

(In reply to Geoff Lankow (:darktrojan) from comment #24)
> Hopefully everyone's stopped changing the files I'm working on (why do they
> suddenly change as soon as I want to patch them, after years of nothing?!!)

Lol! :-) Thanks for the patience :-) I did a quick drive-by review for the
parts I know, and for the rest the patch seems OK if the implications of
comment 22 are understood.

::: browser/base/content/test/browser_save_video.js
@@ +43,5 @@
>  
> +    MockFilePicker.displayDirectory = destDir;
> +    MockFilePicker.filterIndex = 1;
> +    MockFilePicker.showCallback = function(fp) {
> +      fileName = fp.defaultString;

fp.filterIndex should be set inside showCallback, otherwise the code being tested overrides the value, and saves a complete web page instead of a single HTML file.

nit: please keep the comment saying that 1 is kSaveAsType_URL (by the way, the constant might also be available in this context).

::: toolkit/content/tests/browser/browser_save_resend_postdata.js
@@ +90,5 @@
>      var destDir = createTemporarySaveDirectory();
> +    var file = destDir.clone();
> +    file.append("no_default_file_name");
> +    MockFilePicker.returnFiles = [file];
> +    MockFilePicker.filterIndex = 1;

ditto

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 551005 [details] [diff] [review]
patch v5

>--- a/browser/base/content/test/browser_save_video.js
>+++ b/browser/base/content/test/browser_save_video.js
>@@ -1,17 +1,21 @@
> /* Any copyright is dedicated to the Public Domain.
>    http://creativecommons.org/publicdomain/zero/1.0/ */
> 
>+Components.utils.import("resource://mochikit/MockFilePicker.jsm");

You're leaking MockFilePicker into the global (browser.xul) scope here.

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 551005 [details] [diff] [review]
patch v5

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

My only feedback here is related to using the SpecialPowers version of MockFilePicker instead of importing the .jsm all the time.

::: browser/base/content/test/browser_save_video.js
@@ +1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> +Components.utils.import("resource://mochikit/MockFilePicker.jsm");

we can use SpecialPowers.MockFilePicker now

::: layout/forms/test/test_bug536567.html
@@ +23,5 @@
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  const Cm = Components.manager;
>  
> +Cu.import("resource://mochikit/MockFilePicker.jsm");

Why not use the SpecialPowers.MockFilePicker?

::: toolkit/content/tests/browser/browser_save_resend_postdata.js
@@ +33,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +Components.utils.import("resource://mochikit/MockFilePicker.jsm");

we can use SpecialPowers.MockFilePicker now

::: toolkit/mozapps/extensions/test/browser/browser_bug567127.js
@@ +3,5 @@
>   */
>  
>  // Tests bug 567127 - Add install button to the add-ons manager
>  
> +Components.utils.import("resource://mochikit/MockFilePicker.jsm");

we can use SpecialPowers.MockFilePicker now

::: toolkit/mozapps/extensions/test/browser/browser_inlinesettings.js
@@ +9,5 @@
>  var gProvider;
>  
>  const SETTINGS_ROWS = 8;
>  
> +Components.utils.import("resource://mochikit/MockFilePicker.jsm");

we can use SpecialPowers.MockFilePicker now

Comment 28

[Geoff Lankow (:darktrojan)]

I would, except in the browser-chrome tests, I get this:

> ReferenceError: SpecialPowers is not defined

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: patch v6

Serious bitrot removal. The only notable changes are the use of DOMWindowUtils in test_bug500885.html (as discussed on IRC), and how the MockFilePicker gets linked to SpecialPowers (there's probably a better way, I just did it so that it works).

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 568533 [details] [diff] [review]
patch v6

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

Overall, I would just like to ensure we are replacing set*Pref with pushPrefEnv.  Here is an example of using it:
http://mxr.mozilla.org/mozilla-central/source/layout/style/test/test_bug389464.html?force=1#34

Does this pass on try server, including android mochitests?

::: layout/forms/test/test_bug36619.html
@@ +29,5 @@
>  // enable popups the first time
> +var oldAllow = SpecialPowers.getBoolPref("dom.disable_open_during_load");
> +var oldShowBar = SpecialPowers.getBoolPref("privacy.popups.showBrowserMessage");
> +SpecialPowers.setBoolPref("dom.disable_open_during_load", true);
> +SpecialPowers.setBoolPref("privacy.popups.showBrowserMessage", false);

I would really like to use pushPrefEnv here instead of setBoolPref.

@@ +46,3 @@
>  
> +  SpecialPowers.setBoolPref("dom.disable_open_during_load", oldAllow);
> +  SpecialPowers.setBoolPref("privacy.popups.showBrowserMessage", oldShowBar);

using pushPrefEnv, will pop the set values from the stack and set the old values when the testcase (not harness) is finished.  We could avoid these two setBoolPref calls if we used pushPrefEnv above.

::: layout/forms/test/test_bug377624.html
@@ +37,5 @@
>  
>  // disable popups to make sure that the popup blocker does not interfere
>  // with manually opened file pickers.
> +var oldAllow = SpecialPowers.getBoolPref("dom.disable_open_during_load");
> +SpecialPowers.setBoolPref("dom.disable_open_during_load", false);

we should use pushPrefEnv here

Comment 31

[Geoff Lankow (:darktrojan)]

Attachment: patch v7

(In reply to Joel Maher (:jmaher) from comment #30)
> Overall, I would just like to ensure we are replacing set*Pref with
> pushPrefEnv.  Here is an example of using it:
> http://mxr.mozilla.org/mozilla-central/source/layout/style/test/
> test_bug389464.html?force=1#34

Okay, I've done that, and in the process reworked test_bug36619.html (it was a mess), so it'll need reviewing again. The file is probably easier to read than the diff, it's here: https://hg.mozilla.org/try/file/8438be99fdbc/layout/forms/test/test_bug36619.html

> Does this pass on try server, including android mochitests?

Yes it does, https://tbpl.mozilla.org/?tree=Try&rev=69ce98f1c3b6

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 568859 [details] [diff] [review]
patch v7

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

Thanks!

Comment 33

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/53ba7abbf1da

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/53ba7abbf1da