MoTown shouldn't be loaded when running tests

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.).
Depends on: 775779
Created attachment 650370 [details] [diff] [review]
patch
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #650370 - Flags: review?(mixedpuppy)
Attachment #650370 - Flags: review?(mhammond)
(this patch applies on top of bug 775779)
Attachment #650370 - Flags: review?(mixedpuppy) → review+
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)
Attachment #650370 - Flags: review?(mhammond) → review+
No longer depends on: 775779
Depends on: 775779
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.
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?).
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);
>+      });
>+    });
>+  }
> }
Made that tweak and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=38e771a59dbb
...and again with a dumb mistake fixed:
https://tbpl.mozilla.org/?tree=Try&rev=4db53660bc6f
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.
Yeah, already fixed that locally :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e70e92f758
Target Milestone: --- → Firefox 17
https://hg.mozilla.org/mozilla-central/rev/b6e70e92f758
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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
https://hg.mozilla.org/mozilla-central/rev/038266727ddc
Blocks: 780884
You need to log in before you can comment on or make changes to this bug.