665496 - Mozmill test for checking "Remember history" is default setting for new profile

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Alex Lakatos[:AlexLakatos]]

Tracking bug for creating a mozmill test for https://litmus.mozilla.org/show_test.cgi?id=16737

Comment 1

[u415141]

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

Comment 2

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.1

Can you please give me all the feedback in one round instead of multiple rounds as in our previous patches? I would be grateful. Thanks

This patch contains the second patch of bug 661408, in order to work with Mozmill 1.5.4b3. It's applicable and works on a clean repo.

I've put this test under tests/functional/testHistory for now, but I think it belongs under testPreferences. What do you think?

Comment 3

[u279076]

(In reply to comment #2)
> I've put this test under tests/functional/testHistory for now, but I think
> it belongs under testPreferences. What do you think?

I agree -- please move to testPreferences/testRememberHistoryIsDefault.js

Comment 4

[u279076]

Comment on attachment 540470 [details] [diff] [review]
patch v0.1

># HG changeset patch
># User Alex Lakatos <[e-mail]>
>diff --git a/lib/modal-dialog.js b/lib/modal-dialog.js
>diff --git a/lib/utils.js b/lib/utils.js

I'm not 100% familiar with the changes you made to these modules or why they are necessary. Can you please ask Henrik for follow-up and explain why they are necessary? Thanks.

>diff --git a/tests/functional/testHistory/testDefaultSettingForHistory.js

>+  var defaultSetting = new elementslib.Selector(controller.window.document, '#historyMode');
>+  var defaultSettingLabel = defaultSetting.getNode().childNodes[0].childNodes[0].label;

This seems pretty dirty and fragile to me. There must be a better way to get the label for this element -- even if we have to add an XPath to the shared module.

>+  controller.assert(function () {
>+    return defaultSetting.getNode().value === 'remember';
>+  }, "History default setting - got '" + defaultSetting.getNode().value +
>+     "', expected 'remember'");
>+
>+  controller.assert(function () {
>+    return defaultSettingLabel === 'Remember history';
>+  }, "History default setting label - got '" + defaultSettingLabel +
>+     "', expected 'Remember history'");

When comparing element labels you should always grab the localized value from the appropriate dtd file. Check Security::testGreenLarry for examples on how to do that.

Comment 5

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.2

Comment 6

[u279076]

Comment on attachment 543752 [details] [diff] [review]
patch v0.2

>diff --git a/lib/prefs.js b/lib/prefs.js
>   getDtds : function preferencesDialog_getDtds() {
>-    return null;
>+    var dtds = ["chrome://browser/locale/preferences/main.dtd",
>+		"chrome://browser/locale/preferences/tabs.dtd",
>+                "chrome://browser/locale/preferences/content.dtd",
>+		"chrome://browser/locale/preferences/applications.dtd",
>+		"chrome://browser/locale/preferences/privacy.dtd",
>+		"chrome://browser/locale/preferences/security.dtd",
>+		"chrome://browser/locale/preferences/sync.dtd",
>+		"chrome://browser/locale/preferences/advanced.dtd"];
>+    return dtds;

I'm wondering if we could overload this to return a specific dtd based on a parameter. For example, getDTD(aPane) would return "chrome://browser/locale/preferences/" + aPane + ".dtd"
Henrik, please advise.

>+  controller.assert(function () {
>+    return defaultSetting.getNode().value === 'remember';
>+  }, "History default setting - got '" + defaultSetting.getNode().value +
>+     "', expected 'remember'");

Can you use the new assertions.Expect.equals() method from the Assertions module?

>+
>+  controller.assert(function () {
>+    return actualLabel === expectedLabel;
>+  }, "History default setting label - got '" + actualLabel + "', expected '" + expectedLabel +"'");

Same here...

Comment 7

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

(In reply to comment #6)
> >   getDtds : function preferencesDialog_getDtds() {
> >-    return null;
> >+    var dtds = ["chrome://browser/locale/preferences/main.dtd",
> >+		"chrome://browser/locale/preferences/tabs.dtd",
> >+                "chrome://browser/locale/preferences/content.dtd",
> >+		"chrome://browser/locale/preferences/applications.dtd",
> >+		"chrome://browser/locale/preferences/privacy.dtd",
> >+		"chrome://browser/locale/preferences/security.dtd",
> >+		"chrome://browser/locale/preferences/sync.dtd",
> >+		"chrome://browser/locale/preferences/advanced.dtd"];
> >+    return dtds;
> 
> I'm wondering if we could overload this to return a specific dtd based on a
> parameter. For example, getDTD(aPane) would return
> "chrome://browser/locale/preferences/" + aPane + ".dtd"
> Henrik, please advise.

DTDs can contain entities which are defined in another DTD. We should always try to return an array with all available DTD files to be on the safe side. So the current code is fine. Just adjust the indentation, Alex.

Comment 8

[Alex Lakatos[:AlexLakatos]]

(In reply to comment #7)

> DTDs can contain entities which are defined in another DTD. We should always
> try to return an array with all available DTD files to be on the safe side.
> So the current code is fine. Just adjust the indentation, Alex.

As I'm using this in two of my tests, I think I'll split out this part in a separate API bug. Is this ok?

Comment 9

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

Yes, please always do that.

Comment 10

[Alex Lakatos[:AlexLakatos]]

Logged Bug 670085 . Patch coming up in a few minutes.

Comment 11

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.3

Rewrote the asserts to expect and changed the messages.

Comment 12

[u279076]

Comment on attachment 545188 [details] [diff] [review]
patch v0.3

>+++ b/tests/functional/testPreferences/testDefaultSettingForHistory.js
>+/**
>+ * Test the Default setting for History
>+ */
>+function testDefaultSettingForHistory() {

Please rename to testDefaultHistorySettings

>+  var defaultSetting = new elementslib.Selector(controller.window.document, '#historyMode');
>+  var actualLabel = defaultSetting.getNode().label;
>+  var expectedLabel = utils.getEntity(prefDialog.getDtds(), 'historyHeader.remember.label');
>+  var actualValue = defaultSetting.getNode().value;
>+  var expectedValue = "remember";
>+
>+  expect.equal(actualValue, expectedValue, "Remember history value - "+ "got '" + actualValue + "', expected '" + expectedValue + "'" );
>+  expect.equal(actualLabel, expectedLabel, "Remember history label - "+ "got '" + locbarActual + "', expected '" + locbarExpected + "'" );

Please rearrange this code so that you get and assert your variables in a common flow (ie. get label, check label, get value, check value).

Comment 13

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.4

Made the necessary changes. Added comments. Broke the lines

Comment 14

[u279076]

Comment on attachment 545935 [details] [diff] [review]
patch v0.4

>+  // Get the default setting
>+  var defaultSetting = new elementslib.Selector(controller.window.document,
>+                                                '#historyMode');

Please indicate what default setting this is for in the variable name.

>+  // Check the label
>+  var actualLabel = defaultSetting.getNode().label;
>+  var expectedLabel = utils.getEntity(prefDialog.getDtds(),
>+                                      'historyHeader.remember.label');

Same here (and any other place in your code)

>+  expect.equal(actualLabel, expectedLabel, "Remember history label - "+ 
>+               "got '" + actualLabel + "', expected '" + expectedLabel + "'" );

The wrapping I was looking for is something like:

expect.equal(actualLabel, expectedLabel, 
             "Remember history label - got '" + actualLabel + 
             "', expected '" + expectedLabel + "'");

Please use this for your other assertions too.

Other than that, it looks ok. Please ask for follow up from Geo on this patch.

Comment 15

[u279076]

Any progress made here?

Comment 16

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

I don't see why this shouldn't be covered by a Mochitest. Maybe it has already been implemented.

If this request is still wanted for Firefox UI tests please reopen and give a reference to test steps.