1097064 - [zh-CN] Test failure "There are 3 opened tabs " in /testSessionStore/testRestorePreviousSession/test2.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Mihaela Velimiroviciu (:mihaelav)]

Module:    testRestorePreviousSession
Test:      /testSessionStore/testRestorePreviousSession/test2.js    
Failure:   There are 3 opened tabs
Branches:  Beta (34b8), Aurora (35.0a2)
Platforms: All

Reports: http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=35.0&platform=Mac&from=2014-11-04&to=2014-11-11&test=%2FtestSessionStore%2FtestRestorePreviousSession%2Ftest2.js&func=testRestorePreviousSession

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Smells like a localization issue for zh-CN on OS X.

Comment 2

[Andrei Eftimie]

This is across platforms.

With zh-CN, the default loaded page is "http://start.firefoxchina.cn/". When you issue the "Restore Session" command, this tab will not be replaced, so instead of 3 tabs we end up with 4.

If we load up "about:blank" when Firefox reopens in test2, everything works as expected.

I need to still dig a bit, to confirm that this is expected behaviour when restoring a session (I would expect it is: if I have new data when I want to restore a previous session, I don't expect to lose the new data)

Comment 3

[Andrei Eftimie]

We might want to change the startup preference, we're attempting to load this even for functional tests:

zh-CN: `browser.startup.homepage;http://start.firefoxchina.cn`
en-US: `browser.startup.homepage;about:home`

This does look like expected behaviour.

I
===
1. Open a couple tabs
2. Quit Firefox
3. Open Firefox
4. History > Restore Previous Session
Outcome: you have the tabs opened in step 1

II
===
1. Open a couple tabs
2. Quit Firefox
3. Open Firefox
4. Open a couple more (different tabs)
5. History > Restore Previous Session
Outcome: you have all tabs from step 4 + the ones from step 1

If this is an issue in Firefox it should be treated in a different place. The behaviour feels right to me (as in a Restore Session action should NOT be destructive).

Therefore I propose to set the preference `browser.startup.homepage` to `about:home` in mozmill.

(We can do a workaround if needed in the test itself until we get a new mozmill version out since 2.0.9 was recently released).

Does this approach sound ok Henrik?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would suggest to talk to the l10n team first, if it is ok to set the homepage not to about:home. I can see some consequences here in regards of tiles etc.

Comment 5

[Andreea Matei [:AndreeaMatei]]

Not sure if this failure is related, but failed only once on XP, en-US so i'll mention it here before opening a new bug if it fails more:
http://mozmill-release.blargon7.com/#/functional/report/a6906174787fc0d722108aa1fabf995c
Failure: Tab with URL 'http://localhost:43336/layout/mozilla.html' has been opened

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

No Andreea, this is different, I'll explain it in bug 1100953.

Comment 7

[Cosmin Malutan, [:cosmin-malutan]]

Andrei, I wouldn't change the preference, this will remove some localization configuration, it will make the test to ran like on en-US; then what's the point in running all locales. I think it will be best if we either change the preference for this test alone, or we properly set-up the Firefox in test2 setupModule.
Ideally closeAllTabs method should leave us in an pristine new tab, and that should be enough.

Comment 8

[Andrei Eftimie]

(In reply to Cosmin Malutan from comment #7)
> Ideally closeAllTabs method should leave us in an pristine new tab, and that
> should be enough.

So closeAllTabs at the beginning of test2 should revert the only tab to "about:newtab". Should have a similar effect to changing the homepage to be "about:home". Both approaches are fine for me for this test.

Comment 9

[Cosmin Malutan, [:cosmin-malutan]]

Sorry for that, it doesn't actually work, it still restores the closed tabs along with the one already opened. The only working solution that I could tested here so far was 'browser.startup.homepage' preference.

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

This will set the about:home as default homepage, for testRestorePreviousSession test. We need this so for cases like zh-CN the restored tabs will open over the already default opened tab, otherwise it will affect the test in terms of tabs numbers and order. 
This specific test doesn't test any localized items, but as Henrik suggested I would prefer to ask Francesco for a feedback.
http://mozmill-crowd.blargon7.com/#/functional/report/4b004f9a9a3a5bc3aa0f3a07c11bb8c3

Comment 11

[Francesco Lodolo [:flod]]

Comment on attachment 8527507 [details] [diff] [review]
patch v1.0

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

I'm fine with the approach. The different home page was set in bug 642106 (not sure why it's still open), and zh-CN is the only one with a different home page.
http://hg.mozilla.org/releases/l10n/mozilla-aurora/zh-CN/file/48e1590e7358/browser/firefox-l10n.js

Comment 12

[Andrei Eftimie]

Comment on attachment 8527507 [details] [diff] [review]
patch v1.0

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

Looks good, but need to fix a couple nits.

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession/test1.js
@@ +14,5 @@
>    BASE_URL + "layout/mozilla_projects.html",
>    BASE_URL + "layout/mozilla_organizations.html",
>  ];
>  
> +const BROWSER_HOME_PAGE = 'browser.startup.homepage';

nit: please use double quotes
also this being a preference the name should start with PREF_

@@ +20,3 @@
>  function setupModule(aModule) {
> +
> +  // Bug 1097064 - We set the default homepage to about:home so the restored

Put the description on a new line.
See https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide#Commenting

::: firefox/tests/functional/testSessionStore/testRestorePreviousSession/test2.js
@@ +9,4 @@
>  var tabs = require("../../../../lib/tabs");
>  var utils = require("../../../../../lib/utils");
>  
> +const BROWSER_HOME_PAGE = 'browser.startup.homepage';

nit: same here as in test1

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.1

Fixed those nits.
Thanks

Comment 14

[Andrei Eftimie]

Comment on attachment 8528304 [details] [diff] [review]
patch v1.1

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

https://hg.mozilla.org/qa/mozmill-tests/rev/bbc5a0b94732 (default)

Comment 15

[Andrei Eftimie]

Transplanted to Aurora:
https://hg.mozilla.org/qa/mozmill-tests/rev/aad70f313256 (mozilla-aurora)

Comment 16

[Andrei Eftimie]

All green on Beta, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/67e008dcdb37 (mozilla-beta)

Comment 17

[Andrei Eftimie]

All green on Release, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/63dd07038aa6 (mozilla-release)

Comment 18

[Andrei Eftimie]

ESR31 is not affected.
All is done here.