1403502 - Move a few Places xpcshell-tests from unit into unifiedcomplete

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P3)

Description

[Mark Banner (:standard8)]

Per discussion on irc with Marco, we should move:

test_PlacesSearchAutocompleteProvider.js
test_autocomplete_stopSearch_no_throw.js
test_history_autocomplete_tags.js

from toolkit/components/places/tests/unit into toolkit/components/places/tests/unifiedcomplete

This should be done after bug 1094903 has landed, to avoid conflicts.

Comment 1

[Mark Banner (:standard8)]

I'm happy to mentor this.

Basic steps (assuming you've built Firefox, an artifact build is fine):

1) Use 'hg mv' or 'git mv' to move the files mentioned in comment 0 to the new directory.
2) Update the xpcshell.ini files in each directory to move the files across.
3) Run

./mach xpcshell-test toolkit/components/places/tests/

and check the tests still succeed, if not, investigate & fix issues.

Comment 2

[Leonel Jara]

Hey, i am a bit new here. But do u mind if i take on this?

Comment 3

[Mark Banner (:standard8)]

Sure, there's no-one else on it yet afaik.

Comment 4

[Leo]

Well I seem I can not figure out this problem....I got two of js test files moved with no problems. But its the test_PlacesSearchAutocompleteProvider.js file that keeps timing out during ./mach xpcshell-test. After investigating, the function hide_search_engine_nomatch() calls Services.search.removeEngine() but of instead of having the desired affect of making the engine hidden, it instead removes the engine. Which in turn causes await promiseTopic to hang the entire test. I cannot really tell if defaultEngine is installed in a global location? So that removeEngine can just hide the engine, not remove.

Comment 5

[Leo]

I also found another problem, that PlacesSearchAutocompleteProvider.parseSubmissionURL(submissionURL) returns null to the variable result when called in the function test_parseSubmissionURL_basic(). Same file.

Comment 6

[Marco Bonardo [:mak]]

check the manifest, you could have  to add firefox-appdir = browser to xpcshell.ini. compare the 2 manifests.

Comment 7

[Leonel Jara]

After comparing the 2 manifests I did find that I do needed to add firefox-appdir to xpcshell.ini. 

I also found, that the head_autocomplete.js file specifically, ensure_search_engine() seems not to complement well with hide_search_engine_nomatch()(in file PlacesSearchAutocompleteProvider). 

It has something to do with the removal of the existing engines and the addition of its own test engine (MozSearch) in ensure_search_engine(). This was not present in the unit folder head_bookmarks.js file.

Comment 8

[Mark Banner (:standard8)]

Could you post the current WIP patch and also the error that you're getting?

I can see there might be an error somewhere, but it is hard to know what the issue is without having the actual error.

Comment 9

[Leonel Jara]

Attachment: Bug 1403502_WIP.patch - Moved test files, and edited manifests

Error that I get after running: 
./mach xpcshell-test toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js



 0:01.38 LOG: Thread-1 INFO (xpcshell/head.js) | test hide_search_engine_nomatch pending (2)
 0:01.38 LOG: Thread-1 INFO (xpcshell/head.js) | test run_next_test 3 finished (2)
 0:01.38 LOG: Thread-1 INFO "browser-search-engine-modified: engine-removed"
 0:01.38 LOG: Thread-1 INFO "browser-search-engine-modified: engine-changed"
 0:01.38 TEST_STATUS: Thread-1 hide_search_engine_nomatch FAIL [hide_search_engine_nomatch : 35] false == true
c:/mozilla_source/mozilla-central/obj-i686-pc-mingw32/_tests/xpcshell/toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js:hide_search_engine_nomatch:35
 0:01.38 LOG: Thread-1 INFO exiting test
 0:01.39 LOG: Thread-1 ERROR Unexpected exception 2147500036
undefined
 0:01.39 LOG: Thread-1 INFO exiting test

Comment 10

[Mark Banner (:standard8)]

Attachment: test_search_suggestions.js pref changes

Thank you Leonel, that was very helpful.

I've taking a look, and to fix test_PlacesSearchAutocompleteProvider.js, we need to do this:

-function run_test() {
+add_task(async function() {
   // Tell the search service we are running in the US.  This also has the
   // desired side-effect of preventing our geoip lookup.
   Services.prefs.setBoolPref("browser.search.isUS", true);
   Services.prefs.setCharPref("browser.search.countryCode", "US");
   Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", false);
-  run_next_test();
-}
+
+  Services.search.restoreDefaultEngines();
+  Services.search.resetToOriginalDefaultEngine();
+});

The first change here is to make it an async task, that's so it happens after the ensure_search_engine function in head.js. The Services.search.* calls then reset the state of the search engines to before ensure_search_engine was called.

One of the side-effects of adding "firefox-appdir = browser", is that test_search_suggestions.js is now failing. That's because adding that line into the xpcshell.ini file means the tests now pick up Firefox's preference set (firefox.js) as well as the default "global" set. That upsets a few of the tests in that file. It took a while to figure them out, so I'm attaching them in a patch.

If you can apply the changes above into your patch, and also incorporate the changes on my patch, then I think you should find the tests will all pass, and it'll be ready for review.

Let me know if you have any questions.

Comment 11

[Leonel Jara]

Attachment: Edited manifests and moved / fixed test files

Your help was really helpful
I merged our patches ran the test, everything passed. :P

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8920831 [details] [diff] [review]
Edited manifests and moved / fixed test files

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

Looks great, thank you for taking this on.

The only thing I'd say is that I'd change the patch summary to something like "Bug 1403502 - Move some xpcshell-tests from toolkit/components/places/unit to unifiedcomplete.". I'll do that when I land it in a couple of minutes.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8920831 [details] [diff] [review]
Edited manifests and moved / fixed test files

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

::: toolkit/components/places/tests/unifiedcomplete/test_PlacesSearchAutocompleteProvider.js
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  Cu.import("resource://gre/modules/PlacesSearchAutocompleteProvider.jsm");
>  
>  function run_test() {

Oh, I just realised, this run_test() function is not required now, as run_next_test gets called automatically by the test harness (it also causes an eslint failure when running lint - `./mach lint toolkit/components/places`).

I'll fix this on check in as well.

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a91380aed135113cf269371b4056ee637d907173
Bug 1403502 - Move some xpcshell-tests from toolkit/components/places/unit to unifiedcomplete. r=Standard8

Comment 15

[Mark Banner (:standard8)]

Leonel: this is now landed on our integration branch, assuming no issues show up in the full automated tests, then sometime in the next 24 hours this will be merged onto our main branch, at which time this bug will be marked as fixed.

Thank you for working on this.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a91380aed135