1138274 - xpcshell tests in toolkit/components/places/tests/unit/ should be independent from firefox

Status: ASSIGNED

(Toolkit :: Places, defect, P5)

Description

[Hiroyuki Ikezoe (:hiro)]

A patch I will post here depends on the patch for bug 1136019 (A js module to register default search engine for test).

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: set_preferences_for_test_000_frecency

test_000_frecency.js relies on various preference values in firefox.js.

This patch sets those preferences before running the test.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Attachment: move_test_parseSubmissionURL_basic.patch

test_PlacesSearchAutocompleteProvider.js seems to rely on the fact there are another engine other than the default engine. The default engine is removed in hide_search_engine_nomatch. If there is only the default engine, test_parseSubmissionURL_basic fails, so test_parseSubmissionURL_basic should be moved before hide_search_engine_nomatch.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: use_default_engine.patch

Use the default engine by using the registration method defined in the js module for bug 1136019.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: remove_firefox-appdir_in_places.patch

Just remove firefox-appdir from xpcshell.ini

Comment 5

[Hiroyuki Ikezoe (:hiro)]

try server result with all patch sets.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b834f287ca

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8571153 [details] [diff] [review]
set_preferences_for_test_000_frecency

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

These are not needed cause toolkit/places defines default values for them if they are missing.

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8571158 [details] [diff] [review]
remove_firefox-appdir_in_places.patch

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

not sure why this was added, maybe just for the search?
Would have been nice to annotate that with comments in moz.build... :(

Comment 8

[Marco Bonardo [:mak]]

ehr I meant in xpcshell.ini... But I guess it's just for search.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8571156 [details] [diff] [review]
use_default_engine.patch

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

::: toolkit/components/places/tests/unit/head_bookmarks.js
@@ +23,5 @@
> +  do_get_file("../../../search/tests/xpcshell/data/engine-default.xml").copyTo(appEngineDir, "engine-default.xml");
> +
> +  SearchTestUtils.registerTestSearchEngineDir([ appEngineDir ]);
> +  SearchTestUtils.setDefaultEnginePreference("Default search engine");
> +});

just a few tests depend on search engines... I think only the tests in unifiedcomplete folder...
So please move this there.
Exactly which test requires the existence of a default engine? Wouldn't be fine to not have any engine at all?

Also, please try to first land the module before reflagging for review, or it's hard for me to look at it.

Comment 10

[Marco Bonardo [:mak]]

PS: I'm fine if you want to merge all the patches here into a single one. They're small enough.