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

ASSIGNED
Assigned to

Status

()

Toolkit
Places
P3
normal
ASSIGNED
3 years ago
a year ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8571153 [details] [diff] [review]
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.
Attachment #8571153 - Flags: review?(mak77)
(Assignee)

Comment 2

3 years ago
Created attachment 8571155 [details] [diff] [review]
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.
(Assignee)

Comment 3

3 years ago
Created attachment 8571156 [details] [diff] [review]
use_default_engine.patch

Use the default engine by using the registration method defined in the js module for bug 1136019.
Attachment #8571156 - Flags: review?(mak77)
(Assignee)

Comment 4

3 years ago
Created attachment 8571158 [details] [diff] [review]
remove_firefox-appdir_in_places.patch

Just remove firefox-appdir from xpcshell.ini
Attachment #8571158 - Flags: review?(mak77)
(Assignee)

Comment 5

3 years ago
try server result with all patch sets.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93b834f287ca
(Assignee)

Updated

3 years ago
Attachment #8571153 - Attachment is patch: true

Updated

3 years ago
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
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.
Attachment #8571153 - Flags: review?(mak77) → review-
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... :(
Attachment #8571158 - Flags: review?(mak77) → review+
ehr I meant in xpcshell.ini... But I guess it's just for search.
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.
Attachment #8571156 - Flags: review?(mak77)
PS: I'm fine if you want to merge all the patches here into a single one. They're small enough.
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.