585724 - Make Mozmill-test testCheckItemHighlight local

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[u279076]

Module: testAwesomeBar/testCheckItemHighlight.js
Test-page: Use any 1 page from test-files/layout/mozilla*

Comment 1

[u279076]

Attachment: WIP Patch

Here is a WIP patch.  For all intents and purposes it works.  However, it creates a new failure: 
Timeout exceeded for 'subject.underlinedTextCount == 1'

This is because with the use of Mozilla pages, typing "mozilla" in the location bar displays the default bookmarks as well.  When we used Google, this was not a problem.

I'm not sure the best way to approach this scenario.

FYI, the original test is here:
http://hg.mozilla.org/qa/mozmill-tests/file/tip/firefox/testAwesomeBar/testCheckItemHighlight.js

Comment 2

[u279076]

I can use tabbedbrowsing/openinnewtab.html and "open" as the test string instead of using one of the Mozilla pages.  Using this page makes the test pass.  Is this a sufficient work around?

Comment 3

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

Comment on attachment 464654 [details] [diff] [review]
WIP Patch

>+const LOCAL_TEST_FOLDER = collector.addHttpResource('../test-files/');
>+const LOCAL_TEST_PAGES = [LOCAL_TEST_FOLDER + 'layout/mozilla.html',
>+                          'about:blank']

>+  for each (page in LOCAL_TEST_PAGES) {

Please add a var declaration.

>+  var testString = "mozilla";

You should not use mozilla. Instead I propose to load the mozilla_community.html or another page and use 'community' as a search term.

>+  for each (letter in testString) {

Here too.

>+  var titleEntries = locationBar.autoCompleteResults
>+                       .getUnderlinedText(richlistItem, "title");

Please correct the indentation.

>+  // Check that the underlined title matches the entered title
>+  for each (entry in titleEntries) {
>[..]
>+  var URLEntries = locationBar.autoCompleteResults
>+                     .getUnderlinedText(richlistItem, "url");
>[..]
>+  for each (entry in URLEntries) {

*snip*

Comment 4

[u279076]

Attachment: Patch v1 (default)

Comment 5

[Aaron Train [:aaronmt]]

Comment on attachment 464883 [details] [diff] [review]
Patch v1 (default)

>+  // Check that the underlined title matches the entered title
>+  for each (entry in titleEntries) {
>     controller.assertJS("subject.enteredTitle == subject.underlinedTitle",
>-                        {enteredTitle: testString, underlinedTitle: entry.toLowerCase()});
>+                        {enteredTitle: testString, 
>+                         underlinedTitle: entry.toLowerCase()});
>   }
>
var declaration in for each 
>+  // Check that the underlined URL matches the entered URL
>+  for each (entry in URLEntries) {
>     controller.assertJS("subject.enteredUrl == subject.underlinedUrl",
>-                        {enteredUrl: testString, underlinedUrl: entry.toLowerCase()});
>+                        {enteredUrl: testString, 
>+                         underlinedUrl: entry.toLowerCase()});
>   }
> }
and here too

r-, otherwise everything else is fine

Comment 6

[u279076]

Attachment: Patch v2 (default)

Comment 7

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

Comment on attachment 464924 [details] [diff] [review]
Patch v2 (default)

>+const LOCAL_TEST_PAGES = [LOCAL_TEST_FOLDER + 'layout/mozilla_community.html',
>+                          'about:blank']

Please the same indentation as in the other tests.

> var setupModule = function(module)
> {

Can you also move up the brackets?

>+  // Check that there is only 1 entry
>+  controller.waitForEval("subject.underlinedTextCount == 1", TIMEOUT, 100,
>+                         {underlinedTextCount: titleEntries.length});

Why waitForEval? AssertJS is sufficient here.

>     controller.assertJS("subject.enteredUrl == subject.underlinedUrl",
>-                        {enteredUrl: testString, underlinedUrl: entry.toLowerCase()});
>+                        {enteredUrl: testString, 
>+                         underlinedUrl: entry.toLowerCase()});

Having those constructs make it hard to read. What do you think about the following formatting for the future:

>     controller.assertJS("subject.enteredUrl == subject.underlinedUrl", {
>+      enteredUrl: testString, 
>+      underlinedUrl: entry.toLowerCase()
>+    });

Comment 8

[u279076]

> >     controller.assertJS("subject.enteredUrl == subject.underlinedUrl",
> >-                        {enteredUrl: testString, underlinedUrl: entry.toLowerCase()});
> >+                        {enteredUrl: testString, 
> >+                         underlinedUrl: entry.toLowerCase()});
> 
> Having those constructs make it hard to read. What do you think about the
> following formatting for the future:
> 
> >     controller.assertJS("subject.enteredUrl == subject.underlinedUrl", {
> >+      enteredUrl: testString, 
> >+      underlinedUrl: entry.toLowerCase()
> >+    });

I tend to prefer the following if were are going to modify the indentation:

controller.assertJS(
  "subject.enteredUrl == subject.underlinedUrl",
  {entered:Url: testString, underlinedUrl: entry.toLowercase()}
);

I'll let you make the final call before creating a new patch.

Comment 9

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

(In reply to comment #8)
> > >     controller.assertJS("subject.enteredUrl == subject.underlinedUrl", {
> > >+      enteredUrl: testString, 
> > >+      underlinedUrl: entry.toLowerCase()
> > >+    });
> 
> I tend to prefer the following if were are going to modify the indentation:
> 
> controller.assertJS(
>   "subject.enteredUrl == subject.underlinedUrl",
>   {entered:Url: testString, underlinedUrl: entry.toLowercase()}
> );
> 
> I'll let you make the final call before creating a new patch.

It doesn't help to make the object better readable. The expression string isn't that long in most cases but members of the passed in object shouldn't end up in a single line because they can be quite long. It makes it really hard to distinct between those members and values. That's why I only put the object members onto its own lines.

Geo, what do you think?

Comment 10

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

Cleaned up and uses the style we've been using and nitpicking at

Comment 11

[Aaron Train [:aaronmt]]

Attachment: Patch v3 - (default)

same thing, missed to remove module keywords in setupModule

Comment 12

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

Comment on attachment 478023 [details] [diff] [review]
Patch v3 - (default)

>+  controller.assertJS("subject.underliedTextCount == 1",
>+                      {underliedTextCount: titleEntries.length});
>+
>+  // Check that the underlined title matches the entered title
>+  for each (var entry in titleEntries) {
>     controller.assertJS("subject.enteredTitle == subject.underlinedTitle",
>-                        {enteredTitle: testString, underlinedTitle: entry.toLowerCase()});
>+                        {enteredTitle: testString, 
>+                         underlinedTitle: entry.toLowerCase()});
[..]
>+  // Check that the underlined URL matches the entered URL
>+  for each (var entry in URLEntries) {
>     controller.assertJS("subject.enteredUrl == subject.underlinedUrl",
>-                        {enteredUrl: testString, underlinedUrl: entry.toLowerCase()});
>+                        {enteredUrl: testString, 
>+                         underlinedUrl: entry.toLowerCase()});

Please always use the JSON indentation for object declarations. I would like to see that I do not have to mentioned it in upcoming reviews.

r=me with the update.

Comment 13

[Aaron Train [:aaronmt]]

Attachment: Patch v3.1 - (default) + [indent fix]

Comment 14

[u279076]

(In reply to comment #13)
> Created attachment 478271 [details] [diff] [review]
> Patch v3.1 - (default) + [indent fix]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/e00fd30835bd [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/746bd02483e5 [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/c52743a1f65d [mozilla1.9.1]

Comment 15

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

Move of Mozmill Test related project bugs to newly created components. You can
filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.