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

RESOLVED FIXED

Status

Mozilla QA
Mozmill Tests
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: mihaelav, Assigned: cosmin)

Tracking

({regression, reproducible})

Version 2
regression, reproducible

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, firefox35 fixed, firefox36 fixed, firefox-esr31 unaffected)

Details

(Whiteboard: [mozmill-test-failure][sprint], URL)

Attachments

(1 attachment, 1 obsolete attachment)

3.06 KB, patch
Andrei Eftimie
: review+
Andrei Eftimie
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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
Smells like a localization issue for zh-CN on OS X.
Blocks: 1088500
Keywords: regression

Comment 2

4 years ago
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)
status-firefox34: --- → affected
status-firefox35: --- → affected
Keywords: reproducible
Whiteboard: [mozmill-test-failure]

Comment 3

4 years ago
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?
status-firefox33: --- → affected
status-firefox36: --- → affected
status-firefox-esr31: --- → affected
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
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
(Assignee)

Comment 6

4 years ago
No Andreea, this is different, I'll explain it in bug 1100953.
(Assignee)

Comment 7

4 years ago
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

4 years ago
(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.
(Assignee)

Comment 9

4 years ago
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.
Assignee: nobody → cosmin.malutan
Status: NEW → ASSIGNED
Whiteboard: [mozmill-test-failure] → [mozmill-test-failure][sprint]
(Assignee)

Comment 10

3 years ago
Created attachment 8527507 [details] [diff] [review]
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
Attachment #8527507 - Flags: review?(andrei.eftimie)
Attachment #8527507 - Flags: review?(andreea.matei)
Attachment #8527507 - Flags: feedback?(francesco.lodolo)
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
Attachment #8527507 - Flags: feedback?(francesco.lodolo) → feedback+

Comment 12

3 years ago
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
Attachment #8527507 - Flags: review?(andrei.eftimie)
Attachment #8527507 - Flags: review?(andreea.matei)
Attachment #8527507 - Flags: review-
(Assignee)

Comment 13

3 years ago
Created attachment 8528304 [details] [diff] [review]
patch v1.1

Fixed those nits.
Thanks
Attachment #8527507 - Attachment is obsolete: true
Attachment #8528304 - Flags: review?(andrei.eftimie)
Attachment #8528304 - Flags: review?(andreea.matei)

Comment 14

3 years ago
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)
Attachment #8528304 - Flags: review?(andrei.eftimie)
Attachment #8528304 - Flags: review?(andreea.matei)
Attachment #8528304 - Flags: review+
Attachment #8528304 - Flags: checkin+

Updated

3 years ago
status-firefox36: affected → fixed

Comment 15

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

Comment 16

3 years ago
All green on Beta, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/67e008dcdb37 (mozilla-beta)
status-firefox34: affected → fixed
status-firefox35: affected → fixed

Comment 17

3 years ago
All green on Release, transplanted:
https://hg.mozilla.org/qa/mozmill-tests/rev/63dd07038aa6 (mozilla-release)
status-firefox33: affected → fixed

Comment 18

3 years ago
ESR31 is not affected.
All is done here.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox-esr31: affected → unaffected
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.