Move a few Places xpcshell-tests from unit into unifiedcomplete

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: standard8, Assigned: leoneljara3, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla58
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [fxsearch][lang=js])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
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.
Mentor: standard8
Keywords: good-first-bug
Whiteboard: [fxsearch] → [fxsearch][lang=js]
(Assignee)

Comment 2

2 years ago
Hey, i am a bit new here. But do u mind if i take on this?
(Reporter)

Comment 3

2 years ago
Sure, there's no-one else on it yet afaik.
Assignee: nobody → leoneljara3

Comment 4

a year ago
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

a year ago
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.
check the manifest, you could have  to add firefox-appdir = browser to xpcshell.ini. compare the 2 manifests.
(Assignee)

Comment 7

a year ago
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.
(Reporter)

Comment 8

a year ago
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.
Flags: needinfo?(leoneljara3)
(Assignee)

Comment 9

a year ago
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
Flags: needinfo?(leoneljara3)
(Reporter)

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
Your help was really helpful
I merged our patches ran the test, everything passed. :P
Attachment #8920377 - Attachment is obsolete: true
Attachment #8920831 - Flags: review?(standard8)
(Reporter)

Comment 12

a year ago
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.
Attachment #8920831 - Flags: review?(standard8) → review+
(Reporter)

Comment 13

a year ago
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.
(Reporter)

Comment 14

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

Comment 15

a year ago
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.
https://hg.mozilla.org/mozilla-central/rev/a91380aed135
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.