Last Comment Bug 781386 - MoTown shouldn't be loaded when running tests
: MoTown shouldn't be loaded when running tests
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 17
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on: 775779
Blocks: 776554 780884 781125
  Show dependency treegraph
 
Reported: 2012-08-08 17:03 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2012-08-14 08:29 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (13.28 KB, patch)
2012-08-08 17:05 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
mixedpuppy: review+
markh: review+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-08 17:03:22 PDT
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: gavin@gavinsharp.com] 2012-08-08 17:05:07 PDT
Created attachment 650370 [details] [diff] [review]
patch
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-08 17:07:25 PDT
(this patch applies on top of bug 775779)
Comment 3 Mark Hammond [:markh] 2012-08-08 17:46:38 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-08 19:18:08 PDT
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: gavin@gavinsharp.com] 2012-08-08 23:55:15 PDT
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 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-09 00:15:54 PDT
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: gavin@gavinsharp.com] 2012-08-09 11:54:43 PDT
Made that tweak and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=38e771a59dbb
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-09 15:41:13 PDT
...and again with a dumb mistake fixed:
https://tbpl.mozilla.org/?tree=Try&rev=4db53660bc6f
Comment 9 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2012-08-09 16:36:46 PDT
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: gavin@gavinsharp.com] 2012-08-09 16:56:25 PDT
Yeah, already fixed that locally :)
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-09 16:57:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e70e92f758
Comment 12 Ed Morley [:emorley] 2012-08-10 01:57:52 PDT
https://hg.mozilla.org/mozilla-central/rev/b6e70e92f758
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-11 20:57:52 PDT
(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] 2012-08-12 11:24:00 PDT
https://hg.mozilla.org/mozilla-central/rev/038266727ddc

Note You need to log in before you can comment on or make changes to this bug.