Closed Bug 585724 Opened 14 years ago Closed 14 years ago

Make Mozmill-test testCheckItemHighlight local

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: u279076, Assigned: u279076)

References

Details

(Whiteboard: [litmus-data])

Attachments

(3 files, 3 obsolete files)

Module: testAwesomeBar/testCheckItemHighlight.js
Test-page: Use any 1 page from test-files/layout/mozilla*
Blocks: 579965
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
Attached patch WIP Patch (obsolete) — Splinter Review
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
Attachment #464654 - Flags: feedback?(hskupin)
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 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*
Attachment #464654 - Flags: feedback?(hskupin) → feedback+
Attached patch Patch v1 (default) (obsolete) — Splinter Review
Attachment #464654 - Attachment is obsolete: true
Attachment #464883 - Flags: review?(aaron.train)
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
Attachment #464883 - Flags: review?(aaron.train) → review-
Attachment #464883 - Attachment is obsolete: true
Attachment #464924 - Flags: review?(aaron.train)
Attachment #464924 - Flags: review?(aaron.train) → review+
Attachment #464924 - Flags: review?(hskupin)
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()
>+    });
Attachment #464924 - Flags: review?(hskupin) → review-
> >     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.
(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?
Attached patch Patch v3 - (default) (obsolete) — Splinter Review
Cleaned up and uses the style we've been using and nitpicking at
Attachment #478021 - Flags: review?(anthony.s.hughes)
Attachment #478021 - Attachment is obsolete: true
Attachment #478021 - Flags: review?(anthony.s.hughes)
same thing, missed to remove module keywords in setupModule
Attachment #478023 - Flags: review?(anthony.s.hughes)
Attachment #478023 - Flags: review?(hskupin)
Attachment #478023 - Flags: review?(anthony.s.hughes)
Attachment #478023 - Flags: review+
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.
Attachment #478023 - Flags: review?(hskupin) → review+
(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]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Product: Testing → Mozilla QA
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: