Keep track of Preferences dialog state in global variable

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: ashughes, Assigned: balazs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=whimboo][lang=js][good first bug], URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
RE: https://developer.mozilla.org/en/XUL/prefwindow#p-lastSelected

When testPaneRetention.js was originally created, we did not have persisted object in Mozmill.  Given our recent problems with window handling (bug 626674) we should move to remembering state of the preferences dialog in the persisted object.

Currently, we check against a string-literal to verify pane retention:
var prefPaneCheckCallback = function(controller)
{
  let prefDialog = new prefs.preferencesDialog(controller);
  controller.assertJS("subject.paneId == 'panePrivacy'",
                      {paneId: prefDialog.paneId});
  prefDialog.close();
} 

We should move to using the properties available to us, but we should retain these values in persisted storage:
prefDialog.currentPane.id === prefDialog.lastSelected
(Reporter)

Comment 1

8 years ago
Once we have these in persisted storage we can:

1. Open dialog
2. Switch pane
3. Save to persisted
4. Close dialog
5. Open dialog
6. Verify current values against persisted storage values
(Reporter)

Comment 2

7 years ago
This should be moved to Shared Modules or some other component -- I don't think Mozmill Tests is appropriate.
No, this is special to some tests but does not influence anything we want to do generally. Storing data in the persisted object should only be done if necessary.
(Reporter)

Comment 4

7 years ago
Given that, I don't think we need this in a separate bug. Each test which needs it should deal with it on their development bug.

On the other hand, if we have existing tests we want refactored, this should be moved into the refactor project.

Do you agree, Henrik?
Do whatever works best for you. But yeah, sounds good.
(Reporter)

Comment 6

7 years ago
Vlad, can you check if any Prefs tests will need to be refactored for this?
Summary: Keep track of Prefs dialog state in persisted object → Keep track of Preferences dialog state in persisted object
Whiteboard: [mentor=whimboo][lang=js][good first bug]

Comment 7

6 years ago
Hi, I would be interested in working on that bug. Could it be assigned to me?
Sure. You are welcome Theod! Doing it now.
Assignee: nobody → theod.moz
Status: NEW → ASSIGNED
Theod, are you still working on this bug?
Flags: needinfo?(theod.moz)
Given that we have not getting feedback from theod since 2 months on this bug, I'm going to put this bug back into the general bucket.
Assignee: theod.moz → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(theod.moz)
(Assignee)

Comment 11

5 years ago
Created attachment 808152 [details] [diff] [review]
store last selected paneId in persisted v1.0

Functional testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cbd2b0d
Attachment #808152 - Flags: review?(hskupin)
Comment on attachment 808152 [details] [diff] [review]
store last selected paneId in persisted v1.0

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

Given that the test itself is contained in a single file, there is no need anymore to necessarily use the persisted object. Just declare a new variable in setupModule, which will hold the current pane id. It will be accessible also from callback methods. Otherwise it looks good.
Attachment #808152 - Flags: review?(hskupin) → review-
(Assignee)

Comment 13

5 years ago
Created attachment 810186 [details] [diff] [review]
Store last selected paneId in setupModule variable

Moved lastSelectedPaneId variable from persisted storage to setupModule.

Functional testrun:
http://mozmill-crowd.blargon7.com/#/functional/report/837c1e0f361ac93453ee697219863b99
Attachment #808152 - Attachment is obsolete: true
Attachment #810186 - Flags: review?(hskupin)
Comment on attachment 810186 [details] [diff] [review]
Store last selected paneId in setupModule variable

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

Just a minor thing to update in this patch. Then we can get it landed. With the fix you get my r+. Thanks!

::: tests/functional/testPreferences/testPaneRetention.js
@@ +40,5 @@
>  
>    prefDialog.paneId = 'paneAdvanced';
>    prefDialog.paneId = 'panePrivacy';
> +
> +  // Store actual paneId in the persisted object

nit: We don't use the persisted object here anymore. So please update the comment to reflect that.
Attachment #810186 - Flags: review?(hskupin) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 815531 [details] [diff] [review]
Store last selected paneId in setupModule variable v2

Corrected the comment
Attachment #810186 - Attachment is obsolete: true
Attachment #815531 - Flags: review?(hskupin)
(Assignee)

Comment 16

5 years ago
The previous patch clearly applies on:
  default
  aurora
  mozilla-beta
  mozilla-release
  mozilla-esr24

I will create a backported patch for mozilla-esr17.
(Assignee)

Comment 17

5 years ago
Created attachment 815553 [details] [diff] [review]
Store last selected paneId in setupModule variable (mozilla-esr17) v1

Backported patch for (mozilla-esr17)
Attachment #815553 - Flags: review?(hskupin)
Comment on attachment 815531 [details] [diff] [review]
Store last selected paneId in setupModule variable v2

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

::: tests/functional/testPreferences/testPaneRetention.js
@@ +10,5 @@
>  var utils = require("../../../lib/utils");
>  
>  function setupModule(aModule) {
>    aModule.controller = mozmill.getBrowserController();
> +  aModule.lastSelectedPaneId = 'paneMain';

Sorry, that I missed this the last time. But we cannot initialize the variable with 'paneMain'. Reason is that this test is run whenever already other tests have been performed before, so it is not clear in which state the preferences pane currently is. Lets initialize is with undefined.
Attachment #815531 - Flags: review?(hskupin) → review-
Comment on attachment 815553 [details] [diff] [review]
Store last selected paneId in setupModule variable (mozilla-esr17) v1

This bug is not about a test failure, which would require us to backport a patch. It should only land on default.
Attachment #815553 - Attachment is obsolete: true
Attachment #815553 - Flags: review?(hskupin)
(Assignee)

Comment 20

5 years ago
Created attachment 816270 [details] [diff] [review]
Store last selected paneId in setupModule variable v3

initialize lastSelectedPaneId as undefined
Attachment #815531 - Attachment is obsolete: true
Attachment #816270 - Flags: review?(hskupin)
Comment on attachment 816270 [details] [diff] [review]
Store last selected paneId in setupModule variable v3

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

That looks fine now. Thanks Balazs!
Attachment #816270 - Flags: review?(hskupin) → review+
Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/ccb616a6ef8c
Assignee: nobody → juhaszbal
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Keep track of Preferences dialog state in persisted object → Keep track of Preferences dialog state in global variable
You need to log in before you can comment on or make changes to this bug.