Last Comment Bug 856043 - Create tests for no Restore Previous Session options in private windows
: Create tests for no Restore Previous Session options in private windows
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Private Browsing (show other bugs)
: unspecified
: All All
: P3 normal (vote)
: Firefox 23
Assigned To: Ioana (away)
:
: :Ehsan Akhgari
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-29 08:41 PDT by Ioana (away)
Modified: 2013-05-07 19:36 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testNoSessionRestoreMenuItem (3.04 KB, patch)
2013-04-25 10:02 PDT, Ioana (away)
ehsan: review-
Details | Diff | Splinter Review
testNoSessionRestoreMenuItem-v1 (4.18 KB, patch)
2013-04-26 03:36 PDT, Ioana (away)
ehsan: review+
Details | Diff | Splinter Review
testNoSessionRestoreMenuItem-v2 (4.11 KB, patch)
2013-04-26 07:43 PDT, Ioana (away)
ehsan: review+
Details | Diff | Splinter Review
testNoSessionRestoreButtons-v0 (6.06 KB, patch)
2013-04-30 08:38 PDT, Ioana (away)
ehsan: review+
Details | Diff | Splinter Review
testNoSessionRestoreButtons-v1 (5.49 KB, patch)
2013-05-02 05:11 PDT, Ioana (away)
no flags Details | Diff | Splinter Review
testNoSessionRestoreButtons-v1 (5.50 KB, patch)
2013-05-07 04:15 PDT, Ioana (away)
no flags Details | Diff | Splinter Review
testNoSessionRestoreMenuItem-v2 (3.22 KB, patch)
2013-05-07 05:12 PDT, Ioana (away)
no flags Details | Diff | Splinter Review

Description Ioana (away) 2013-03-29 08:41:04 PDT
STR:
1. Launch Firefox.
2. Open a private window (File menu or Ctrl+Shift+P / Cmd+Shift+P).
3. Load some web pages in new tabs, in the private window.
4. Close the private window (all, if you had more already opened).
5. Open a new private window.
6. Try to restore the previous private session.

Expected Results:
There are no Restore Previous Session options in the private window (in home page, history, about:sessionrestore).

Notes:
MozTrap test #6729.
https://bugzilla.mozilla.org/show_bug.cgi?id=819274#c13
Comment 1 Ioana (away) 2013-04-10 02:24:43 PDT
I ran into a problem while trying to verify that the Restore Previous Session option is disabled in the History menu. The 'disabled' property for this item is only set to 'true' after the History menu has been opened once.

The problem is that on Mac OSX, you can't click the main menu main items. You can click the History menu items (Show History, Restore Previous Session etc), but that does not initialize the 'disabled' property I need.

I had a short discussion with Gavin Sharp, which has worked on this, and he suggests we skip this part of the test on Mac.

I also posted this issue on dev-automation, but didn't get any suggestions.

Henrik, do you have any ideas whether I can open the History menu some way, or do you agree with Gavin?
Comment 2 Ioana (away) 2013-04-25 09:33:54 PDT
Considering bug 862371 comment 7, I am going to create a browser test for this test case too.

Considering that the 3 options are quite different, I think it would be better to create separate tests for them:
- 1 browser chrome test for the menu option
- 1 mochitest for the about:home option
- 1 mochitest for the about:sessionrestore option
Comment 3 Ioana (away) 2013-04-25 10:02:12 PDT
Created attachment 741916 [details] [diff] [review]
testNoSessionRestoreMenuItem

The first test in the list - the test that checks that restoring a session from the main menu is disabled.
Comment 4 :Ehsan Akhgari 2013-04-25 14:09:20 PDT
Comment on attachment 741916 [details] [diff] [review]
testNoSessionRestoreMenuItem

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

::: browser/components/privatebrowsing/test/browser/Makefile.in
@@ +31,5 @@
>  		browser_privatebrowsing_localStorage_before_after_page2.html \
>  		browser_privatebrowsing_localStorage_page1.html \
>  		browser_privatebrowsing_localStorage_page2.html \
>  		browser_privatebrowsing_nonbrowser.js \
> +               browser_privatebrowsing_noSessionRestoreMenuOption.js \

Nit: indentation.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js
@@ +16,5 @@
> +  }, false);
> +
> +  //Open a new PB window 
> +  //and check that the Session Restore menu option is not enabled
> +  let win2 = OpenBrowserWindow({private: true});

This creates two windows right after each other, and it's possible for each one of them to open up before the other, I think.  You probably want to make sure that the first window is fully opened and loaded before loading the second one.
Comment 5 Ioana (away) 2013-04-26 03:36:29 PDT
Created attachment 742293 [details] [diff] [review]
testNoSessionRestoreMenuItem-v1

Added the following:
- check that the first private window gets loaded
- wait for the first private window to get closed and confirm that the second one got loaded before testing the Session Restore option (as the manual test requires)

I ran the test locally (single and with the whole suite) several times and it passed each time.
Comment 6 Ioana (away) 2013-04-26 03:42:24 PDT
(In reply to Ioana Budnar, QA [:ioana] from comment #5)
> Added the following:
> - check that the first private window gets loaded
> - wait for the first private window to get closed and confirm that the
> second one got loaded before testing the Session Restore option (as the
> manual test requires)

Just to make sure it's clear:
These are the only checks that matter for my test. The two windows can be opened in parallel without issues. The purpose of the test is to make sure that the Session Store option doesn't show up in private mode after creating little history (fully loading and closing a page from the same type of session).
Comment 7 :Ehsan Akhgari 2013-04-26 06:28:20 PDT
Comment on attachment 742293 [details] [diff] [review]
testNoSessionRestoreMenuItem-v1

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

r=me with the comment below fixed.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_noSessionRestoreMenuOption.js
@@ +52,5 @@
> +  }, false);
> +
> +  //Wait until the first private window is closed, 
> +  //then check that the Session Restore menu option is not enabled the second one
> +  waitForWinClose(windowsCount()==1);

Hmm, this is a bit weird, why not just call testNoSessionRestoreMenuItem from the load handler after you call win.close()?  Then you should be able to just test to make sure windowsCount() returns 1, and get rid of the polling logic inside waitForWinClose.
Comment 8 Ioana (away) 2013-04-26 07:43:22 PDT
Created attachment 742366 [details] [diff] [review]
testNoSessionRestoreMenuItem-v2

Patch with the changes requested in the previous comment.
Comment 9 :Ehsan Akhgari 2013-04-26 09:45:33 PDT
Comment on attachment 742366 [details] [diff] [review]
testNoSessionRestoreMenuItem-v2

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

(FWIW when somebody r+'es one of your patches with comments, it means that they trust you to make those changes, so you don't need to ask for review again.  :-)
Comment 10 Ioana (away) 2013-04-29 00:36:47 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #9)
> Comment on attachment 742366 [details] [diff] [review]
> testNoSessionRestoreMenuItem-v2
> 
> Review of attachment 742366 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (FWIW when somebody r+'es one of your patches with comments, it means that
> they trust you to make those changes, so you don't need to ask for review
> again.  :-)

Sorry, didn't know that... Thanks for pointing it out :)
Comment 11 Ioana (away) 2013-04-30 08:38:58 PDT
Created attachment 743657 [details] [diff] [review]
testNoSessionRestoreButtons-v0

The remaining two tests for this bug.
Comment 12 :Ehsan Akhgari 2013-05-01 07:03:55 PDT
Comment on attachment 743657 [details] [diff] [review]
testNoSessionRestoreButtons-v0

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

::: browser/components/privatebrowsing/test/browser/Makefile.in
@@ +41,5 @@
>  		browser_privatebrowsing_placesTitleNoUpdate.html \
>  		browser_privatebrowsing_popupblocker.js \
>  		browser_privatebrowsing_protocolhandler.js \
>  		browser_privatebrowsing_protocolhandler_page.html \
> +                browser_privatebrowsing_ssAboutHomeButton.js \

Nit: call this browser_privatebrowsing_aboutHomeButtonAfterWindowClose.js, please.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_aboutSessionRestore.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// This test checks that the session restore button from about:sessionrestore
> +//is disabled in private mode

Nit: please use a space after // everywhere.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_ssAboutHomeButton.js
@@ +23,5 @@
> +            info("about:home got loaded");
> +            let sessionRestoreButton = win.gBrowser
> +                                          .contentDocument
> +                                          .getElementById("restorePreviousSession");
> +            is(window.getComputedStyle(sessionRestoreButton).display, 

win.getComputedStyle
Comment 13 Ioana (away) 2013-05-02 05:11:11 PDT
Created attachment 744583 [details] [diff] [review]
testNoSessionRestoreButtons-v1
Comment 14 Ioana (away) 2013-05-07 04:15:08 PDT
Created attachment 746336 [details] [diff] [review]
testNoSessionRestoreButtons-v1

Same patch, but with the bug number in the commit message.
Comment 15 Ioana (away) 2013-05-07 05:12:46 PDT
Created attachment 746354 [details] [diff] [review]
testNoSessionRestoreMenuItem-v2

Same patch, but with the bug number in the commit message.
Comment 16 Ioana (away) 2013-05-07 05:14:09 PDT
Checkin needed for both patches.
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-05-07 06:05:55 PDT
Comment on attachment 746336 [details] [diff] [review]
testNoSessionRestoreButtons-v1

gah

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