Closed Bug 1266783 Opened 4 years ago Closed 4 years ago

Create unit tests for testing locale specific search engines

Categories

(Firefox :: Search, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox47 --- fixed
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: mkaply, Assigned: florian)

Details

(Whiteboard: [test] fxsearch)

Attachments

(3 files, 4 obsolete files)

We have a google engine that is in the tree, but it is not used by the default locale.

As a result, it doesn't get tested.

We need to add a unit test that uses the geo specific code to add the hidden engine and verify it.
Attached patch First testcase pass (obsolete) — Splinter Review
This is my first pass at a unit test.

It borrows heavily from:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_hidden.js?force=1
Attachment #8744393 - Flags: feedback?(florian)
Attachment #8744393 - Attachment is patch: true
Attachment #8744393 - Attachment mime type: application/x-javascript → text/plain
https://reviewboard.mozilla.org/r/48519/#review45227

Looks reasonable overall.

::: browser/components/search/test/browser_google_codes.js:57
(Diff revision 1)
> +          .observe(null, "nsPref:changed", kLocalePref);
> +
> +  return promise;
> +}
> +
> +add_task(function* () {

The split in 3 parts would be more readable if you gave the tasks explicit names, eg:
 add_task(function* preparation() {

::: browser/components/search/test/browser_google_codes.js:59
(Diff revision 1)
> +  return promise;
> +}
> +
> +add_task(function* () {
> +  const kUrlPref = "geoSpecificDefaults.url";
> +  const BROWSER_SEARCH_PREF = "browser.search.";

These 2 constants should be shared at the top of the file.

::: browser/components/search/test/browser_google_codes.js:72
(Diff revision 1)
> +
> +  removeCacheFile();
> +
> +  // Geo specific defaults won't be fetched if there's no country code.
> +  Services.prefs.setCharPref("browser.search.geoip.url",
> +                             'data:application/json,{"country_code": "US"}');

Should we clear this at the end of the test?

::: browser/components/search/test/browser_google_codes.js:75
(Diff revision 1)
> +  // Geo specific defaults won't be fetched if there's no country code.
> +  Services.prefs.setCharPref("browser.search.geoip.url",
> +                             'data:application/json,{"country_code": "US"}');
> +
> +  Services.prefs.setBoolPref("browser.search.geoSpecificDefaults", true);
> +                             

Please ensure you remove all the trailing whitespace for the final version of the patch.

::: browser/components/search/test/browser_google_codes.js:128
(Diff revision 1)
> +  // Verify search service is not initialized
> +  is(Services.search.isInitialized, false, "Search service should NOT be initialized");
> +  removeCacheFile();
> +
> +  // Geo specific defaults won't be fetched if there's no country code.
> +  Services.prefs.clearUserPref("browser.search.geoip.url");

This line doesn't do anything, the url is on the default branch.

::: browser/components/search/test/browser_google_codes.js:135
(Diff revision 1)
> +  Services.prefs.clearUserPref("browser.search.geoSpecificDefaults");
> +
> +try {
> +// We can't set the pref back to the original because it references search.services.mozilla.com
> +// which causes the tests to fail.
> +//  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF).setCharPref(kUrlPref, originalGeoURL);

This line looks correct, why is it causing test failures? Services.prefs.clearUserPref("browser.search.geoSpecificDefaults"); should disable geo-defaults, right?
> This line looks correct, why is it causing test failures? Services.prefs.clearUserPref("browser.search.geoSpecificDefaults"); should disable geo-defaults, right?

That's what I thought, but when the search service is reinited by the next test (browser_healthreport), I get this failure:

FATAL ERROR: Non-local network connections are disabled and a connection attempt to search.services.mozilla.com (52.10.203.178) was made.
You should only access hostnames available via the test networking proxy (if running mochitests) or from a test-specific httpd.js server (if running xpcshell tests). Browser services should be disabled or redirected to a local server.
TEST-INFO | Main app process: exit 1
TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_healthreport.js | application terminated with exit code 1

Another test before that fails as well:

537 INFO TEST-UNEXPECTED-FAIL | browser/components/search/test/browser_healthreport.js | Current engine is Foo - Got Google, expected Foo

So clearly my cleanup isn't working as expected...
browser.search.geoSpecificDefaults defaults to true.

It's set to false for testing.

We set it to true for our testing, so when we reset the value, it goes back to true.

IT should go back to false...
Ah, I missed that http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js is setting prefs on the user branch.
OK, I think I got it all figured out.

The one weird thing I had to do was to cause a Sync reinit of search at the end, not an async reinit.

Async reinit reused the Google stuff from the testcases, which broke all the testcases after.
Attachment #8744393 - Attachment is obsolete: true
Attachment #8744401 - Attachment is obsolete: true
Attachment #8744393 - Flags: feedback?(florian)
Attachment #8744467 - Flags: review?(florian)
Comment on attachment 8744467 [details] [diff] [review]
Updated patch with comments addressed

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

Looks good overall, but having to force a sync re-init doesn't sound great, especially if we don't really know why it's needed.

::: browser/components/search/test/browser_google_codes.js
@@ +128,5 @@
> +
> +  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF).setCharPref(kUrlPref, originalGeoURL);
> +
> +  // Force sync reinit
> +  // Async reinit reuses the stored Google information

I would like to understand why. Seems like a bug.

@@ +129,5 @@
> +  Services.prefs.getDefaultBranch(BROWSER_SEARCH_PREF).setCharPref(kUrlPref, originalGeoURL);
> +
> +  // Force sync reinit
> +  // Async reinit reuses the stored Google information
> +  is(Services.search.currentEngine.name, "Google", "Search engine should be back to Google");

I don't think we want to hardcode the current default in a test that isn't actually about testing that. And are you sure it's always google? Is there something forcing isUS to be false?
Attachment #8744467 - Flags: feedback+
> I would like to understand why. Seems like a bug.

I'll do some more looking. I'm pretty stumped.

> I don't think we want to hardcode the current default in a test that isn't actually about testing that. And are you sure it's always google? Is there something forcing isUS to be false?

Honestly, just having Services.search.currentEngine is all we need to cause the reinit of the search service.

So I don't need a check at all.
Here's what happens after my test during the asyncReinit:

1128 INFO Console message: _asyncReInit
1129 INFO Console message: finalizing batch task
1130 INFO Console message: batchTask: Invalidating engine cache
1131 INFO Console message: _buildCache: Writing to cache file.
1132 INFO Console message: _asyncReInit
1133 INFO Console message: _init: Initing search plugin from [app]/chrome/en-US/locale/browser/searchplugins/google.xml
1134 INFO Console message: _parseImage: Image textContent: "data:image/x-icon;base64,AAABAAIAEBAAAAEAIABoBAAAJgAAACAgAAABACAAqBAAAI4EAAAoAAAAEAAAACAAAAABACAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA..."
1135 INFO Console message: _setIcon: Setting icon url "data:image/x-icon;base64,AAABAAIAEBAAAAEAIABoBAAAJgAAACAgAAABACAAqBAAAI4EAAAoAAAAEAAAACAAAAABACAAAAAAAAAEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA..." for engine "Google".
1136 INFO Console message: _addEngineToStore: Adding engine: "Google"
1137 INFO Console message: _asyncLoadEngines: loading user-installed engines from the obsolete cache
1138 INFO Console message: _buildCache: Writing to cache file.
MEMORY STAT | vsize 4517MB | residentFast 479MB | heapAllocated 194MB
1139 INFO TEST-OK | browser/components/search/test/browser_google_codes.js | took 54ms

Looking at the code, it seems to be going to the cache file even though I am explicitly removing it.
it's looking like race condition. When I start instrumenting search service, it goes away...
Attached patch Attempt to debug (obsolete) — Splinter Review
MozReview-Commit-ID: IpVnoRvjE9n
Assignee: mozilla → florian
Status: NEW → ASSIGNED
I'm to a point where I pass intermittently (by removing the Atomic TMP file).

The difference between passing and not passing is that if you do a check to see if the file exists after the file.remove in removeCacheFile, it didn't actually go away.

So I think we definitely have a race condition here where the cache file is still in use and doesn't delete.
Assignee: florian → mozilla
Priority: -- → P1
Whiteboard: [test] fxsearch
On my mac the test fails all the time for me. On Linux opt, it passes most of the time (fails about once in 50 runs). On Linux debug, it fails about half the time, which makes looking at strace for both cases possible. Attached is a diff of the strace for a passing and a failing run on the linux debug build.

The passing run is writing to the cache file one more time before we unlink it.
This is Mike's test that I've finished debugging and cleaned up a little bit, so r=me on this attachment.
Attachment #8746786 - Flags: feedback?(mozilla)
Attachment #8744969 - Attachment is obsolete: true
Assignee: mozilla → florian
Attachment #8744467 - Attachment is obsolete: true
Attachment #8744467 - Flags: review?(florian)
The _buildCache method is called either at the end of the initialization of the search service when the cache has to be rebuilt, or from a DeferredTask armed by any metadata change. Our test here was (sometimes) failing because after a re-initialization of the search service, the cache was written to the disk twice: once immediately at the end of the re-initialization, and a second time from the DeferredTask. The test was waiting for the cache to be flushed to disk before continuing, but wasn't waiting for a second flush... which could sometimes inconveniently happen right after we attempted to clear the cache by removing the file.

This double flush of the cache is completely useless, and the straightforward solution is to always disarm the DeferredTask (if it exists) from the _buildCache method.
Attachment #8746794 - Flags: review?(adw)
Comment on attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

r=me, the feedback request on Mike is just to double check that the changes I made make sense.
Attachment #8746786 - Flags: review+
Attachment #8746794 - Flags: review?(adw) → review+
Don't forget that for this to pass, it needs the new google.xml from release and for that to work, the other two google tests have to be updated as well. I'll attach a patch for the test update.
Ignore that last comment. Those two tests would use google-nocodes.

Note to future self - artifact builds use the wrong list.txt, so tests don't work properly.
Everything is testing great for me on fx-team.
https://hg.mozilla.org/integration/fx-team/rev/1647dd64832e5247d47340aefab91126a3267ef2
Bug 1266783 - prevent the search cache file from being flushed twice in a row after a re-initialization, r=adw.

https://hg.mozilla.org/integration/fx-team/rev/70443ad6bcc6c58f5e76ed71bf3f2bd075d17be8
Bug 1266783 - Add unit tests for Google engine with codes, r=florian.
https://hg.mozilla.org/mozilla-central/rev/1647dd64832e
https://hg.mozilla.org/mozilla-central/rev/70443ad6bcc6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Comment on attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

Approval Request Comment
[Feature/regressing bug #]: bug 1264786
[User impact if declined]: The changes to the Google search plugin landed in bug 1264786 may regress.
[Describe test coverage new/current, TreeHerder]: Current test coverage: none. The point of this patch is to add test coverage.
[Risks and why]: none for this specific patch as the change is tests-only, but we'll need the low risk associated search service patch.
[String/UUID change made/needed]: none.
Attachment #8746786 - Flags: feedback?(mozilla)
Attachment #8746786 - Flags: approval-mozilla-beta?
Attachment #8746786 - Flags: approval-mozilla-aurora?
Comment on attachment 8746794 [details] [diff] [review]
avoid double cache flush

Approval Request Comment
[Feature/regressing bug #]: Unclear, but likely a regression from bug 1203167, in the sense that the affected code didn't really exist or behave that way before bug 1203167.
[User impact if declined]: For users, small overhead during the first startup, due to pointlessly writing the search cache file a second time. This patch is required to make the browser_google_codes.js test pass consistently.
[Describe test coverage new/current, TreeHerder]: No test coverage for this specific change, but the code area it touches has a lot of xpcshell tests.
[Risks and why]: Low risk: self contained patch that will only prevent writing the search cache file twice in a row.
[String/UUID change made/needed]: none.
Attachment #8746794 - Flags: approval-mozilla-beta?
Attachment #8746794 - Flags: approval-mozilla-aurora?
Note: the patch from bug 1181645 should be uplifted at the same time as the patches here.
Comment on attachment 8746786 [details] [diff] [review]
Add unit tests for Google engine with codes,

Test changes are auto approved and do not need release management review.
Attachment #8746786 - Flags: approval-mozilla-beta?
Attachment #8746786 - Flags: approval-mozilla-aurora?
Comment on attachment 8746794 [details] [diff] [review]
avoid double cache flush

Search plugin related changes, Beta47+, Aurora48+
Attachment #8746794 - Flags: approval-mozilla-beta?
Attachment #8746794 - Flags: approval-mozilla-beta+
Attachment #8746794 - Flags: approval-mozilla-aurora?
Attachment #8746794 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.