Closed Bug 715133 Opened 9 years ago Closed 9 years ago

Automated tests for inline auto complete

Categories

(Toolkit :: Places, defect)

12 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: ddahl, Assigned: ddahl)

References

Details

(Whiteboard: [inline-autocomplete:blocker])

Attachments

(1 file, 5 obsolete files)

This is a followup bug for the inline autocomplete work in bug 566489. We need a few browser chrome tests that make sure we don't regress this new code.
Assignee: nobody → ddahl
Depends on: 566489
no need for b-c tests, we can do xpcshell tests like the ones in toolkit/components/places/tests/autocomplete/
(In reply to Marco Bonardo [:mak] from comment #1)
> no need for b-c tests, we can do xpcshell tests like the ones in
> toolkit/components/places/tests/autocomplete/

Marco:

Can you describe the tests you need - its been a few weeks and I am hazy on what they need to do.
functional tests for autocomplete, add urls and check that they are reported in the correct order, that http:// and www. are ignored, that autocompleting after an existing host completes to the url and so on.
Depends on: 720110
Attached patch v 1 saving work (obsolete) — Splinter Review
Saving work. mak: how does this look?
Attachment #591650 - Flags: feedback?(mak77)
Blocks: 566489
No longer depends on: 566489
Whiteboard: [inline-autocomplete:blocker]
Comment on attachment 591650 [details] [diff] [review]
v 1 saving work

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

::: toolkit/components/places/tests/inline/test_autocomplete_functional.js
@@ +11,5 @@
> +{
> +  this.transitionType =
> +    aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
> +  this.visitDate = aVisitTime || Date.now() * 1000;
> +}

you can move these utils to the head_autocomplete.js file

@@ +19,5 @@
> +
> +add_autocomplete_test([
> +  "Add urls, check for correct order",
> +  "moz",
> +  "moz",

this looks wrong :)

btw, inline searches on the start of a domain (www excluded) so here you should likely search for "visit" or "vis" to get matches, not moz
the exepected value should be "visit2.mozilla.org" cause it's typed (for example)

@@ +49,5 @@
> +        do_throw("gHistory.updatePlaces() failed");
> +      },
> +      handleCompletion: function () {
> +        ensure_results("visit", "visit2.mozilla.org");
> +        run_next_test();

you should not run_next_test, the test harness takes care of that for you.

@@ +52,5 @@
> +        ensure_results("visit", "visit2.mozilla.org");
> +        run_next_test();
> +      }
> +    });
> +  }

You may add a addVisits util to head_autocomplete.js taking an array of objects with infos you need.
Attachment #591650 - Flags: feedback?(mak77)
Attached patch v 2 patch (obsolete) — Splinter Review
Let me know about further checks to make. I will play around with inline autocomplete later today and see what else i come up with.
Attachment #591650 - Attachment is obsolete: true
Attachment #591853 - Flags: review?(mak77)
Comment on attachment 591853 [details] [diff] [review]
v 2 patch

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

::: toolkit/components/places/tests/inline/test_autocomplete_functional.js
@@ +36,5 @@
> +      };
> +      places.push(place);
> +    });
> +
> +    gHistory.updatePlaces(places, FAKE_CALLBACK);

please wrap all of this in an addVisits helper function in head_autocomplete that just gets your urls array as input.

you don't need FAKE_CALLBACK just do gHistory.updatePlaces(places);

@@ +102,5 @@
> +
> +// 4. search for www.me should yield www.me.mozilla.org/
> +
> +add_autocomplete_test([
> +  "Autocompleting after an existing host completes to the url",

use a different description in any test, if not possible add an increasing number to the end, just to distinguish tests in the output

@@ +125,5 @@
> +      places.push(place);
> +    });
> +
> +    gHistory.updatePlaces(places, FAKE_CALLBACK);
> +  }

Add some test that adds a visit to a page and a bookmark to another one, and check the bookmark is returned

Also add tests for urls with a path, not just the domain, we autocomplete to the next slash depending on the search string
Attachment #591853 - Flags: review?(mak77)
Attached patch v 3 Patch (obsolete) — Splinter Review
Not sure why I was getting this error earlier: 

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "handleResult"' when calling method: [mozIVisitInfoCallback::handleResult]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "<unknown>"  data: no]
************************************************************
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'JavaScript component does not have a method named: "handleCompletion"' when calling method: [mozIVisitInfoCallback::handleCompletion]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "<unknown>"  data: no]
************************************************************

It is gone now though.

Addressed review comments.
Attachment #591853 - Attachment is obsolete: true
Attachment #591877 - Flags: review?(mak77)
Comment on attachment 591877 [details] [diff] [review]
v 3 Patch

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

Once these are addressed, and it passes, I'd say it'd be pretty good. We can add more tests while we proceed.

::: toolkit/components/places/tests/inline/head_autocomplete.js
@@ +27,5 @@
> +    aTransitionType === undefined ? TRANSITION_LINK : aTransitionType;
> +  this.visitDate = aVisitTime || Date.now() * 1000;
> +}
> +
> +function add_visits(aUrls)

just for coherence with addBookmark, make this addVisits

@@ +30,5 @@
> +
> +function add_visits(aUrls)
> +{
> +  let places = [];
> +  aUrls.forEach(function(url) {

nit: add a whitespace after "function" for anonymous functions

@@ +38,5 @@
> +                  visits: [
> +                    new VisitInfo(url.transition),
> +                  ],
> +    };
> +    places.push(place);

nit: you may avoid making the temp var and just
places.push({
  bla
  bla
});

::: toolkit/components/places/tests/inline/test_autocomplete_functional.js
@@ +15,5 @@
> +                { url: NetUtil.newURI("http://visit2.mozilla.org"),
> +                  transition: TRANSITION_TYPED,
> +                }];
> +
> +    add_visits(urls);

global-nit: you may avoid the newline before addVisit, or even directly passing the array as
addVisits([ {

          ]);
whatever, I don't mind

@@ +25,5 @@
> +  "visit1",
> +  "visit1.mozilla.org/",
> +  function () {
> +
> +    let urls = [{ url: NetUtil.newURI("http://www.visit1.mozilla.org"),

no newline after function() opening please, at a maximum if you want to increase redability move the opening brace to a new line

@@ +70,5 @@
> +  "bookmark",
> +  "bookmark.mozilla.org/",
> +  function () {
> +
> +    let urls = [{ url: NetUtil.newURI("http://bookmark.mozilla.org/foo"),

to better differentiate here you should maybe use bookmark1.mozilla.org, otherwise you can't tell what is returned (we always crop at the first / after your search string)

@@ +114,5 @@
> +               ];
> +
> +    add_visits(urls);
> +  }
> +]);

add a test that wants to complete the query string after a ? and maybe even one with a # fragment
Attachment #591877 - Flags: review?(mak77) → review+
Attached patch v 4 Patch comments addressed (obsolete) — Splinter Review
Comments addressed, all checks pass
Attachment #591877 - Attachment is obsolete: true
Attachment #591929 - Flags: review+
Attached patch v 4.1 Patch (obsolete) — Splinter Review
Forgot to hg qref...

mak: can you look at the last test that checks for #fragment ?
Attachment #591929 - Attachment is obsolete: true
Attachment #591931 - Flags: review+
Comment on attachment 591931 [details] [diff] [review]
v 4.1 Patch

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

::: toolkit/components/places/tests/inline/head_autocomplete.js
@@ +36,5 @@
> +                  uri: url.url,
> +                  title: "test for " + url.url,
> +                  visits: [
> +                    new VisitInfo(url.transition),
> +                  ],

indent just 2 spaces as usual all these properties

::: toolkit/components/places/tests/inline/test_autocomplete_functional.js
@@ +26,5 @@
> +  "visit1.mozilla.org/",
> +  function ()
> +  {
> +
> +    let urls = [{ url: NetUtil.newURI("http://www.visit1.mozilla.org"),

please avoid these newlines?

@@ +133,5 @@
> +]);
> +
> +add_autocomplete_test([
> +  "Check to make sure we autocomplete after #",
> +  "smokey.mozilla.org/foo?bacon=delicious#bar",

I'd probably search up to ba and let it complete to bar

Does it fail, why should I especially look at it?
(In reply to Marco Bonardo [:mak] from comment #12)
> Comment on attachment 591931 [details] [diff] [review]
> Does it fail, why should I especially look at it?
No failures. Just added another check and wanted you to peek at it before landing.
ping? can this land?
(In reply to Marco Bonardo [:mak] from comment #14)
> ping? can this land?

yes indeed.
This can be checked into m-c
Attachment #591931 - Attachment is obsolete: true
Attachment #597577 - Flags: review+
Thank you
https://hg.mozilla.org/integration/mozilla-inbound/rev/d34882ca9d04
Flags: in-testsuite+
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/d34882ca9d04
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.