781386 - MoTown shouldn't be loaded when running tests

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Causes all sorts of bad issues (tests hitting the network, leaks, etc. - see dependency list).

To make this happen I made a couple of changes:
a) add a social.skipLoadingProviders pref, that causes the SocialService to avoid loading providers from prefs. Set this pref in the test harness profile.
b) adjust the shareButton test to avoid depending on the default provider, by making it use runSocialTestWithProvider
c) remove the test-social-ui-ready code, and have runSocialTestWithProvider just unconditionally clobber Social.provider before it enables the social pref
d) (not strictly necessary) adjust runSocialTestWithProvider to make it handle removing the added provider, by passing a callback to be invoked when the caller wants the tests to end

c) means that the test harness now assumes that social is disabled by default. I don't think this is a problem, because I think that will always be true. It also means that during the test run, SocialUI_providerReady doesn't do any work, because Social.provider is not set on window load (there are no providers to load). Thankfully, the parts of SocialUI_providerReady that tests depend on are covered by the "social:pref-changed" handler, which is triggered by runSocialTestWithProvider setting the pref to true, so we're OK for the moment. In a followup, though, I'd like to consolidate these code paths, since the SocialUI_providerReady path is the path we take when normal users use the feature, and it is responsible for some initialization tasks that we should have tests for (like making sure that the "remove" menu item is initialized, etc.).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(this patch applies on top of bug 775779)

Comment 3

[Mark Hammond [:markh] [:mhammond]]

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

looks good, although I've still some concerns about the sidebar being left in a loaded state after the sidebar tests (ie, that 'function finishSocialTest()' should also reset the state of the sidebar and other social elements)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

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

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

::: browser/base/content/test/browser_shareButton.js
@@ -24,2 @@
>    registerCleanupFunction(function() {
> -    Services.prefs.clearUserPref(prefName);

This pref is set in line 154. With the knowledge that we have today, we may never intend to make the pref 'true' by default, but it seems like good manners to clear the user-defined pref on cleanup from the test.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Pushed to try, but the test now fails on Win/Linux because it's timing out waiting for checkNextInTabOrder. I think this is the same issue Jared ran into in bug 780010, so we'll probably want to rebase this on top of that (or vice versa?).

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

I think we should just pull the part of 780010 that tweaked checkNextInTabOrder into this patch. The issue was introduced now because the test is now receiving a profile for the user, thus more elements are appearing in the tab order of the share button.

This is the relevant part of the patch for bug 780010 that can be pulled in to this bug:

> function checkOKButton() {
>+  let displayName = document.getElementById("socialUserDisplayName");
>+  let okButton = document.getElementById("editSharePopupOkButton");
>+  let undoButton = document.getElementById("editSharePopupUndoButton");
>+
>   is(document.activeElement, okButton, "ok button should be focused by default");
>-  checkNextInTabOrder(undoButton, function () {
>-    checkNextInTabOrder(okButton, testCloseBySpace);
>-  });
>+  // Linux has the buttons in the [unshare] [ok] order, so displayName will come first.
>+  if (navigator.platform.indexOf("Linux") != -1) {
>+    checkNextInTabOrder(displayName, function () {
>+      checkNextInTabOrder(undoButton, function () {
>+        checkNextInTabOrder(okButton, testCloseBySpace);
>+      });
>+    });
>+  } else {
>+    checkNextInTabOrder(undoButton, function () {
>+      checkNextInTabOrder(displayName, function () {
>+        checkNextInTabOrder(okButton, testCloseBySpace);
>+      });
>+    });
>+  }
> }

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Made that tweak and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=38e771a59dbb

Comment 8

[:Gavin Sharp [email: [e-mail]]]

...and again with a dumb mistake fixed:
https://tbpl.mozilla.org/?tree=Try&rev=4db53660bc6f

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

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

># Parent  1ac9cb49c34132223836c8cc160e4bf4806f9c3a
>Bug 776554: add pref to avoid loading built-in providers during test runs, and other social service cleanup

FYI: The bug number in the comment doesn't match this bug.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Yeah, already fixed that locally :)

Comment 11

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e70e92f758

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b6e70e92f758

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mark Hammond (:markh) from comment #3)
> looks good, although I've still some concerns about the sidebar being left
> in a loaded state after the sidebar tests

Forgot to follow up in the bug, but Mark and I discussed this on IRC, and concluded that we're OK as-is.

(In reply to Jared Wein [:jaws] from comment #4)
> This pref is set in line 154. With the knowledge that we have today, we may
> never intend to make the pref 'true' by default, but it seems like good
> manners to clear the user-defined pref on cleanup from the test.

Oops, I missed this comment - I agree:
https://hg.mozilla.org/integration/mozilla-inbound/rev/038266727ddc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/038266727ddc