665497 - Mozmill test for checking defaults for pref "Use Custom Settings for History"

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=16735

Comment 1

[u415141]

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

Comment 2

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.1

I'm currently checking the "checked" status of the options. Should I check the labels on them as well?
I'm only checking labels for the options with a dropdown. Should I check the labels on all options or only the selected one?

Ignore the format of the asserts for now, I'll change them in the new format in the next feedback? . Though, do they look ok?

Any other feedback is appreciated.

Comment 3

[u279076]

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

>+    var dtds = ["chrome://browser/locale/preferences/main.dtd",
>+		"chrome://browser/locale/preferences/tabs.dtd",
>+                "chrome://browser/locale/preferences/content.dtd",

Fix intendation

>+  var popupMenu = new elementslib.Selector(controller.window.document, '#historyMode');
>+  var customHistory = new elementslib.Selector(controller.window.document, "menuitem[value='custom']");

Can you make these variable names a bit more specific?

>+  // privateBrowsingAutoStart
>+  var privateBrowsingAutoStart = new elementslib.Selector(controller.window.document, '#privateBrowsingAutoStart');
>+  var pbState = privateBrowsingAutoStart.getNode().checked;

Same with these...

Fundamentally, I think your asserts are fine. I don't think you need to be checking the text of the selector label. This is something Mochitests should cover. We only need to verify the functionality is expected.

Comment 4

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.2

Changed the asserts to expects, and changed their messages.
Changed the ResetCallback for teardownModule because I wasn't leaving the profile completely clean in the last wip

(In reply to comment #3)
> Comment on attachment 544445 [details] [diff] [review] [review]
> patch v0.1
> Can you make these variable names a bit more specific?
Can you please provide some suitable examples?

Comment 5

[u279076]

In general, an element variable should be named based on it's element type and how it is unique from other elements of the same type.

>+  var popupMenu = new elementslib.Selector(controller.window.document, '#historyMode');

What specifically does this variable refer to? If it's a popup menu, what popup menu? 

>+  var customHistory = new elementslib.Selector(controller.window.document, "menuitem[value='custom']");

Same with this...I'm not clear about what customHistory refers to in the UI.

>+  // privateBrowsingAutoStart
>+  var privateBrowsingAutoStart = new elementslib.Selector(controller.window.document, '#privateBrowsingAutoStart');

Is this a menu item or something else? I can't tell from this line of code.

>+  var pbState = privateBrowsingAutoStart.getNode().checked;

For this one, simply expand out the acronym for the benefit of those who may not know what pb means: privateBrowsingState

Comment 6

[u279076]

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


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

Rename to testCustomHistorySettingsDefault

>+  var popupMenu = new elementslib.Selector(controller.window.document, '#historyMode');
>+  var customHistory = new elementslib.Selector(controller.window.document, "menuitem[value='custom']");

Please rename these variables to be more indicative of what elements they represent.

>+  // privateBrowsingAutoStart
>+  var privateBrowsingAutoStart = new elementslib.Selector(controller.window.document, '#privateBrowsingAutoStart');

Same with this variable...and any other variables in your tests.

>+  var pbState = privateBrowsingAutoStart.getNode().checked;

Any references to pb should be expanded to "privateBrowsing"

>+  // rememberHistory

Please be more specific with comments.

>+  expect.ok(downloadsState, "'Remember download history' is not checked");

Please indicate element type in your message.

>+  expect.equal(untilActual, untilExpected, "'Keep until:'"+ "got '" + untilActual + "', expected '" + untilExpected + "'" );

Please move the message to the next line and wrap as necessary.

>+  expect.equal(locbarActual, locbarExpected, "'When using the location bar, suggest:'"+ "got '" + locbarActual + "', expected '" + locbarExpected + "'" );

Same here.

Comment 7

[Alex Lakatos[:AlexLakatos]]

Attachment: patch v0.3

Changed the variable names. broke extremely long lines.

Comment 8

[u279076]

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

>+  var historyPopupMenu = new elementslib.Selector(controller.window.document, '#historyMode');
>+  var customHistoryOption = new elementslib.Selector(controller.window.document, "menuitem[value='custom']");

We should move the selectors into getter methods in the API. Can you file a bug for this? Please include all elements you've used 'elementslib' for in this code and other tests. Please also include Vlad on this bug so he can add his own instances.

>+  expect.equal(locbarActualLabel, locbarExpectedLabel, "'When using the location bar, suggest:'" +
>+               "got '" + locbarActualLabel + "', expected '" + locbarExpectedLabel + "'" );

Please use the following wrapping format:

expect.equal(locbarActualLabel, locbarExpectedLabel, 
             "'When using the location bar, suggest:' - got '" + locbarActualLabel + 
             "', expected '" + locbarExpectedLabel + "'" );

Please ask Geo for follow up review.

Comment 9

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

There is no need anymore to use get/expected in the message. That's done internally. Just say what you expect.

Comment 10

[u279076]

Any progress here?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.