Closed
Bug 628030
Opened 13 years ago
Closed 11 years ago
Keep track of Preferences dialog state in global variable
Categories
(Mozilla QA Graveyard :: Mozmill Tests, defect)
Mozilla QA Graveyard
Mozmill Tests
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: u279076, Assigned: balazs)
References
()
Details
(Whiteboard: [mentor=whimboo][lang=js][good first bug])
Attachments
(1 file, 4 obsolete files)
2.13 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
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
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
This should be moved to Shared Modules or some other component -- I don't think Mozmill Tests is appropriate.
Comment 3•13 years ago
|
||
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.
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?
Comment 5•13 years ago
|
||
Do whatever works best for you. But yeah, sounds good.
Vlad, can you check if any Prefs tests will need to be refactored for this?
Updated•11 years ago
|
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]
Hi, I would be interested in working on that bug. Could it be assigned to me?
Comment 8•11 years ago
|
||
Sure. You are welcome Theod! Doing it now.
Assignee: nobody → theod.moz
Status: NEW → ASSIGNED
Comment 10•11 years ago
|
||
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•11 years ago
|
||
Functional testrun: http://mozmill-crowd.blargon7.com/#/functional/report/1039ea48a9d69a5a1cc4fd228cbd2b0d
Attachment #808152 -
Flags: review?(hskupin)
Comment 12•11 years ago
|
||
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•11 years ago
|
||
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 14•11 years ago
|
||
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•11 years ago
|
||
Corrected the comment
Attachment #810186 -
Attachment is obsolete: true
Attachment #815531 -
Flags: review?(hskupin)
Assignee | ||
Comment 16•11 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•11 years ago
|
||
Backported patch for (mozilla-esr17)
Attachment #815553 -
Flags: review?(hskupin)
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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•11 years ago
|
||
initialize lastSelectedPaneId as undefined
Attachment #815531 -
Attachment is obsolete: true
Attachment #816270 -
Flags: review?(hskupin)
Comment 21•11 years ago
|
||
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+
Comment 22•11 years ago
|
||
Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/ccb616a6ef8c
Assignee: nobody → juhaszbal
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Summary: Keep track of Preferences dialog state in persisted object → Keep track of Preferences dialog state in global variable
Updated•5 years ago
|
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•