656255 - Have only one test function in testPaneRetention.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Alex Lakatos[:AlexLakatos]]

File: tests/functional/testPreferences/testPaneRetention.js

This test module consists of two tests:
 * testOpenCloseOptionsDialog()
 * testOptionsDialogRetention()

These should be split into two separate test modules.  I recommend moving testOpenCloseOptionsDialog() to its own module testOpenClosePane.js.

Comment 1

[Alex Lakatos[:AlexLakatos]]

* testOpenCloseOptionsDialog() - https://litmus.mozilla.org/show_test.cgi?id=15481

* testOptionsDialogRetention()-https://litmus.mozilla.org/show_test.cgi?id=15480

Comment 2

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/13265015

Comment 3

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v1.0

Comment 4

[u415141]

Alex Lakatos changed story state to started in Pivotal Tracker

Comment 5

[Alex Lakatos[:AlexLakatos]]

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

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

Comment 6

[u279076]

I'm not sure about the usefulness of testOpenCloseOptionsDialog(). This is being tested inherently through testOptionsDialogRetention(). For that matter, we might want to disable the Litmus test as well.

Henrik, please advise.

Comment 7

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

I agree. This test seems to be well covered by other existing tests in the BFT section as the pane retention one.

Comment 8

[Alex Lakatos[:AlexLakatos]]

Disabled https://litmus.mozilla.org/show_test.cgi?id=15481

Comment 9

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v2.0

Comment 10

[u279076]

Comment on attachment 531869 [details] [diff] [review]
patch v2.0

>  * The Initial Developer of the Original Code is Mozilla Foundation.
>  * Portions created by the Initial Developer are Copyright (C) 2009

Date should be updated to 2011.

> const gDelay = 0;
> const gTimeout = 5000;

Can you rename gDelay to DELAY and gTimeout to TIMEOUT? Also, if gDelay is not used anywhere in the test, simply remove it.

> var setupModule = function(module) {
>   module.controller = mozmill.getBrowserController();
> }

Can you change this from "module.controller = " to "controller ="?

>-var testOpenCloseOptionsDialog = function()
>-{
>-  // Reset pane to the main pane before starting the test
>-  prefs.openPreferencesDialog(controller, prefPaneResetCallback);
>-}

We should probably include this in a teardownModule() so the test exits in a clean state. You'll need to incorporate some aspect of prefPaneResetCallback. Actually you might be able to do this using the Prefs shared module. Henrik, please advise.

>  * Panes of preferences dialog should retain state when opened next time
>  */

I don't really like the wording of this. Can you change it to "Test the Preferences dialog retains state"?

> var testOptionsDialogRetention = function()
> {

Since we refer to the dialog as "Preferences" everywhere else, we should probably rename this function and the JS file.

>   // Choose the Privacy pane
>   prefs.openPreferencesDialog(controller, prefPaneSetCallback);
> 
>   // And check if the Privacy pane is still selected
>   prefs.openPreferencesDialog(controller, prefPaneCheckCallback);

Same with resetting the pane to Main, you can probably do this without the callback.  Henrik, please advise.
http://hg.mozilla.org/qa/mozmill-tests/file/9f5ffc869d1f/lib/prefs.js#l110
 
> /**
>  * Map test functions to litmus tests
>  */
>+// testOptionsDialogRetention.meta = {litmusids : [15480]};

I think this can be completely removed. Henrik, please advise.

Comment 11

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

(In reply to comment #10)
> > const gDelay = 0;
> > const gTimeout = 5000;
> 
> Can you rename gDelay to DELAY and gTimeout to TIMEOUT? Also, if gDelay is
> not used anywhere in the test, simply remove it.
> 
> > var setupModule = function(module) {
> >   module.controller = mozmill.getBrowserController();
> > }
> 
> Can you change this from "module.controller = " to "controller ="?

We do not want to completely rewrite those tests. It's really just a separation. Any further modifications we should do once we start with the transition to the new API.

> >-var testOpenCloseOptionsDialog = function()
> >-{
> >-  // Reset pane to the main pane before starting the test
> >-  prefs.openPreferencesDialog(controller, prefPaneResetCallback);
> >-}
> 
> We should probably include this in a teardownModule() so the test exits in a
> clean state. You'll need to incorporate some aspect of
> prefPaneResetCallback. Actually you might be able to do this using the Prefs
> shared module. Henrik, please advise.

Not sure, why an advise is requested here. 

> >   // Choose the Privacy pane
> >   prefs.openPreferencesDialog(controller, prefPaneSetCallback);
> > 
> >   // And check if the Privacy pane is still selected
> >   prefs.openPreferencesDialog(controller, prefPaneCheckCallback);
> 
> Same with resetting the pane to Main, you can probably do this without the
> callback.  Henrik, please advise.

Not sure to what you are referring to here. Any callback is necessary when opening the preferences dialog.

> I think this can be completely removed. Henrik, please advise.

This is clear and we have agreed on it already a while back.

Comment 12

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v4.0

Comment 13

[u279076]

Comment on attachment 532910 [details] [diff] [review]
patch v4.0

>+function prefPaneResetCallback(controller)
>+{
>+  let prefDialog = new prefs.preferencesDialog(controller);

Nit: Use "var" not "let".

Comment 14

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v5.0

Comment 15

[u279076]

Comment on attachment 533219 [details] [diff] [review]
patch v5.0

Looks fine to me, thanks. Over to Henrik for final review.

Comment 16

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/3217cb80f392 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/f697be187e0e (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/0d5c8722110a (beta)

Please update the Litmus with the new test path. Also, for the next patches please use hg export so your username and the commit message will appear in the patch. Thanks. Leaving open until the Litmus test has been updated.

Comment 17

[Alex Lakatos[:AlexLakatos]]

Updated https://litmus.mozilla.org/show_test.cgi?id=15480 .
Marking this RESOLVED FIXED