Last Comment Bug 780010 - Update the browser_shareButton.js test to use common SocialAPI test functions
: Update the browser_shareButton.js test to use common SocialAPI test functions
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Mark Hammond [:markh]
:
Mentors:
Depends on: 776606
Blocks: 780987
  Show dependency treegraph
 
Reported: 2012-08-02 15:27 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-08-25 08:46 PDT (History)
1 user (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (8.10 KB, patch)
2012-08-02 15:27 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
Reinstate the share button test (6.88 KB, patch)
2012-08-22 23:31 PDT, Mark Hammond [:markh]
jaws: review+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-08-02 15:27:50 PDT
Created attachment 648517 [details] [diff] [review]
WIP

To match the other SocialAPI tests, we should rename browser_shareButton.js to browser_social_shareButton.js. The test should also be refactored to use the generic socialAPI functions defined in head.js.

I've attached a start of a patch that also sets a profile when testing the share button.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-03 01:14:58 PDT
Comment on attachment 648517 [details] [diff] [review]
WIP

>diff --git a/browser/base/content/test/browser_shareButton.js b/browser/base/content/test/browser_shareButton.js

> function test() {

>   registerCleanupFunction(function() {
>     Services.prefs.clearUserPref(prefName);
>-    gBrowser.removeTab(tab);
>   });

> function testDisable() {

>+  while (gBrowser.tabs.length > 1) {
>+    gBrowser.removeCurrentTab();
>+  }

Why this change? It's generally better for tests to explicitly remove the tabs they add, rather than introducing "catch all" cleanup code that might wallpaper over other issues.
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2012-08-03 01:21:18 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> Why this change? It's generally better for tests to explicitly remove the
> tabs they add, rather than introducing "catch all" cleanup code that might
> wallpaper over other issues.

I meant to undo this change. It was one of the things I was doing to try to narrow down the leaks. The next version of this patch won't have it in there.
Comment 3 Mark Hammond [:markh] 2012-08-22 23:31:28 PDT
Created attachment 654525 [details] [diff] [review]
Reinstate the share button test

fairly trivial update to the test to make things work.  FWIW, this patch is in the try run at https://tbpl.mozilla.org/?tree=Try&rev=1e0b679f0a6b
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-08-23 17:00:17 PDT
Comment on attachment 654525 [details] [diff] [review]
Reinstate the share button test

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

Please remove the comment in Makefile.in here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/Makefile.in#67
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-08-25 08:46:32 PDT
https://hg.mozilla.org/mozilla-central/rev/c58f5c9a5e0b

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